Cleanup BWE SendTimeHistory class

Review-Url: https://codereview.webrtc.org/2011473002
Cr-Commit-Position: refs/heads/master@{#13590}

webrtc/modules/remote_bitrate_estimator/include/send_time_history.h

@@ -13,37 +13,42 @@
 
 #include <map>
 
-#include "webrtc/base/constructormagic.h"
 #include "webrtc/base/basictypes.h"
-#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/modules/include/module_common_types.h"
 
 namespace webrtc {
+class Clock;
+struct PacketInfo;
 
 class SendTimeHistory {
  public:
-  SendTimeHistory(Clock* clock, int64_t packet_age_limit);
+  SendTimeHistory(Clock* clock, int64_t packet_age_limit_ms);
-  virtual ~SendTimeHistory();
+  ~SendTimeHistory();
 
-  void AddAndRemoveOld(uint16_t sequence_number,
-                       size_t length,
-                       int probe_cluster_id);
-  bool OnSentPacket(uint16_t sequence_number, int64_t timestamp);
-  // Look up PacketInfo for a sent packet, based on the sequence number, and
-  // populate all fields except for receive_time. The packet parameter must
-  // thus be non-null and have the sequence_number field set.
-  bool GetInfo(PacketInfo* packet, bool remove);
   void Clear();
 
+  // Cleanup old entries, then add new packet info with provided parameters.
+  void AddAndRemoveOld(uint16_t sequence_number,
+                       size_t payload_size,
+                       int probe_cluster_id);
+
+  // Updates packet info identified by |sequence_number| with |send_time_ms|.
+  // Return false if not found.
+  bool OnSentPacket(uint16_t sequence_number, int64_t send_time_ms);
+
+  // Look up PacketInfo for a sent packet, based on the sequence number, and
+  // populate all fields except for arrival_time. The packet parameter must
+  // thus be non-null and have the sequence_number field set.
+  bool GetInfo(PacketInfo* packet_info, bool remove);
+
  private:
-  void EraseOld();
-  void UpdateOldestSequenceNumber();
-
   Clock* const clock_;
-  const int64_t packet_age_limit_;
+  const int64_t packet_age_limit_ms_;
-  uint16_t oldest_sequence_number_;  // Oldest may not be lowest.
+  SequenceNumberUnwrapper seq_num_unwrapper_;
-  std::map<uint16_t, PacketInfo> history_;
+  std::map<int64_t, PacketInfo> history_;
 
-  RTC_DISALLOW_COPY_AND_ASSIGN(SendTimeHistory);
+  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(SendTimeHistory);
 };
 
 }  // namespace webrtc

webrtc/modules/remote_bitrate_estimator/send_time_history.cc

@@ -8,93 +8,71 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include <assert.h>
-
 #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h"
 
+#include "webrtc/base/checks.h"
+#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
+#include "webrtc/system_wrappers/include/clock.h"
+
 namespace webrtc {
 
-SendTimeHistory::SendTimeHistory(Clock* clock, int64_t packet_age_limit)
+SendTimeHistory::SendTimeHistory(Clock* clock, int64_t packet_age_limit_ms)
-    : clock_(clock),
+    : clock_(clock), packet_age_limit_ms_(packet_age_limit_ms) {}
-      packet_age_limit_(packet_age_limit),
-      oldest_sequence_number_(0) {}
 
-SendTimeHistory::~SendTimeHistory() {
+SendTimeHistory::~SendTimeHistory() {}
-}
 
 void SendTimeHistory::Clear() {
   history_.clear();
 }
 
 void SendTimeHistory::AddAndRemoveOld(uint16_t sequence_number,
-                                      size_t length,
+                                      size_t payload_size,
                                       int probe_cluster_id) {
-  EraseOld();
+  int64_t now_ms = clock_->TimeInMilliseconds();
+  // Remove old.
+  while (!history_.empty() &&
+         now_ms - history_.begin()->second.creation_time_ms >
+             packet_age_limit_ms_) {
+    // TODO(sprang): Warn if erasing (too many) old items?
+    history_.erase(history_.begin());
+  }
 
-  if (history_.empty())
+  // Add new.
-    oldest_sequence_number_ = sequence_number;
+  int64_t unwrapped_seq_num = seq_num_unwrapper_.Unwrap(sequence_number);
-
+  int64_t creation_time_ms = now_ms;
-  history_.insert(std::pair<uint16_t, PacketInfo>(
+  constexpr int64_t kNoArrivalTimeMs = -1;  // Arrival time is ignored.
-      sequence_number, PacketInfo(clock_->TimeInMilliseconds(), 0, -1,
+  constexpr int64_t kNoSendTimeMs = -1;     // Send time is set by OnSentPacket.
-                                  sequence_number, length, probe_cluster_id)));
+  history_.insert(std::make_pair(
+      unwrapped_seq_num,
+      PacketInfo(creation_time_ms, kNoArrivalTimeMs, kNoSendTimeMs,
+                 sequence_number, payload_size, probe_cluster_id)));
 }
 
 bool SendTimeHistory::OnSentPacket(uint16_t sequence_number,
                                    int64_t send_time_ms) {
-  auto it = history_.find(sequence_number);
+  int64_t unwrapped_seq_num = seq_num_unwrapper_.Unwrap(sequence_number);
+  auto it = history_.find(unwrapped_seq_num);
   if (it == history_.end())
     return false;
   it->second.send_time_ms = send_time_ms;
   return true;
 }
 
-void SendTimeHistory::EraseOld() {
+bool SendTimeHistory::GetInfo(PacketInfo* packet_info, bool remove) {
-  while (!history_.empty()) {
+  RTC_DCHECK(packet_info);
-    auto it = history_.find(oldest_sequence_number_);
+  int64_t unwrapped_seq_num =
-    assert(it != history_.end());
+      seq_num_unwrapper_.Unwrap(packet_info->sequence_number);
-
+  auto it = history_.find(unwrapped_seq_num);
-    if (clock_->TimeInMilliseconds() - it->second.creation_time_ms <=
-        packet_age_limit_) {
-      return;  // Oldest packet within age limit, return.
-    }
-
-    // TODO(sprang): Warn if erasing (too many) old items?
-    history_.erase(it);
-    UpdateOldestSequenceNumber();
-  }
-}
-
-void SendTimeHistory::UpdateOldestSequenceNumber() {
-  // After removing an element from the map, update oldest_sequence_number_ to
-  // the element with the lowest sequence number higher than the previous
-  // value (there might be gaps).
-  if (history_.empty())
-    return;
-  auto it = history_.upper_bound(oldest_sequence_number_);
-  if (it == history_.end()) {
-    // No element with higher sequence number than oldest_sequence_number_
-    // found, check wrap around. Note that history_.upper_bound(0) will not
-    // find 0 even if it is there, need to explicitly check for 0.
-    it = history_.find(0);
-    if (it == history_.end())
-      it = history_.upper_bound(0);
-  }
-  assert(it != history_.end());
-  oldest_sequence_number_ = it->first;
-}
-
-bool SendTimeHistory::GetInfo(PacketInfo* packet, bool remove) {
-  auto it = history_.find(packet->sequence_number);
   if (it == history_.end())
     return false;
-  int64_t receive_time = packet->arrival_time_ms;
+
-  *packet = it->second;
+  // Save arrival_time not to overwrite it.
-  packet->arrival_time_ms = receive_time;
+  int64_t arrival_time_ms = packet_info->arrival_time_ms;
-  if (remove) {
+  *packet_info = it->second;
+  packet_info->arrival_time_ms = arrival_time_ms;
+
+  if (remove)
     history_.erase(it);
-    if (packet->sequence_number == oldest_sequence_number_)
-      UpdateOldestSequenceNumber();
-  }
   return true;
 }
 

webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc

@@ -14,6 +14,7 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h"
+#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/system_wrappers/include/clock.h"
 
 namespace webrtc {

webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h

@@ -18,6 +18,7 @@
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
 #include "webrtc/modules/include/module_common_types.h"
+#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h"
 
 namespace webrtc {