From 12d30840d82e4f3c570a1a285f5bbd531860ea6e Mon Sep 17 00:00:00 2001 From: minyue-webrtc Date: Wed, 19 Jul 2017 11:44:06 +0200 Subject: [PATCH] Correct the calculation of discard rate. Bug: webrtc:7903 Change-Id: Ib5d6fd882a994dd542b616e5fe1c75710346dd31 Reviewed-on: https://chromium-review.googlesource.com/575057 Commit-Queue: Minyue Li Reviewed-by: Oskar Sundbom Cr-Commit-Position: refs/heads/master@{#19101} --- .../neteq/mock/mock_packet_buffer.h | 19 ++--- .../modules/audio_coding/neteq/neteq_impl.cc | 2 +- .../audio_coding/neteq/neteq_impl_unittest.cc | 2 +- .../neteq/neteq_network_stats_unittest.cc | 1 + .../audio_coding/neteq/neteq_unittest.cc | 8 +- .../audio_coding/neteq/packet_buffer.cc | 18 ++++- .../audio_coding/neteq/packet_buffer.h | 5 +- .../neteq/packet_buffer_unittest.cc | 79 ++++++++++++------- 8 files changed, 85 insertions(+), 49 deletions(-) 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 7967ab98aa..1530329a45 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 @@ class MockPacketBuffer : public PacketBuffer { 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* current_rtp_payload_type, - rtc::Optional* 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* current_rtp_payload_type, + rtc::Optional* 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 88e07e857c..4b95253d71 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc @@ -728,7 +728,7 @@ int NetEqImpl::InsertPacketInternal(const RTPHeader& rtp_header, 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 9d9be30b9a..fafc2df03e 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 @@ TEST_F(NetEqImplTest, InsertPacket) { .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(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 c244a97355..4feb448ff9 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 @@ NetEqNetworkStatsTest(NetEqDecoder codec, 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 612c2c6b18..fd163c4983 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc @@ -482,10 +482,10 @@ TEST_F(NetEqDecodingTest, MAYBE_TestOpusBitExactness) { "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 efb350542a..1e71525dc0 100644 --- a/webrtc/modules/audio_coding/neteq/packet_buffer.cc +++ b/webrtc/modules/audio_coding/neteq/packet_buffer.cc @@ -68,7 +68,7 @@ bool PacketBuffer::Empty() const { 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 @@ int PacketBuffer::InsertPacket(Packet&& packet) { // 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 @@ int PacketBuffer::InsertPacket(Packet&& packet) { 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 @@ int PacketBuffer::InsertPacketList( PacketList* packet_list, const DecoderDatabase& decoder_database, rtc::Optional* current_rtp_payload_type, - rtc::Optional* current_cng_rtp_payload_type) { + rtc::Optional* 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 @@ int PacketBuffer::InsertPacketList( } *current_rtp_payload_type = rtc::Optional(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 @@ int PacketBuffer::DiscardNextPacket(StatisticsCalculator* stats) { // 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 @@ void PacketBuffer::DiscardOldPackets(uint32_t timestamp_limit, 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 @@ void PacketBuffer::DiscardPacketsWithPayloadType(uint8_t payload_type, ++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 806ca70c64..afd5f04454 100644 --- a/webrtc/modules/audio_coding/neteq/packet_buffer.h +++ b/webrtc/modules/audio_coding/neteq/packet_buffer.h @@ -52,7 +52,7 @@ class PacketBuffer { // 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 @@ class PacketBuffer { PacketList* packet_list, const DecoderDatabase& decoder_database, rtc::Optional* current_rtp_payload_type, - rtc::Optional* current_cng_rtp_payload_type); + rtc::Optional* 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 103c3aa2a1..2622c0cc0a 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 @@ TEST(PacketBuffer, InsertPacket) { TickTimer tick_timer; PacketBuffer buffer(10, &tick_timer); // 10 packets. PacketGenerator gen(17u, 4711u, 0, 10); + StrictMock 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 @@ TEST(PacketBuffer, FlushBuffer) { PacketBuffer buffer(10, &tick_timer); // 10 packets. PacketGenerator gen(0, 0, 0, 10); const int payload_len = 10; + StrictMock 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 @@ TEST(PacketBuffer, OverfillBuffer) { TickTimer tick_timer; PacketBuffer buffer(10, &tick_timer); // 10 packets. PacketGenerator gen(0, 0, 0, 10); + StrictMock 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 @@ TEST(PacketBuffer, OverfillBuffer) { 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 @@ TEST(PacketBuffer, InsertPacketList) { const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderPCMu, factory); EXPECT_CALL(decoder_database, GetDecoderInfo(0)) .WillRepeatedly(Return(&info)); + + StrictMock mock_stats; + rtc::Optional current_pt; rtc::Optional 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(0), @@ -220,12 +226,14 @@ TEST(PacketBuffer, InsertPacketListChangePayloadType) { const DecoderDatabase::DecoderInfo info1(NetEqDecoder::kDecoderPCMa, factory); EXPECT_CALL(decoder_database, GetDecoderInfo(1)) .WillRepeatedly(Return(&info1)); + + StrictMock mock_stats; + rtc::Optional current_pt; rtc::Optional 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(1), @@ -271,6 +279,13 @@ TEST(PacketBuffer, ExtractOrderRedundancy) { PacketGenerator gen(0, 0, 0, kFrameSize); + StrictMock 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 check; for (int i = 0; i < kPackets; ++i) { gen.Reset(packet_facts[i].sequence_number, packet_facts[i].timestamp, @@ -278,10 +293,16 @@ TEST(PacketBuffer, ExtractOrderRedundancy) { 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 @@ TEST(PacketBuffer, DiscardPackets) { PacketGenerator gen(start_seq_no, start_ts, 0, ts_increment); PacketList list; const int payload_len = 10; + StrictMock 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 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 @@ TEST(PacketBuffer, Reordering) { rtc::Optional current_pt; rtc::Optional current_cng_pt; - EXPECT_EQ(PacketBuffer::kOK, buffer.InsertPacketList(&list, - decoder_database, - ¤t_pt, - ¤t_cng_pt)); + StrictMock 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 @@ TEST(PacketBuffer, CngFirstThenSpeechWithNewSampleRate) { list.push_back(gen.NextPacket(kPayloadLen)); rtc::Optional current_pt; rtc::Optional current_cng_pt; + + StrictMock 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 @@ TEST(PacketBuffer, CngFirstThenSpeechWithNewSampleRate) { // 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 @@ TEST(PacketBuffer, Failures) { int payload_len = 100; PacketGenerator gen(start_seq_no, start_ts, 0, ts_increment); TickTimer tick_timer; + StrictMock 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 @@ TEST(PacketBuffer, Failures) { EXPECT_EQ(NULL, buffer->PeekNextPacket()); EXPECT_FALSE(buffer->GetNextPacket()); - StrictMock 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 @@ TEST(PacketBuffer, Failures) { // 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 @@ TEST(PacketBuffer, Failures) { rtc::Optional current_pt; rtc::Optional 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;