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
diff --git a/webrtc/modules/bitrate_controller/send_time_history.cc b/webrtc/modules/bitrate_controller/send_time_history.cc
index 62e7300..7e0c89e 100644
--- a/webrtc/modules/bitrate_controller/send_time_history.cc
+++ b/webrtc/modules/bitrate_controller/send_time_history.cc
@@ -39,35 +39,35 @@
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 @@
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 fb22c81..8835856 100644
--- a/webrtc/modules/bitrate_controller/send_time_history.h
+++ b/webrtc/modules/bitrate_controller/send_time_history.h
@@ -29,6 +29,7 @@
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 40d8479..fc7099d 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 @@
EXPECT_TRUE(history_.GetSendTime(1, ×tamp, 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
