diff --git a/webrtc/modules/audio_coding/BUILD.gn b/webrtc/modules/audio_coding/BUILD.gn index 1aa4031a4c..ce775dc077 100644 --- a/webrtc/modules/audio_coding/BUILD.gn +++ b/webrtc/modules/audio_coding/BUILD.gn @@ -2114,6 +2114,7 @@ if (rtc_include_tests) { "neteq/mock/mock_external_decoder_pcm16b.h", "neteq/mock/mock_packet_buffer.h", "neteq/mock/mock_red_payload_splitter.h", + "neteq/mock/mock_statistics_calculator.h", "neteq/nack_tracker_unittest.cc", "neteq/neteq_external_decoder_unittest.cc", "neteq/neteq_impl_unittest.cc", 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 5a2f7e11e1..7967ab98aa 100644 --- a/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h +++ b/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h @@ -48,12 +48,13 @@ class MockPacketBuffer : public PacketBuffer { const Packet*()); MOCK_METHOD0(GetNextPacket, rtc::Optional()); - MOCK_METHOD0(DiscardNextPacket, - int()); - MOCK_METHOD2(DiscardOldPackets, - int(uint32_t timestamp_limit, uint32_t horizon_samples)); - MOCK_METHOD1(DiscardAllOldPackets, - int(uint32_t timestamp_limit)); + MOCK_METHOD1(DiscardNextPacket, int(StatisticsCalculator* stats)); + MOCK_METHOD3(DiscardOldPackets, + void(uint32_t timestamp_limit, + uint32_t horizon_samples, + StatisticsCalculator* stats)); + MOCK_METHOD2(DiscardAllOldPackets, + void(uint32_t timestamp_limit, StatisticsCalculator* stats)); MOCK_CONST_METHOD0(NumPacketsInBuffer, size_t()); MOCK_METHOD1(IncrementWaitingTimes, diff --git a/webrtc/modules/audio_coding/neteq/mock/mock_statistics_calculator.h b/webrtc/modules/audio_coding/neteq/mock/mock_statistics_calculator.h new file mode 100644 index 0000000000..da4e3ed465 --- /dev/null +++ b/webrtc/modules/audio_coding/neteq/mock/mock_statistics_calculator.h @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef WEBRTC_MODULES_AUDIO_CODING_NETEQ_MOCK_MOCK_STATISTICS_CALCULATOR_H_ +#define WEBRTC_MODULES_AUDIO_CODING_NETEQ_MOCK_MOCK_STATISTICS_CALCULATOR_H_ + +#include "webrtc/modules/audio_coding/neteq/statistics_calculator.h" + +#include "webrtc/test/gmock.h" + +namespace webrtc { + +class MockStatisticsCalculator : public StatisticsCalculator { + public: + // For current unittest, we mock only one method. + MOCK_METHOD1(PacketsDiscarded, void(size_t num_packets)); +}; + +} // namespace webrtc +#endif // WEBRTC_MODULES_AUDIO_CODING_NETEQ_MOCK_MOCK_STATISTICS_CALCULATOR_H_ diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc index fb3cc11ec8..bf4e4eb4e8 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc @@ -217,7 +217,7 @@ void NetEqImpl::SetCodecs(const std::map& codecs) { const std::vector changed_payload_types = decoder_database_->SetCodecs(codecs); for (const int pt : changed_payload_types) { - packet_buffer_->DiscardPacketsWithPayloadType(pt); + packet_buffer_->DiscardPacketsWithPayloadType(pt, &stats_); } } @@ -268,7 +268,7 @@ int NetEqImpl::RemovePayloadType(uint8_t rtp_payload_type) { rtc::CritScope lock(&crit_sect_); int ret = decoder_database_->Remove(rtp_payload_type); if (ret == DecoderDatabase::kOK || ret == DecoderDatabase::kDecoderNotFound) { - packet_buffer_->DiscardPacketsWithPayloadType(rtp_payload_type); + packet_buffer_->DiscardPacketsWithPayloadType(rtp_payload_type, &stats_); return kOK; } return kFail; @@ -1063,7 +1063,8 @@ int NetEqImpl::GetDecision(Operations* operation, uint32_t end_timestamp = sync_buffer_->end_timestamp(); if (!new_codec_) { const uint32_t five_seconds_samples = 5 * fs_hz_; - packet_buffer_->DiscardOldPackets(end_timestamp, five_seconds_samples); + packet_buffer_->DiscardOldPackets(end_timestamp, five_seconds_samples, + &stats_); } const Packet* packet = packet_buffer_->PeekNextPacket(); @@ -1084,12 +1085,12 @@ int NetEqImpl::GetDecision(Operations* operation, (end_timestamp >= packet->timestamp || end_timestamp + generated_noise_samples > packet->timestamp)) { // Don't use this packet, discard it. - if (packet_buffer_->DiscardNextPacket() != PacketBuffer::kOK) { + if (packet_buffer_->DiscardNextPacket(&stats_) != PacketBuffer::kOK) { assert(false); // Must be ok by design. } // Check buffer again. if (!new_codec_) { - packet_buffer_->DiscardOldPackets(end_timestamp, 5 * fs_hz_); + packet_buffer_->DiscardOldPackets(end_timestamp, 5 * fs_hz_, &stats_); } packet = packet_buffer_->PeekNextPacket(); } @@ -2003,7 +2004,7 @@ int NetEqImpl::ExtractPackets(size_t required_samples, // we could end up in the situation where we never decode anything, since // all incoming packets are considered too old but the buffer will also // never be flooded and flushed. - packet_buffer_->DiscardAllOldPackets(timestamp_); + packet_buffer_->DiscardAllOldPackets(timestamp_, &stats_); } return rtc::dchecked_cast(extracted_samples); diff --git a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc index eed77405f8..5049a6b5a4 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("0869a450a819b14bf2a91eb6f3629a3421d17606", - "0869a450a819b14bf2a91eb6f3629a3421d17606", - "0869a450a819b14bf2a91eb6f3629a3421d17606", - "0869a450a819b14bf2a91eb6f3629a3421d17606"); + PlatformChecksum("7a29daca4dfa4fb6eaec06d7e63c1729da7708f6", + "7a29daca4dfa4fb6eaec06d7e63c1729da7708f6", + "7a29daca4dfa4fb6eaec06d7e63c1729da7708f6", + "7a29daca4dfa4fb6eaec06d7e63c1729da7708f6"); 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 78a71c746d..cea6b3dbfb 100644 --- a/webrtc/modules/audio_coding/neteq/packet_buffer.cc +++ b/webrtc/modules/audio_coding/neteq/packet_buffer.cc @@ -19,6 +19,7 @@ #include "webrtc/api/audio_codecs/audio_decoder.h" #include "webrtc/base/logging.h" #include "webrtc/modules/audio_coding/neteq/decoder_database.h" +#include "webrtc/modules/audio_coding/neteq/statistics_calculator.h" #include "webrtc/modules/audio_coding/neteq/tick_timer.h" namespace webrtc { @@ -206,41 +207,51 @@ rtc::Optional PacketBuffer::GetNextPacket() { return packet; } -int PacketBuffer::DiscardNextPacket() { +int PacketBuffer::DiscardNextPacket(StatisticsCalculator* stats) { if (Empty()) { return kBufferEmpty; } // Assert that the packet sanity checks in InsertPacket method works. RTC_DCHECK(!buffer_.front().empty()); buffer_.pop_front(); + stats->PacketsDiscarded(1); return kOK; } -int PacketBuffer::DiscardOldPackets(uint32_t timestamp_limit, - uint32_t horizon_samples) { +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() != kOK) { + if (DiscardNextPacket(stats) != kOK) { assert(false); // Must be ok by design. } } - return 0; } -int PacketBuffer::DiscardAllOldPackets(uint32_t timestamp_limit) { - return DiscardOldPackets(timestamp_limit, 0); +void PacketBuffer::DiscardAllOldPackets(uint32_t timestamp_limit, + StatisticsCalculator* stats) { + DiscardOldPackets(timestamp_limit, 0, stats); } -void PacketBuffer::DiscardPacketsWithPayloadType(uint8_t payload_type) { +void PacketBuffer::DiscardPacketsWithPayloadType(uint8_t payload_type, + StatisticsCalculator* stats) { + int packets_discarded = 0; for (auto it = buffer_.begin(); it != buffer_.end(); /* */) { const Packet& packet = *it; if (packet.payload_type == payload_type) { it = buffer_.erase(it); + ++packets_discarded; } else { ++it; } } + if (packets_discarded > 0) + 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 a26d6c5c67..f93732242f 100644 --- a/webrtc/modules/audio_coding/neteq/packet_buffer.h +++ b/webrtc/modules/audio_coding/neteq/packet_buffer.h @@ -20,6 +20,7 @@ namespace webrtc { class DecoderDatabase; +class StatisticsCalculator; class TickTimer; // This is the actual buffer holding the packets before decoding. @@ -92,22 +93,24 @@ class PacketBuffer { // Discards the first packet in the buffer. The packet is deleted. // Returns PacketBuffer::kBufferEmpty if the buffer is empty, // PacketBuffer::kOK otherwise. - virtual int DiscardNextPacket(); + virtual int DiscardNextPacket(StatisticsCalculator* stats); // Discards all packets that are (strictly) older than timestamp_limit, // but newer than timestamp_limit - horizon_samples. Setting horizon_samples // to zero implies that the horizon is set to half the timestamp range. That // is, if a packet is more than 2^31 timestamps into the future compared with // timestamp_limit (including wrap-around), it is considered old. - // Returns number of packets discarded. - virtual int DiscardOldPackets(uint32_t timestamp_limit, - uint32_t horizon_samples); + virtual void DiscardOldPackets(uint32_t timestamp_limit, + uint32_t horizon_samples, + StatisticsCalculator* stats); // Discards all packets that are (strictly) older than timestamp_limit. - virtual int DiscardAllOldPackets(uint32_t timestamp_limit); + virtual void DiscardAllOldPackets(uint32_t timestamp_limit, + StatisticsCalculator* stats); // Removes all packets with a specific payload type from the buffer. - virtual void DiscardPacketsWithPayloadType(uint8_t payload_type); + virtual void DiscardPacketsWithPayloadType(uint8_t payload_type, + StatisticsCalculator* stats); // Returns the number of packets in the buffer, including duplicates and // redundant packets. diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc b/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc index e8795a9a15..82f7246cc7 100644 --- a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc @@ -10,15 +10,17 @@ // Unit tests for PacketBuffer class. +#include "webrtc/modules/audio_coding/neteq/packet_buffer.h" #include "webrtc/api/audio_codecs/builtin_audio_decoder_factory.h" #include "webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h" +#include "webrtc/modules/audio_coding/neteq/mock/mock_statistics_calculator.h" #include "webrtc/modules/audio_coding/neteq/packet.h" -#include "webrtc/modules/audio_coding/neteq/packet_buffer.h" #include "webrtc/modules/audio_coding/neteq/tick_timer.h" #include "webrtc/test/gmock.h" #include "webrtc/test/gtest.h" using ::testing::Return; +using ::testing::StrictMock; using ::testing::_; namespace webrtc { @@ -299,22 +301,42 @@ TEST(PacketBuffer, DiscardPackets) { PacketList list; const int payload_len = 10; + constexpr int kTotalPackets = 10; // Insert 10 small packets. - for (int i = 0; i < 10; ++i) { + for (int i = 0; i < kTotalPackets; ++i) { buffer.InsertPacket(gen.NextPacket(payload_len)); } 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 // front of the buffer. - uint32_t current_ts = start_ts; - for (int i = 0; i < 10; ++i) { + constexpr int kDiscardPackets = 5; + for (int i = 0; i < kDiscardPackets; ++i) { uint32_t ts; EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&ts)); EXPECT_EQ(current_ts, ts); - EXPECT_EQ(PacketBuffer::kOK, buffer.DiscardNextPacket()); + EXPECT_CALL(mock_stats, PacketsDiscarded(1)).Times(1); + EXPECT_EQ(PacketBuffer::kOK, buffer.DiscardNextPacket(&mock_stats)); current_ts += ts_increment; } + + constexpr int kRemainingPackets = kTotalPackets - kDiscardPackets; + // This will not discard any packets because the oldest packet is newer than + // the indicated horizon_samples. + buffer.DiscardOldPackets(start_ts + kTotalPackets * ts_increment, + kRemainingPackets * ts_increment, &mock_stats); + 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); + buffer.DiscardAllOldPackets(start_ts + kTotalPackets * ts_increment, + &mock_stats); + EXPECT_TRUE(buffer.Empty()); } @@ -452,8 +474,12 @@ TEST(PacketBuffer, Failures) { buffer->NextHigherTimestamp(0, &temp_ts)); EXPECT_EQ(NULL, buffer->PeekNextPacket()); EXPECT_FALSE(buffer->GetNextPacket()); - EXPECT_EQ(PacketBuffer::kBufferEmpty, buffer->DiscardNextPacket()); - EXPECT_EQ(0, buffer->DiscardAllOldPackets(0)); // 0 packets discarded. + + 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)); + buffer->DiscardAllOldPackets(0, &mock_stats); // Insert one packet to make the buffer non-empty. EXPECT_EQ(PacketBuffer::kOK, diff --git a/webrtc/modules/audio_coding/neteq/statistics_calculator.h b/webrtc/modules/audio_coding/neteq/statistics_calculator.h index fb869ab025..320a74a065 100644 --- a/webrtc/modules/audio_coding/neteq/statistics_calculator.h +++ b/webrtc/modules/audio_coding/neteq/statistics_calculator.h @@ -64,7 +64,7 @@ class StatisticsCalculator { void AddZeros(size_t num_samples); // Reports that |num_packets| packets were discarded. - void PacketsDiscarded(size_t num_packets); + virtual void PacketsDiscarded(size_t num_packets); // Reports that |num_samples| were lost. void LostSamples(size_t num_samples);