Correct the calculation of discard rate.
Bug: webrtc:7903
Change-Id: Ib5d6fd882a994dd542b616e5fe1c75710346dd31
Reviewed-on: https://chromium-review.googlesource.com/575057
Commit-Queue: Minyue Li <minyue@webrtc.org>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#19101}diff --git a/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h b/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h
index 7967ab9..1530329 100644
--- a/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h
+++ b/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h
@@ -27,19 +27,20 @@
void());
MOCK_CONST_METHOD0(Empty,
bool());
- int InsertPacket(Packet&& packet) {
- return InsertPacketWrapped(&packet);
+ int InsertPacket(Packet&& packet, StatisticsCalculator* stats) {
+ return InsertPacketWrapped(&packet, stats);
}
// Since gtest does not properly support move-only types, InsertPacket is
// implemented as a wrapper. You'll have to implement InsertPacketWrapped
// instead and move from |*packet|.
- MOCK_METHOD1(InsertPacketWrapped,
- int(Packet* packet));
- MOCK_METHOD4(InsertPacketList,
- int(PacketList* packet_list,
- const DecoderDatabase& decoder_database,
- rtc::Optional<uint8_t>* current_rtp_payload_type,
- rtc::Optional<uint8_t>* current_cng_rtp_payload_type));
+ MOCK_METHOD2(InsertPacketWrapped,
+ int(Packet* packet, StatisticsCalculator* stats));
+ MOCK_METHOD5(InsertPacketList,
+ int(PacketList* packet_list,
+ const DecoderDatabase& decoder_database,
+ rtc::Optional<uint8_t>* current_rtp_payload_type,
+ rtc::Optional<uint8_t>* current_cng_rtp_payload_type,
+ StatisticsCalculator* stats));
MOCK_CONST_METHOD1(NextTimestamp,
int(uint32_t* next_timestamp));
MOCK_CONST_METHOD2(NextHigherTimestamp,
diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc
index 88e07e8..4b95253 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc
@@ -728,7 +728,7 @@
packet_buffer_->NumPacketsInBuffer();
const int ret = packet_buffer_->InsertPacketList(
&parsed_packet_list, *decoder_database_, ¤t_rtp_payload_type_,
- ¤t_cng_rtp_payload_type_);
+ ¤t_cng_rtp_payload_type_, &stats_);
if (ret == PacketBuffer::kFlushed) {
// Reset DSP timestamp etc. if packet buffer flushed.
new_codec_ = true;
diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
index 9d9be30..fafc2df 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
@@ -350,7 +350,7 @@
.WillOnce(Return(false)); // Called once after first packet is inserted.
EXPECT_CALL(*mock_packet_buffer_, Flush())
.Times(1);
- EXPECT_CALL(*mock_packet_buffer_, InsertPacketList(_, _, _, _))
+ EXPECT_CALL(*mock_packet_buffer_, InsertPacketList(_, _, _, _, _))
.Times(2)
.WillRepeatedly(
DoAll(SetArgPointee<2>(rtc::Optional<uint8_t>(kPayloadType)),
diff --git a/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc
index c244a97..4feb448 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc
@@ -273,6 +273,7 @@
expects.stats_ref.packet_loss_rate = 0;
expects.stats_ref.expand_rate = expects.stats_ref.speech_expand_rate = 0;
expects.stats_ref.secondary_decoded_rate = 2006;
+ expects.stats_ref.packet_discard_rate = 13374;
RunTest(50, expects);
}
diff --git a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
index 612c2c6..fd163c4 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
@@ -482,10 +482,10 @@
"6237dd113ad80d7764fe4c90b55b2ec035eae64e");
const std::string network_stats_checksum =
- PlatformChecksum("7a29daca4dfa4fb6eaec06d7e63c1729da7708f6",
- "7a29daca4dfa4fb6eaec06d7e63c1729da7708f6",
- "7a29daca4dfa4fb6eaec06d7e63c1729da7708f6",
- "7a29daca4dfa4fb6eaec06d7e63c1729da7708f6");
+ PlatformChecksum("dda4cee006d9369c7114a03790c5761346cf5e23",
+ "dda4cee006d9369c7114a03790c5761346cf5e23",
+ "dda4cee006d9369c7114a03790c5761346cf5e23",
+ "dda4cee006d9369c7114a03790c5761346cf5e23");
const std::string rtcp_stats_checksum = PlatformChecksum(
"e37c797e3de6a64dda88c9ade7a013d022a2e1e0",
diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer.cc b/webrtc/modules/audio_coding/neteq/packet_buffer.cc
index efb3505..1e71525 100644
--- a/webrtc/modules/audio_coding/neteq/packet_buffer.cc
+++ b/webrtc/modules/audio_coding/neteq/packet_buffer.cc
@@ -68,7 +68,7 @@
return buffer_.empty();
}
-int PacketBuffer::InsertPacket(Packet&& packet) {
+int PacketBuffer::InsertPacket(Packet&& packet, StatisticsCalculator* stats) {
if (packet.empty()) {
LOG(LS_WARNING) << "InsertPacket invalid packet";
return kInvalidPacket;
@@ -99,6 +99,8 @@
// timestamp as |rit|, which has a higher priority, do not insert the new
// packet to list.
if (rit != buffer_.rend() && packet.timestamp == rit->timestamp) {
+ RTC_CHECK(stats);
+ stats->PacketsDiscarded(1);
return return_val;
}
@@ -108,6 +110,8 @@
PacketList::iterator it = rit.base();
if (it != buffer_.end() && packet.timestamp == it->timestamp) {
it = buffer_.erase(it);
+ RTC_CHECK(stats);
+ stats->PacketsDiscarded(1);
}
buffer_.insert(it, std::move(packet)); // Insert the packet at that position.
@@ -118,7 +122,9 @@
PacketList* packet_list,
const DecoderDatabase& decoder_database,
rtc::Optional<uint8_t>* current_rtp_payload_type,
- rtc::Optional<uint8_t>* current_cng_rtp_payload_type) {
+ rtc::Optional<uint8_t>* current_cng_rtp_payload_type,
+ StatisticsCalculator* stats) {
+ RTC_DCHECK(stats);
bool flushed = false;
for (auto& packet : *packet_list) {
if (decoder_database.IsComfortNoise(packet.payload_type)) {
@@ -145,7 +151,7 @@
}
*current_rtp_payload_type = rtc::Optional<uint8_t>(packet.payload_type);
}
- int return_val = InsertPacket(std::move(packet));
+ int return_val = InsertPacket(std::move(packet), stats);
if (return_val == kFlushed) {
// The buffer flushed, but this is not an error. We can still continue.
flushed = true;
@@ -214,6 +220,7 @@
// Assert that the packet sanity checks in InsertPacket method works.
RTC_DCHECK(!buffer_.front().empty());
buffer_.pop_front();
+ RTC_CHECK(stats);
stats->PacketsDiscarded(1);
return kOK;
}
@@ -227,6 +234,7 @@
IsObsoleteTimestamp(p.timestamp, timestamp_limit, horizon_samples);
});
if (old_size > buffer_.size()) {
+ RTC_CHECK(stats);
stats->PacketsDiscarded(old_size - buffer_.size());
}
}
@@ -248,8 +256,10 @@
++it;
}
}
- if (packets_discarded > 0)
+ if (packets_discarded > 0) {
+ RTC_CHECK(stats);
stats->PacketsDiscarded(packets_discarded);
+ }
}
size_t PacketBuffer::NumPacketsInBuffer() const {
diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer.h b/webrtc/modules/audio_coding/neteq/packet_buffer.h
index 806ca70..afd5f04 100644
--- a/webrtc/modules/audio_coding/neteq/packet_buffer.h
+++ b/webrtc/modules/audio_coding/neteq/packet_buffer.h
@@ -52,7 +52,7 @@
// the packet object.
// Returns PacketBuffer::kOK on success, PacketBuffer::kFlushed if the buffer
// was flushed due to overfilling.
- virtual int InsertPacket(Packet&& packet);
+ virtual int InsertPacket(Packet&& packet, StatisticsCalculator* stats);
// Inserts a list of packets into the buffer. The buffer will take over
// ownership of the packet objects.
@@ -66,7 +66,8 @@
PacketList* packet_list,
const DecoderDatabase& decoder_database,
rtc::Optional<uint8_t>* current_rtp_payload_type,
- rtc::Optional<uint8_t>* current_cng_rtp_payload_type);
+ rtc::Optional<uint8_t>* current_cng_rtp_payload_type,
+ StatisticsCalculator* stats);
// Gets the timestamp for the first packet in the buffer and writes it to the
// output variable |next_timestamp|.
diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc b/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
index 103c3aa..2622c0c 100644
--- a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
@@ -89,10 +89,11 @@
TickTimer tick_timer;
PacketBuffer buffer(10, &tick_timer); // 10 packets.
PacketGenerator gen(17u, 4711u, 0, 10);
+ StrictMock<MockStatisticsCalculator> mock_stats;
const int payload_len = 100;
const Packet packet = gen.NextPacket(payload_len);
- EXPECT_EQ(0, buffer.InsertPacket(packet.Clone()));
+ EXPECT_EQ(0, buffer.InsertPacket(packet.Clone(), &mock_stats));
uint32_t next_ts;
EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&next_ts));
EXPECT_EQ(4711u, next_ts);
@@ -111,11 +112,12 @@
PacketBuffer buffer(10, &tick_timer); // 10 packets.
PacketGenerator gen(0, 0, 0, 10);
const int payload_len = 10;
+ StrictMock<MockStatisticsCalculator> mock_stats;
// Insert 10 small packets; should be ok.
for (int i = 0; i < 10; ++i) {
EXPECT_EQ(PacketBuffer::kOK,
- buffer.InsertPacket(gen.NextPacket(payload_len)));
+ buffer.InsertPacket(gen.NextPacket(payload_len), &mock_stats));
}
EXPECT_EQ(10u, buffer.NumPacketsInBuffer());
EXPECT_FALSE(buffer.Empty());
@@ -131,13 +133,14 @@
TickTimer tick_timer;
PacketBuffer buffer(10, &tick_timer); // 10 packets.
PacketGenerator gen(0, 0, 0, 10);
+ StrictMock<MockStatisticsCalculator> mock_stats;
// Insert 10 small packets; should be ok.
const int payload_len = 10;
int i;
for (i = 0; i < 10; ++i) {
EXPECT_EQ(PacketBuffer::kOK,
- buffer.InsertPacket(gen.NextPacket(payload_len)));
+ buffer.InsertPacket(gen.NextPacket(payload_len), &mock_stats));
}
EXPECT_EQ(10u, buffer.NumPacketsInBuffer());
uint32_t next_ts;
@@ -146,7 +149,8 @@
const Packet packet = gen.NextPacket(payload_len);
// Insert 11th packet; should flush the buffer and insert it after flushing.
- EXPECT_EQ(PacketBuffer::kFlushed, buffer.InsertPacket(packet.Clone()));
+ EXPECT_EQ(PacketBuffer::kFlushed,
+ buffer.InsertPacket(packet.Clone(), &mock_stats));
EXPECT_EQ(1u, buffer.NumPacketsInBuffer());
EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&next_ts));
// Expect last inserted packet to be first in line.
@@ -174,12 +178,14 @@
const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderPCMu, factory);
EXPECT_CALL(decoder_database, GetDecoderInfo(0))
.WillRepeatedly(Return(&info));
+
+ StrictMock<MockStatisticsCalculator> mock_stats;
+
rtc::Optional<uint8_t> current_pt;
rtc::Optional<uint8_t> current_cng_pt;
- EXPECT_EQ(PacketBuffer::kOK, buffer.InsertPacketList(&list,
- decoder_database,
- ¤t_pt,
- ¤t_cng_pt));
+ EXPECT_EQ(PacketBuffer::kOK,
+ buffer.InsertPacketList(&list, decoder_database, ¤t_pt,
+ ¤t_cng_pt, &mock_stats));
EXPECT_TRUE(list.empty()); // The PacketBuffer should have depleted the list.
EXPECT_EQ(10u, buffer.NumPacketsInBuffer());
EXPECT_EQ(rtc::Optional<uint8_t>(0),
@@ -220,12 +226,14 @@
const DecoderDatabase::DecoderInfo info1(NetEqDecoder::kDecoderPCMa, factory);
EXPECT_CALL(decoder_database, GetDecoderInfo(1))
.WillRepeatedly(Return(&info1));
+
+ StrictMock<MockStatisticsCalculator> mock_stats;
+
rtc::Optional<uint8_t> current_pt;
rtc::Optional<uint8_t> current_cng_pt;
- EXPECT_EQ(PacketBuffer::kFlushed, buffer.InsertPacketList(&list,
- decoder_database,
- ¤t_pt,
- ¤t_cng_pt));
+ EXPECT_EQ(PacketBuffer::kFlushed,
+ buffer.InsertPacketList(&list, decoder_database, ¤t_pt,
+ ¤t_cng_pt, &mock_stats));
EXPECT_TRUE(list.empty()); // The PacketBuffer should have depleted the list.
EXPECT_EQ(1u, buffer.NumPacketsInBuffer()); // Only the last packet.
EXPECT_EQ(rtc::Optional<uint8_t>(1),
@@ -271,6 +279,13 @@
PacketGenerator gen(0, 0, 0, kFrameSize);
+ StrictMock<MockStatisticsCalculator> mock_stats;
+
+ // 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 < kPackets; ++i) {
gen.Reset(packet_facts[i].sequence_number,
packet_facts[i].timestamp,
@@ -278,10 +293,16 @@
kFrameSize);
Packet packet = gen.NextPacket(kPayloadLength);
packet.priority.red_level = packet_facts[i].primary ? 0 : 1;
- EXPECT_EQ(PacketBuffer::kOK, buffer.InsertPacket(packet.Clone()));
+ if (packet_facts[i].extract_order < 0) {
+ EXPECT_CALL(mock_stats, PacketsDiscarded(1));
+ }
+ EXPECT_CALL(check, Call(i));
+ EXPECT_EQ(PacketBuffer::kOK,
+ buffer.InsertPacket(packet.Clone(), &mock_stats));
if (packet_facts[i].extract_order >= 0) {
expect_order[packet_facts[i].extract_order] = std::move(packet);
}
+ check.Call(i);
}
EXPECT_EQ(kExpectPacketsInBuffer, buffer.NumPacketsInBuffer());
@@ -302,15 +323,15 @@
PacketGenerator gen(start_seq_no, start_ts, 0, ts_increment);
PacketList list;
const int payload_len = 10;
+ StrictMock<MockStatisticsCalculator> mock_stats;
constexpr int kTotalPackets = 10;
// Insert 10 small packets.
for (int i = 0; i < kTotalPackets; ++i) {
- buffer.InsertPacket(gen.NextPacket(payload_len));
+ buffer.InsertPacket(gen.NextPacket(payload_len), &mock_stats);
}
EXPECT_EQ(10u, buffer.NumPacketsInBuffer());
- StrictMock<MockStatisticsCalculator> mock_stats;
uint32_t current_ts = start_ts;
// Discard them one by one and make sure that the right packets are at the
@@ -386,10 +407,11 @@
rtc::Optional<uint8_t> current_pt;
rtc::Optional<uint8_t> current_cng_pt;
- EXPECT_EQ(PacketBuffer::kOK, buffer.InsertPacketList(&list,
- decoder_database,
- ¤t_pt,
- ¤t_cng_pt));
+ StrictMock<MockStatisticsCalculator> mock_stats;
+
+ EXPECT_EQ(PacketBuffer::kOK,
+ buffer.InsertPacketList(&list, decoder_database, ¤t_pt,
+ ¤t_cng_pt, &mock_stats));
EXPECT_EQ(10u, buffer.NumPacketsInBuffer());
// Extract them and make sure that come out in the right order.
@@ -433,9 +455,12 @@
list.push_back(gen.NextPacket(kPayloadLen));
rtc::Optional<uint8_t> current_pt;
rtc::Optional<uint8_t> current_cng_pt;
+
+ StrictMock<MockStatisticsCalculator> mock_stats;
+
EXPECT_EQ(PacketBuffer::kOK,
buffer.InsertPacketList(&list, decoder_database, ¤t_pt,
- ¤t_cng_pt));
+ ¤t_cng_pt, &mock_stats));
EXPECT_TRUE(list.empty());
EXPECT_EQ(1u, buffer.NumPacketsInBuffer());
ASSERT_TRUE(buffer.PeekNextPacket());
@@ -454,7 +479,7 @@
// new speech sample rate.
EXPECT_EQ(PacketBuffer::kFlushed,
buffer.InsertPacketList(&list, decoder_database, ¤t_pt,
- ¤t_cng_pt));
+ ¤t_cng_pt, &mock_stats));
EXPECT_TRUE(list.empty());
EXPECT_EQ(1u, buffer.NumPacketsInBuffer());
ASSERT_TRUE(buffer.PeekNextPacket());
@@ -475,13 +500,14 @@
int payload_len = 100;
PacketGenerator gen(start_seq_no, start_ts, 0, ts_increment);
TickTimer tick_timer;
+ StrictMock<MockStatisticsCalculator> mock_stats;
PacketBuffer* buffer = new PacketBuffer(100, &tick_timer); // 100 packets.
{
Packet packet = gen.NextPacket(payload_len);
packet.payload.Clear();
EXPECT_EQ(PacketBuffer::kInvalidPacket,
- buffer->InsertPacket(std::move(packet)));
+ buffer->InsertPacket(std::move(packet), &mock_stats));
}
// Buffer should still be empty. Test all empty-checks.
uint32_t temp_ts;
@@ -491,7 +517,6 @@
EXPECT_EQ(NULL, buffer->PeekNextPacket());
EXPECT_FALSE(buffer->GetNextPacket());
- StrictMock<MockStatisticsCalculator> mock_stats;
// Discarding packets will not invoke mock_stats.PacketDiscarded() because the
// packet buffer is empty.
EXPECT_EQ(PacketBuffer::kBufferEmpty, buffer->DiscardNextPacket(&mock_stats));
@@ -499,7 +524,7 @@
// Insert one packet to make the buffer non-empty.
EXPECT_EQ(PacketBuffer::kOK,
- buffer->InsertPacket(gen.NextPacket(payload_len)));
+ buffer->InsertPacket(gen.NextPacket(payload_len), &mock_stats));
EXPECT_EQ(PacketBuffer::kInvalidPointer, buffer->NextTimestamp(NULL));
EXPECT_EQ(PacketBuffer::kInvalidPointer,
buffer->NextHigherTimestamp(0, NULL));
@@ -525,10 +550,8 @@
rtc::Optional<uint8_t> current_pt;
rtc::Optional<uint8_t> current_cng_pt;
EXPECT_EQ(PacketBuffer::kInvalidPacket,
- buffer->InsertPacketList(&list,
- decoder_database,
- ¤t_pt,
- ¤t_cng_pt));
+ buffer->InsertPacketList(&list, decoder_database, ¤t_pt,
+ ¤t_cng_pt, &mock_stats));
EXPECT_TRUE(list.empty()); // The PacketBuffer should have depleted the list.
EXPECT_EQ(1u, buffer->NumPacketsInBuffer());
delete buffer;