Cleanup BWE SendTimeHistory class

webrtc/modules/remote_bitrate_estimator/send_time_history.cc

Index: webrtc/modules/remote_bitrate_estimator/send_time_history.cc
diff --git a/webrtc/modules/remote_bitrate_estimator/send_time_history.cc b/webrtc/modules/remote_bitrate_estimator/send_time_history.cc
index 2784df62040f46582b7718ec1f16272ddd76dcb5..e6285ba83c9813fa066bcf4a692d85b1b4ebc020 100644
--- a/webrtc/modules/remote_bitrate_estimator/send_time_history.cc
+++ b/webrtc/modules/remote_bitrate_estimator/send_time_history.cc
@@ -8,35 +8,44 @@
  *  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/include/module_common_types.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)
-    : clock_(clock),
-      packet_age_limit_(packet_age_limit),
-      oldest_sequence_number_(0) {}
+SendTimeHistory::SendTimeHistory(Clock* clock, int64_t packet_age_limit_ms)
+    : clock_(clock), packet_age_limit_ms_(packet_age_limit_ms) {}
 
-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();
-
-  if (history_.empty())
-    oldest_sequence_number_ = sequence_number;
+  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());
+  }
 
-  history_.insert(std::pair<uint16_t, PacketInfo>(
-      sequence_number, PacketInfo(clock_->TimeInMilliseconds(), 0, -1,
-                                  sequence_number, length, probe_cluster_id)));
+  // Add new.
+  int64_t creation_time_ms = now_ms;
+  constexpr int64_t kNoArrivalTimeMs = 0;  // Arrival time is ignored.

[stefan-webrtc, 2016/08/01 11:22:49]

Would -1 be a better number for an ignored arrival time?

[danilchap, 2016/08/01 11:47:08]

Used same value that was before. Since it is ignored, the actual value (0 or -1)
doesn't matter: user never can see it.
Having ignored as 0 and unset as  -1 distracts with a unimportant question
'why?'.
Made both -1 for consistency.

+  constexpr int64_t kNoSendTimeMs = -1;    // Send time is set by OnSentPacket.
+  history_.insert(std::make_pair(
+      sequence_number,
+      PacketInfo(creation_time_ms, kNoArrivalTimeMs, kNoSendTimeMs,
+                 sequence_number, payload_size, probe_cluster_id)));
 }
 
 bool SendTimeHistory::OnSentPacket(uint16_t sequence_number,
@@ -48,54 +57,25 @@ bool SendTimeHistory::OnSentPacket(uint16_t sequence_number,
   return true;
 }
 
-void SendTimeHistory::EraseOld() {
-  while (!history_.empty()) {
-    auto it = history_.find(oldest_sequence_number_);
-    assert(it != history_.end());
+bool SendTimeHistory::GetInfo(PacketInfo* packet_info, bool remove) {
+  RTC_DCHECK(packet_info);
+  auto it = history_.find(packet_info->sequence_number);
+  if (it == history_.end())
+    return false;
 
-    if (clock_->TimeInMilliseconds() - it->second.creation_time_ms <=
-        packet_age_limit_) {
-      return;  // Oldest packet within age limit, return.
-    }
+  // Save arrival_time not to overwrite it.
+  int64_t arrival_time_ms = packet_info->arrival_time_ms;
+  *packet_info = it->second;
+  packet_info->arrival_time_ms = arrival_time_ms;
 
-    // TODO(sprang): Warn if erasing (too many) old items?
+  if (remove)
     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;
+  return true;
 }
 
-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;
-  packet->arrival_time_ms = receive_time;
-  if (remove) {
-    history_.erase(it);
-    if (packet->sequence_number == oldest_sequence_number_)
-      UpdateOldestSequenceNumber();
-  }
-  return true;
+bool SendTimeHistory::SeqNoComparer::operator()(uint16_t lhs,
+                                                uint16_t rhs) const {
+  return IsNewerSequenceNumber(rhs, lhs);
 }
 
 }  // namespace webrtc