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 <ivoc@webrtc.org>
Reviewed-by: Jakob Ivarsson <jakobi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34970}

modules/audio_coding/neteq/nack_tracker.cc

@@ -10,20 +10,39 @@
 
 #include "modules/audio_coding/neteq/nack_tracker.h"
 
-
 #include <cstdint>
 #include <utility>
 
 #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<uint16_t> NackTracker::GetNackList(
+std::vector<uint16_t> NackTracker::GetNackList(int64_t round_trip_time_ms) {
-    int64_t round_trip_time_ms) const {
   RTC_DCHECK_GE(round_trip_time_ms, 0);
   std::vector<uint16_t> 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

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<uint16_t> 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<uint16_t> 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

modules/audio_coding/neteq/nack_tracker_unittest.cc

@@ -16,6 +16,7 @@
 #include <memory>
 
 #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<NackTracker> 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<uint16_t> 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<uint16_t> 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<NackTracker> 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, &timestamp] {
+    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

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_);