From 0367d1a1fb0a74ddb0e256625c48107648675885 Mon Sep 17 00:00:00 2001 From: Ying Wang Date: Fri, 2 Nov 2018 14:51:15 +0100 Subject: [PATCH] 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 Reviewed-by: Philip Eliasson Commit-Queue: Ying Wang Cr-Commit-Position: refs/heads/master@{#25488} --- modules/video_coding/BUILD.gn | 1 + modules/video_coding/nack_module.cc | 48 ++++++++++++-------- modules/video_coding/nack_module.h | 8 +++- modules/video_coding/nack_module_unittest.cc | 42 +++++++++++++++++ 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index ff1e202ad9..15954af663 100644 --- a/modules/video_coding/BUILD.gn +++ b/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", ] } diff --git a/modules/video_coding/nack_module.cc b/modules/video_coding/nack_module.cc index e3eeb84d9f..0c69b57749 100644 --- a/modules/video_coding/nack_module.cc +++ b/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 NackModule::GetNackBatch(NackFilterOptions options) { std::vector 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; diff --git a/modules/video_coding/nack_module.h b/modules/video_coding/nack_module.h index 1720df9c3c..f86dd16db2 100644 --- a/modules/video_coding/nack_module.h +++ b/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 diff --git a/modules/video_coding/nack_module_unittest.cc b/modules/video_coding/nack_module_unittest.cc index 208a6f3a34..8efb8ac154 100644 --- a/modules/video_coding/nack_module_unittest.cc +++ b/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& 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 clock_; + NackModule nack_module_; + std::vector 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