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..5c836476beeeaa007737f0e8fc29f4cc8d6565c3 100644
--- a/webrtc/modules/remote_bitrate_estimator/send_time_history.cc
+++ b/webrtc/modules/remote_bitrate_estimator/send_time_history.cc
@@ -8,35 +8,52 @@
  *  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),
-      packet_age_limit_(packet_age_limit),
-      oldest_sequence_number_(0) {}
+      packet_age_limit_ms_(packet_age_limit_ms),
+      oldest_packet_info_(history_.end()) {}
 
-SendTimeHistory::~SendTimeHistory() {
-}
+SendTimeHistory::~SendTimeHistory() {}
 
 void SendTimeHistory::Clear() {
   history_.clear();
+  oldest_packet_info_ = history_.end();
 }
 
 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() &&

[philipel, 2016/06/21 09:48:30]

Is the packet with the oldest sequence number also the oldest packet time wise?
If so, you can just iterate from 'history_.begin()' to find the range of packets
that should be erased. (Assuming you use the custom comparator).

[danilchap, 2016/06/21 11:45:30]

might not be the case when packets arrive out of order.

searching range to delete might be more efficient, but less readable. (since
search is done by some custom predicate instead of by key)
This function is called for every new packet, so with about same packet rate it
should delete 0 - 2 packets per call.

[philipel, 2016/06/21 12:07:14]

Acknowledged.

+         now_ms - oldest_packet_info_->second.creation_time_ms >
+             packet_age_limit_ms_) {
+    // TODO(sprang): Warn if erasing (too many) old items?
+    EraseOldestPacketInfo();

[philipel, 2016/06/21 09:48:30]

Remove 'EraseOldestPacketInfo' and just do 'history_.erase()' instead.

[danilchap, 2016/06/21 11:45:30]

Done.

+  }
 
-  if (history_.empty())
-    oldest_sequence_number_ = sequence_number;
+  RTC_DCHECK_EQ(history_.empty(), oldest_packet_info_ == history_.end());
 
+  // Add new.
+  int64_t creation_time_ms = now_ms;
+  constexpr int64_t kNoArrivalTimeMs = 0;  // Arrival time is ignored.
+  constexpr int64_t kNoSendTimeMs = -1;    // Send time is set by OnSentPacket.
   history_.insert(std::pair<uint16_t, PacketInfo>(
-      sequence_number, PacketInfo(clock_->TimeInMilliseconds(), 0, -1,
-                                  sequence_number, length, probe_cluster_id)));
+      sequence_number,
+      PacketInfo(creation_time_ms, kNoArrivalTimeMs, kNoSendTimeMs,
+                 sequence_number, payload_size, probe_cluster_id)));
+
+  // Set as oldest if it was the first packet info.
+  if (oldest_packet_info_ == history_.end())
+    oldest_packet_info_ = history_.begin();
 }
 
 bool SendTimeHistory::OnSentPacket(uint16_t sequence_number,
@@ -48,52 +65,29 @@ 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());
-
-    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::EraseOldestPacketInfo() {
+  RTC_DCHECK(oldest_packet_info_ != history_.end());
+  history_.erase(oldest_packet_info_++);
+  if (oldest_packet_info_ == history_.end())
+    oldest_packet_info_ = history_.begin();  // Wrap around.
 }
 
-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);
+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;
-  int64_t receive_time = packet->arrival_time_ms;
-  *packet = it->second;
-  packet->arrival_time_ms = receive_time;
+
+  // 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;
+
   if (remove) {
-    history_.erase(it);
-    if (packet->sequence_number == oldest_sequence_number_)
-      UpdateOldestSequenceNumber();
+    if (it == oldest_packet_info_)
+      EraseOldestPacketInfo();
+    else
+      history_.erase(it);
   }
   return true;
 }