From 3a9640a5aa939b137fde2f39baeed07245c85647 Mon Sep 17 00:00:00 2001 From: Ivo Creusen Date: Fri, 10 Sep 2021 11:45:47 +0000 Subject: [PATCH] Improve nacking logic Requesting nacks in more cases allows the delay adaptation to better predict if it's worth it to increase the jitter buffer delay to wait for the nacked packets to arrive. Bug: webrtc:10178 Change-Id: I46adb76c013dbb8df0b99eb3f7a398f8f21c2bf6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231648 Commit-Queue: Ivo Creusen Reviewed-by: Jakob Ivarsson Cr-Commit-Position: refs/heads/main@{#34970} --- modules/audio_coding/neteq/nack_tracker.cc | 48 +++++++++++++-- modules/audio_coding/neteq/nack_tracker.h | 28 ++++++++- .../neteq/nack_tracker_unittest.cc | 60 +++++++++++++++++++ modules/audio_coding/neteq/neteq_impl.cc | 2 +- 4 files changed, 132 insertions(+), 6 deletions(-) diff --git a/modules/audio_coding/neteq/nack_tracker.cc b/modules/audio_coding/neteq/nack_tracker.cc index 9f04534af1..c95679ac17 100644 --- a/modules/audio_coding/neteq/nack_tracker.cc +++ b/modules/audio_coding/neteq/nack_tracker.cc @@ -10,20 +10,39 @@ #include "modules/audio_coding/neteq/nack_tracker.h" - #include #include #include "rtc_base/checks.h" +#include "rtc_base/experiments/struct_parameters_parser.h" +#include "rtc_base/logging.h" +#include "system_wrappers/include/field_trial.h" namespace webrtc { namespace { const int kDefaultSampleRateKhz = 48; const int kDefaultPacketSizeMs = 20; +constexpr char kNackTrackerConfigFieldTrial[] = + "WebRTC-Audio-NetEqNackTrackerConfig"; } // namespace +NackTracker::Config::Config() { + auto parser = StructParametersParser::Create( + "packet_loss_forget_factor", &packet_loss_forget_factor, + "ms_per_loss_percent", &ms_per_loss_percent, "never_nack_multiple_times", + &never_nack_multiple_times); + parser->Parse( + webrtc::field_trial::FindFullName(kNackTrackerConfigFieldTrial)); + RTC_LOG(LS_INFO) << "Nack tracker config:" + " packet_loss_forget_factor=" + << packet_loss_forget_factor + << " ms_per_loss_percent=" << ms_per_loss_percent + << " never_nack_multiple_times=" + << never_nack_multiple_times; +} + NackTracker::NackTracker(int nack_threshold_packets) : nack_threshold_packets_(nack_threshold_packets), sequence_num_last_received_rtp_(0), @@ -74,6 +93,8 @@ void NackTracker::UpdateLastReceivedPacket(uint16_t sequence_number, if (IsNewerSequenceNumber(sequence_num_last_received_rtp_, sequence_number)) return; + UpdatePacketLossRate(sequence_number - sequence_num_last_received_rtp_ - 1); + UpdateSamplesPerPacket(sequence_number, timestamp); UpdateList(sequence_number); @@ -215,17 +236,36 @@ int64_t NackTracker::TimeToPlay(uint32_t timestamp) const { } // We don't erase elements with time-to-play shorter than round-trip-time. -std::vector NackTracker::GetNackList( - int64_t round_trip_time_ms) const { +std::vector NackTracker::GetNackList(int64_t round_trip_time_ms) { RTC_DCHECK_GE(round_trip_time_ms, 0); std::vector sequence_numbers; + // The estimated packet loss is between 0 and 1, so we need to multiply by 100 + // here. + int max_wait_ms = + 100.0 * config_.ms_per_loss_percent * packet_loss_rate_ / (1 << 30); for (NackList::const_iterator it = nack_list_.begin(); it != nack_list_.end(); ++it) { + int64_t time_since_packet_ms = + (timestamp_last_received_rtp_ - it->second.estimated_timestamp) / + sample_rate_khz_; if (it->second.is_missing && - it->second.time_to_play_ms > round_trip_time_ms) + (it->second.time_to_play_ms > round_trip_time_ms || + time_since_packet_ms + round_trip_time_ms < max_wait_ms)) sequence_numbers.push_back(it->first); } + if (config_.never_nack_multiple_times) { + nack_list_.clear(); + } return sequence_numbers; } +void NackTracker::UpdatePacketLossRate(int packets_lost) { + const uint64_t alpha_q30 = (1 << 30) * config_.packet_loss_forget_factor; + // Exponential filter. + packet_loss_rate_ = (alpha_q30 * packet_loss_rate_) >> 30; + for (int i = 0; i < packets_lost; ++i) { + packet_loss_rate_ = + ((alpha_q30 * packet_loss_rate_) >> 30) + ((1 << 30) - alpha_q30); + } +} } // namespace webrtc diff --git a/modules/audio_coding/neteq/nack_tracker.h b/modules/audio_coding/neteq/nack_tracker.h index ac0a77f98f..f3cce74560 100644 --- a/modules/audio_coding/neteq/nack_tracker.h +++ b/modules/audio_coding/neteq/nack_tracker.h @@ -87,16 +87,34 @@ class NackTracker { // Get a list of "missing" packets which have expected time-to-play larger // than the given round-trip-time (in milliseconds). // Note: Late packets are not included. - std::vector GetNackList(int64_t round_trip_time_ms) const; + // Calling this method multiple times may give different results, since the + // internal nack list may get flushed if never_nack_multiple_times_ is true. + std::vector GetNackList(int64_t round_trip_time_ms); // Reset to default values. The NACK list is cleared. // `nack_threshold_packets_` & `max_nack_list_size_` preserve their values. void Reset(); + // Returns the estimated packet loss rate in Q30, for testing only. + uint32_t GetPacketLossRateForTest() { return packet_loss_rate_; } + private: // This test need to access the private method GetNackList(). FRIEND_TEST_ALL_PREFIXES(NackTrackerTest, EstimateTimestampAndTimeToPlay); + // Options that can be configured via field trial. + struct Config { + Config(); + + // The exponential decay factor used to estimate the packet loss rate. + double packet_loss_forget_factor = 0.996; + // How many additional ms we are willing to wait (at most) for nacked + // packets for each additional percentage of packet loss. + int ms_per_loss_percent = 20; + // If true, never nack packets more than once. + bool never_nack_multiple_times = false; + }; + struct NackElement { NackElement(int64_t initial_time_to_play_ms, uint32_t initial_timestamp, @@ -173,6 +191,11 @@ class NackTracker { // Compute time-to-play given a timestamp. int64_t TimeToPlay(uint32_t timestamp) const; + // Updates the estimated packet lost rate. + void UpdatePacketLossRate(int packets_lost); + + const Config config_; + // If packet N is arrived, any packet prior to N - `nack_threshold_packets_` // which is not arrived is considered missing, and should be in NACK list. // Also any packet in the range of N-1 and N - `nack_threshold_packets_`, @@ -204,6 +227,9 @@ class NackTracker { // NACK list will not keep track of missing packets prior to // `sequence_num_last_received_rtp_` - `max_nack_list_size_`. size_t max_nack_list_size_; + + // Current estimate of the packet loss rate in Q30. + uint32_t packet_loss_rate_ = 0; }; } // namespace webrtc diff --git a/modules/audio_coding/neteq/nack_tracker_unittest.cc b/modules/audio_coding/neteq/nack_tracker_unittest.cc index 3f5a05b1fc..6ee8f2af0a 100644 --- a/modules/audio_coding/neteq/nack_tracker_unittest.cc +++ b/modules/audio_coding/neteq/nack_tracker_unittest.cc @@ -16,6 +16,7 @@ #include #include "modules/audio_coding/include/audio_coding_module_typedefs.h" +#include "test/field_trial.h" #include "test/gtest.h" namespace webrtc { @@ -479,4 +480,63 @@ TEST(NackTrackerTest, RoudTripTimeIsApplied) { EXPECT_EQ(5, nack_list[1]); } +// Set never_nack_multiple_times to true with a field trial and verify that +// packets are not nacked multiple times. +TEST(NackTrackerTest, DoNotNackMultipleTimes) { + test::ScopedFieldTrials field_trials( + "WebRTC-Audio-NetEqNackTrackerConfig/" + "packet_loss_forget_factor:0.996,ms_per_loss_percent:20," + "never_nack_multiple_times:true/"); + const int kNackListSize = 200; + std::unique_ptr nack(NackTracker::Create(0)); + nack->UpdateSampleRate(kSampleRateHz); + nack->SetMaxNackListSize(kNackListSize); + + uint16_t seq_num = 0; + uint32_t timestamp = 0x87654321; + nack->UpdateLastReceivedPacket(seq_num, timestamp); + + uint16_t kNumLostPackets = 3; + + seq_num += (1 + kNumLostPackets); + timestamp += (1 + kNumLostPackets) * kTimestampIncrement; + nack->UpdateLastReceivedPacket(seq_num, timestamp); + + std::vector nack_list = nack->GetNackList(10); + ASSERT_EQ(3u, nack_list.size()); + EXPECT_EQ(1, nack_list[0]); + EXPECT_EQ(2, nack_list[1]); + EXPECT_EQ(3, nack_list[2]); + // When we get the nack list again, it should be empty. + std::vector nack_list2 = nack->GetNackList(10); + EXPECT_TRUE(nack_list2.empty()); +} + +// Test if estimated packet loss rate is correct. +TEST(NackTrackerTest, PacketLossRateCorrect) { + const int kNackListSize = 200; + std::unique_ptr nack(NackTracker::Create(0)); + nack->UpdateSampleRate(kSampleRateHz); + nack->SetMaxNackListSize(kNackListSize); + uint16_t seq_num = 0; + uint32_t timestamp = 0x87654321; + auto add_packet = [&nack, &seq_num, ×tamp] { + nack->UpdateLastReceivedPacket(seq_num, timestamp); + seq_num++; + timestamp += kTimestampIncrement; + }; + // Add some packets, but every fourth packet is lost. + for (int i = 0; i < 300; i++) { + add_packet(); + add_packet(); + add_packet(); + // The next packet is lost. + seq_num++; + timestamp += kTimestampIncrement; + } + // 1 << 28 is 0.25 in Q30. We expect the packet loss estimate to be within + // 0.01 of that. + EXPECT_NEAR(nack->GetPacketLossRateForTest(), 1 << 28, (1 << 30) / 100); +} + } // namespace webrtc diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc index c7b98a5450..ae79960305 100644 --- a/modules/audio_coding/neteq/neteq_impl.cc +++ b/modules/audio_coding/neteq/neteq_impl.cc @@ -514,7 +514,7 @@ void NetEqImpl::FlushBuffers() { void NetEqImpl::EnableNack(size_t max_nack_list_size) { MutexLock lock(&mutex_); if (!nack_enabled_) { - const int kNackThresholdPackets = 2; + const int kNackThresholdPackets = 0; nack_.reset(NackTracker::Create(kNackThresholdPackets)); nack_enabled_ = true; nack_->UpdateSampleRate(fs_hz_);