From b144b4b74ed1cdc74678aba6468fd2f835bd06a9 Mon Sep 17 00:00:00 2001
From: "sprang@webrtc.org" <sprang@webrtc.org>
Date: Tue, 3 Mar 2015 15:44:15 +0000
Subject: [PATCH] Fixed bug in SendTimeHistory, where deleting packets via the
 getter would not update the oldest suence number.

BUG=
R=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/42589004

Cr-Commit-Position: refs/heads/master@{#8574}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8574 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 .../bitrate_controller/send_time_history.cc   | 53 ++++++++++---------
 .../bitrate_controller/send_time_history.h    |  1 +
 .../send_time_history_unittest.cc             | 19 +++++++
 3 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/webrtc/modules/bitrate_controller/send_time_history.cc b/webrtc/modules/bitrate_controller/send_time_history.cc
index 62e730089..7e0c89e73 100644
--- a/webrtc/modules/bitrate_controller/send_time_history.cc
+++ b/webrtc/modules/bitrate_controller/send_time_history.cc
@@ -39,35 +39,35 @@ void SendTimeHistory::EraseOld(int64_t limit) {
   while (!history_.empty()) {
     auto it = history_.find(oldest_sequence_number_);
     assert(it != history_.end());
-    if (it->second <= limit) {
-      // Packet too old, remove it.
-      history_.erase(it);
-      // TODO(sprang): Warn if erasing (too many) old items?
-    } else {
-      // Oldest packet within age limit, return.
-      return;
-    }
 
-    if (history_.empty())
-      return;
+    if (it->second > limit)
+      return;  // Oldest packet within age limit, return.
 
-    // After removing 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).
-    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;
+    // 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::GetSendTime(uint16_t sequence_number,
                                   int64_t* timestamp,
                                   bool remove) {
@@ -75,8 +75,11 @@ bool SendTimeHistory::GetSendTime(uint16_t sequence_number,
   if (it == history_.end())
     return false;
   *timestamp = it->second;
-  if (remove)
+  if (remove) {
     history_.erase(it);
+    if (sequence_number == oldest_sequence_number_)
+      UpdateOldestSequenceNumber();
+  }
   return true;
 }
 
diff --git a/webrtc/modules/bitrate_controller/send_time_history.h b/webrtc/modules/bitrate_controller/send_time_history.h
index fb22c818a..883585635 100644
--- a/webrtc/modules/bitrate_controller/send_time_history.h
+++ b/webrtc/modules/bitrate_controller/send_time_history.h
@@ -29,6 +29,7 @@ class SendTimeHistory {
 
  private:
   void EraseOld(int64_t limit);
+  void UpdateOldestSequenceNumber();
 
   const int64_t packet_age_limit_;
   uint16_t oldest_sequence_number_;  // Oldest may not be lowest.
diff --git a/webrtc/modules/bitrate_controller/send_time_history_unittest.cc b/webrtc/modules/bitrate_controller/send_time_history_unittest.cc
index 40d8479a2..fc7099dbd 100644
--- a/webrtc/modules/bitrate_controller/send_time_history_unittest.cc
+++ b/webrtc/modules/bitrate_controller/send_time_history_unittest.cc
@@ -127,4 +127,23 @@ TEST_F(SendTimeHistoryTest, HistorySizeWithWraparound) {
   EXPECT_TRUE(history_.GetSendTime(1, &timestamp, false));
 }
 
+TEST_F(SendTimeHistoryTest, InterlievedGetAndRemove) {
+  const uint16_t kSeqNo = 1;
+  const int64_t kTimestamp = 2;
+
+  history_.AddAndRemoveOldSendTimes(kSeqNo, kTimestamp);
+  history_.AddAndRemoveOldSendTimes(kSeqNo + 1, kTimestamp + 1);
+
+  int64_t time = 0;
+  EXPECT_TRUE(history_.GetSendTime(kSeqNo, &time, true));
+  EXPECT_EQ(kTimestamp, time);
+
+  history_.AddAndRemoveOldSendTimes(kSeqNo + 2, kTimestamp + 2);
+
+  EXPECT_TRUE(history_.GetSendTime(kSeqNo + 1, &time, true));
+  EXPECT_EQ(kTimestamp + 1, time);
+  EXPECT_TRUE(history_.GetSendTime(kSeqNo + 2, &time, true));
+  EXPECT_EQ(kTimestamp + 2, time);
+}
+
 }  // namespace webrtc