NetEq: Rectify the implementation of PacketBuffer::DiscardOldPackets
The implementation of this method did not follow the description in
the method comment. It was supposed to delete all packets in a range
[A, B], but if at least one packet in the buffer had a timestamp lower
than A, then no packets at all were discarded. This is now fixed.
BUG=webrtc:7937
Review-Url: https://codereview.webrtc.org/2969123003
Cr-Commit-Position: refs/heads/master@{#18903}
diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer.cc b/webrtc/modules/audio_coding/neteq/packet_buffer.cc
index cea6b3d..4c8c462 100644
--- a/webrtc/modules/audio_coding/neteq/packet_buffer.cc
+++ b/webrtc/modules/audio_coding/neteq/packet_buffer.cc
@@ -221,15 +221,13 @@
void PacketBuffer::DiscardOldPackets(uint32_t timestamp_limit,
uint32_t horizon_samples,
StatisticsCalculator* stats) {
- // TODO(minyue): the following implementation is wrong. It won't discard
- // old packets if the buffer_.front() is newer than timestamp_limit -
- // horizon_samples. https://bugs.chromium.org/p/webrtc/issues/detail?id=7937
- while (!Empty() && timestamp_limit != buffer_.front().timestamp &&
- IsObsoleteTimestamp(buffer_.front().timestamp, timestamp_limit,
- horizon_samples)) {
- if (DiscardNextPacket(stats) != kOK) {
- assert(false); // Must be ok by design.
- }
+ const size_t old_size = buffer_.size();
+ buffer_.remove_if([timestamp_limit, horizon_samples](const Packet& p) {
+ return timestamp_limit != p.timestamp &&
+ IsObsoleteTimestamp(p.timestamp, timestamp_limit, horizon_samples);
+ });
+ if (old_size > buffer_.size()) {
+ stats->PacketsDiscarded(old_size - buffer_.size());
}
}
diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc b/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
index 82f7246..103c3aa 100644
--- a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
@@ -22,6 +22,8 @@
using ::testing::Return;
using ::testing::StrictMock;
using ::testing::_;
+using ::testing::InSequence;
+using ::testing::MockFunction;
namespace webrtc {
@@ -314,26 +316,40 @@
// Discard them one by one and make sure that the right packets are at the
// front of the buffer.
constexpr int kDiscardPackets = 5;
+
+ // Interleaving the EXPECT_CALL sequence with expectations on the MockFunction
+ // check ensures that exactly one call to PacketsDiscarded happens in each
+ // DiscardNextPacket call.
+ InSequence s;
+ MockFunction<void(int check_point_id)> check;
for (int i = 0; i < kDiscardPackets; ++i) {
uint32_t ts;
EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&ts));
EXPECT_EQ(current_ts, ts);
- EXPECT_CALL(mock_stats, PacketsDiscarded(1)).Times(1);
+ EXPECT_CALL(mock_stats, PacketsDiscarded(1));
+ EXPECT_CALL(check, Call(i));
EXPECT_EQ(PacketBuffer::kOK, buffer.DiscardNextPacket(&mock_stats));
current_ts += ts_increment;
+ check.Call(i);
}
constexpr int kRemainingPackets = kTotalPackets - kDiscardPackets;
- // This will not discard any packets because the oldest packet is newer than
- // the indicated horizon_samples.
+ // This will discard all remaining packets but one. The oldest packet is older
+ // than the indicated horizon_samples, and will thus be left in the buffer.
+ constexpr size_t kSkipPackets = 1;
+ EXPECT_CALL(mock_stats, PacketsDiscarded(kRemainingPackets - kSkipPackets));
+ EXPECT_CALL(check, Call(17)); // Arbitrary id number.
buffer.DiscardOldPackets(start_ts + kTotalPackets * ts_increment,
kRemainingPackets * ts_increment, &mock_stats);
+ check.Call(17); // Same arbitrary id number.
+
+ EXPECT_EQ(kSkipPackets, buffer.NumPacketsInBuffer());
uint32_t ts;
EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&ts));
EXPECT_EQ(current_ts, ts);
// Discard all remaining packets.
- EXPECT_CALL(mock_stats, PacketsDiscarded(1)).Times(kRemainingPackets);
+ EXPECT_CALL(mock_stats, PacketsDiscarded(kSkipPackets));
buffer.DiscardAllOldPackets(start_ts + kTotalPackets * ts_increment,
&mock_stats);