Adds a field trial parameter to configure waiting time before sending Nack packets.

Currently we send Nack as soon as we see packets out of order(a skip in packet sequence number). Sometimes this is not necessary because these "missing" packets just late for a couple of millisecond, or these packets can be recovered by FEC. This CL add a field trial parameter to configure a delay before sending Nack.

Bug: webrtc:9953
Change-Id: Ia8f5995d874f7c55a74091bc90d8395b9b88e66b
Reviewed-on: https://webrtc-review.googlesource.com/c/109080
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Ying Wang <yinwa@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25488}

modules/video_coding/BUILD.gn

@@ -57,6 +57,7 @@ rtc_static_library("nack_module") {
     "../../rtc_base:rtc_base_approved",
     "../../rtc_base:rtc_numerics",
     "../../system_wrappers",
+    "../../system_wrappers:field_trial",
     "../utility:utility",
   ]
 }

modules/video_coding/nack_module.cc

@@ -16,6 +16,7 @@
 #include "modules/utility/include/process_thread.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
+#include "system_wrappers/include/field_trial.h"
 
 namespace webrtc {
 
@@ -28,14 +29,29 @@ const int kProcessFrequency = 50;
 const int kProcessIntervalMs = 1000 / kProcessFrequency;
 const int kMaxReorderedPackets = 128;
 const int kNumReorderingBuckets = 10;
+const int kDefaultSendNackDelayMs = 0;
+
+int64_t GetSendNackDelay() {
+  int64_t delay_ms = strtol(
+      webrtc::field_trial::FindFullName("WebRTC-SendNackDelayMs").c_str(),
+      nullptr, 10);
+  if (delay_ms > 0 && delay_ms <= 20) {
+    RTC_LOG(LS_INFO) << "SendNackDelay is set to " << delay_ms;
+    return delay_ms;
+  }
+  return kDefaultSendNackDelayMs;
+}
 }  // namespace
 
 NackModule::NackInfo::NackInfo()
     : seq_num(0), send_at_seq_num(0), sent_at_time(-1), retries(0) {}
 
-NackModule::NackInfo::NackInfo(uint16_t seq_num, uint16_t send_at_seq_num)
+NackModule::NackInfo::NackInfo(uint16_t seq_num,
+                               uint16_t send_at_seq_num,
+                               int64_t created_at_time)
     : seq_num(seq_num),
       send_at_seq_num(send_at_seq_num),
+      created_at_time(created_at_time),
       sent_at_time(-1),
       retries(0) {}
 
@@ -49,7 +65,8 @@ NackModule::NackModule(Clock* clock,
       initialized_(false),
       rtt_ms_(kDefaultRttMs),
       newest_seq_num_(0),
-      next_process_time_ms_(-1) {
+      next_process_time_ms_(-1),
+      send_nack_delay_ms_(GetSendNackDelay()) {
   RTC_DCHECK(clock_);
   RTC_DCHECK(nack_sender_);
   RTC_DCHECK(keyframe_request_sender_);
@@ -225,7 +242,8 @@ void NackModule::AddPacketsToNack(uint16_t seq_num_start,
     // Do not send nack for packets that are already recovered by FEC or RTX
     if (recovered_list_.find(seq_num) != recovered_list_.end())
       continue;
-    NackInfo nack_info(seq_num, seq_num + WaitNumberOfPackets(0.5));
+    NackInfo nack_info(seq_num, seq_num + WaitNumberOfPackets(0.5),
+                       clock_->TimeInMilliseconds());
     RTC_DCHECK(nack_list_.find(seq_num) == nack_list_.end());
     nack_list_[seq_num] = nack_info;
   }
@@ -238,22 +256,14 @@ std::vector<uint16_t> NackModule::GetNackBatch(NackFilterOptions options) {
   std::vector<uint16_t> nack_batch;
   auto it = nack_list_.begin();
   while (it != nack_list_.end()) {
-    if (consider_seq_num && it->second.sent_at_time == -1 &&
-        AheadOrAt(newest_seq_num_, it->second.send_at_seq_num)) {
-      nack_batch.emplace_back(it->second.seq_num);
-      ++it->second.retries;
-      it->second.sent_at_time = now_ms;
-      if (it->second.retries >= kMaxNackRetries) {
-        RTC_LOG(LS_WARNING) << "Sequence number " << it->second.seq_num
-                            << " removed from NACK list due to max retries.";
-        it = nack_list_.erase(it);
-      } else {
-        ++it;
-      }
-      continue;
-    }
-
-    if (consider_timestamp && it->second.sent_at_time + rtt_ms_ <= now_ms) {
+    bool delay_timed_out =
+        now_ms - it->second.created_at_time >= send_nack_delay_ms_;
+    bool nack_on_rtt_passed = now_ms - it->second.sent_at_time >= rtt_ms_;
+    bool nack_on_seq_num_passed =
+        it->second.sent_at_time == -1 &&
+        AheadOrAt(newest_seq_num_, it->second.send_at_seq_num);
+    if (delay_timed_out && ((consider_seq_num && nack_on_seq_num_passed) ||
+                            (consider_timestamp && nack_on_rtt_passed))) {
       nack_batch.emplace_back(it->second.seq_num);
       ++it->second.retries;
       it->second.sent_at_time = now_ms;

modules/video_coding/nack_module.h

@@ -52,10 +52,13 @@ class NackModule : public Module {
   // we have tried to nack this packet.
   struct NackInfo {
     NackInfo();
-    NackInfo(uint16_t seq_num, uint16_t send_at_seq_num);
+    NackInfo(uint16_t seq_num,
+             uint16_t send_at_seq_num,
+             int64_t created_at_time);
 
     uint16_t seq_num;
     uint16_t send_at_seq_num;
+    int64_t created_at_time;
     int64_t sent_at_time;
     int retries;
   };
@@ -98,6 +101,9 @@ class NackModule : public Module {
 
   // Only touched on the process thread.
   int64_t next_process_time_ms_;
+
+  // Adds a delay before send nack on packet received.
+  const int64_t send_nack_delay_ms_;
 };
 
 }  // namespace webrtc

modules/video_coding/nack_module_unittest.cc

@@ -14,6 +14,7 @@
 #include "modules/video_coding/include/video_coding_defines.h"
 #include "modules/video_coding/nack_module.h"
 #include "system_wrappers/include/clock.h"
+#include "test/field_trial.h"
 #include "test/gtest.h"
 
 namespace webrtc {
@@ -285,4 +286,45 @@ TEST_F(TestNackModule, HandleFecRecoveredPacket) {
   EXPECT_EQ(2u, sent_nacks_.size());
 }
 
+TEST_F(TestNackModule, SendNackWithoutDelay) {
+  nack_module_.OnReceivedPacket(0, false, false);
+  nack_module_.OnReceivedPacket(100, false, false);
+  EXPECT_EQ(99u, sent_nacks_.size());
+}
+
+class TestNackModuleWithFieldTrial : public ::testing::Test,
+                                     public NackSender,
+                                     public KeyFrameRequestSender {
+ protected:
+  TestNackModuleWithFieldTrial()
+      : nack_delay_field_trial_("WebRTC-SendNackDelayMs/10/"),
+        clock_(new SimulatedClock(0)),
+        nack_module_(clock_.get(), this, this),
+        keyframes_requested_(0) {}
+
+  void SendNack(const std::vector<uint16_t>& sequence_numbers) override {
+    sent_nacks_.insert(sent_nacks_.end(), sequence_numbers.begin(),
+                       sequence_numbers.end());
+  }
+
+  void RequestKeyFrame() override { ++keyframes_requested_; }
+
+  test::ScopedFieldTrials nack_delay_field_trial_;
+  std::unique_ptr<SimulatedClock> clock_;
+  NackModule nack_module_;
+  std::vector<uint16_t> sent_nacks_;
+  int keyframes_requested_;
+};
+
+TEST_F(TestNackModuleWithFieldTrial, SendNackWithDelay) {
+  nack_module_.OnReceivedPacket(0, false, false);
+  nack_module_.OnReceivedPacket(100, false, false);
+  EXPECT_EQ(0u, sent_nacks_.size());
+  clock_->AdvanceTimeMilliseconds(10);
+  nack_module_.OnReceivedPacket(106, false, false);
+  EXPECT_EQ(99u, sent_nacks_.size());
+  clock_->AdvanceTimeMilliseconds(10);
+  nack_module_.OnReceivedPacket(109, false, false);
+  EXPECT_EQ(104u, sent_nacks_.size());
+}
 }  // namespace webrtc