From d257cb73335c12b0757c217ea647fd60ce877df4 Mon Sep 17 00:00:00 2001 From: philipel Date: Tue, 16 Jan 2024 15:55:44 +0100 Subject: [PATCH] Remove keyframe tracking from NackRequester. Tracking keyframe packets is a useless optimization that kicked in when the nack list is full (1000 packets). Bug: none Change-Id: I134ecb4d51131718e5bb8775847fbde18f262ef9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/334645 Reviewed-by: Danil Chapovalov Commit-Queue: Philip Eliasson Cr-Commit-Position: refs/heads/main@{#41547} --- modules/video_coding/nack_requester.cc | 56 +----- modules/video_coding/nack_requester.h | 10 +- .../video_coding/nack_requester_unittest.cc | 160 ++++-------------- video/rtp_video_stream_receiver2.cc | 9 +- 4 files changed, 43 insertions(+), 192 deletions(-) diff --git a/modules/video_coding/nack_requester.cc b/modules/video_coding/nack_requester.cc index 008420f4da..b3e928d05e 100644 --- a/modules/video_coding/nack_requester.cc +++ b/modules/video_coding/nack_requester.cc @@ -141,13 +141,12 @@ void NackRequester::ProcessNacks() { } } -int NackRequester::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) { +int NackRequester::OnReceivedPacket(uint16_t seq_num) { RTC_DCHECK_RUN_ON(worker_thread_); - return OnReceivedPacket(seq_num, is_keyframe, false); + return OnReceivedPacket(seq_num, false); } int NackRequester::OnReceivedPacket(uint16_t seq_num, - bool is_keyframe, bool is_recovered) { RTC_DCHECK_RUN_ON(worker_thread_); // TODO(philipel): When the packet includes information whether it is @@ -158,8 +157,6 @@ int NackRequester::OnReceivedPacket(uint16_t seq_num, if (!initialized_) { newest_seq_num_ = seq_num; - if (is_keyframe) - keyframe_list_.insert(seq_num); initialized_ = true; return 0; } @@ -182,15 +179,6 @@ int NackRequester::OnReceivedPacket(uint16_t seq_num, return nacks_sent_for_packet; } - // Keep track of new keyframes. - if (is_keyframe) - keyframe_list_.insert(seq_num); - - // And remove old ones so we don't accumulate keyframes. - auto it = keyframe_list_.lower_bound(seq_num - kMaxPacketAge); - if (it != keyframe_list_.begin()) - keyframe_list_.erase(keyframe_list_.begin(), it); - if (is_recovered) { recovered_list_.insert(seq_num); @@ -225,8 +213,6 @@ void NackRequester::ClearUpTo(uint16_t seq_num) { // thread. RTC_DCHECK_RUN_ON(worker_thread_); nack_list_.erase(nack_list_.begin(), nack_list_.lower_bound(seq_num)); - keyframe_list_.erase(keyframe_list_.begin(), - keyframe_list_.lower_bound(seq_num)); recovered_list_.erase(recovered_list_.begin(), recovered_list_.lower_bound(seq_num)); } @@ -236,25 +222,6 @@ void NackRequester::UpdateRtt(int64_t rtt_ms) { rtt_ = TimeDelta::Millis(rtt_ms); } -bool NackRequester::RemovePacketsUntilKeyFrame() { - // Called on worker_thread_. - while (!keyframe_list_.empty()) { - auto it = nack_list_.lower_bound(*keyframe_list_.begin()); - - if (it != nack_list_.begin()) { - // We have found a keyframe that actually is newer than at least one - // packet in the nack list. - nack_list_.erase(nack_list_.begin(), it); - return true; - } - - // If this keyframe is so old it does not remove any packets from the list, - // remove it from the list of keyframes and try the next keyframe. - keyframe_list_.erase(keyframe_list_.begin()); - } - return false; -} - void NackRequester::AddPacketsToNack(uint16_t seq_num_start, uint16_t seq_num_end) { // Called on worker_thread_. @@ -262,22 +229,13 @@ void NackRequester::AddPacketsToNack(uint16_t seq_num_start, auto it = nack_list_.lower_bound(seq_num_end - kMaxPacketAge); nack_list_.erase(nack_list_.begin(), it); - // If the nack list is too large, remove packets from the nack list until - // the latest first packet of a keyframe. If the list is still too large, - // clear it and request a keyframe. uint16_t num_new_nacks = ForwardDiff(seq_num_start, seq_num_end); if (nack_list_.size() + num_new_nacks > kMaxNackPackets) { - while (RemovePacketsUntilKeyFrame() && - nack_list_.size() + num_new_nacks > kMaxNackPackets) { - } - - if (nack_list_.size() + num_new_nacks > kMaxNackPackets) { - nack_list_.clear(); - RTC_LOG(LS_WARNING) << "NACK list full, clearing NACK" - " list and requesting keyframe."; - keyframe_request_sender_->RequestKeyFrame(); - return; - } + nack_list_.clear(); + RTC_LOG(LS_WARNING) << "NACK list full, clearing NACK" + " list and requesting keyframe."; + keyframe_request_sender_->RequestKeyFrame(); + return; } for (uint16_t seq_num = seq_num_start; seq_num != seq_num_end; ++seq_num) { diff --git a/modules/video_coding/nack_requester.h b/modules/video_coding/nack_requester.h index c860787dcf..b5ef30f01b 100644 --- a/modules/video_coding/nack_requester.h +++ b/modules/video_coding/nack_requester.h @@ -78,8 +78,8 @@ class NackRequester final : public NackRequesterBase { void ProcessNacks() override; - int OnReceivedPacket(uint16_t seq_num, bool is_keyframe); - int OnReceivedPacket(uint16_t seq_num, bool is_keyframe, bool is_recovered); + int OnReceivedPacket(uint16_t seq_num); + int OnReceivedPacket(uint16_t seq_num, bool is_recovered); void ClearUpTo(uint16_t seq_num); void UpdateRtt(int64_t rtt_ms); @@ -108,10 +108,6 @@ class NackRequester final : public NackRequesterBase { void AddPacketsToNack(uint16_t seq_num_start, uint16_t seq_num_end) RTC_EXCLUSIVE_LOCKS_REQUIRED(worker_thread_); - // Removes packets from the nack list until the next keyframe. Returns true - // if packets were removed. - bool RemovePacketsUntilKeyFrame() - RTC_EXCLUSIVE_LOCKS_REQUIRED(worker_thread_); std::vector GetNackBatch(NackFilterOptions options) RTC_EXCLUSIVE_LOCKS_REQUIRED(worker_thread_); @@ -134,8 +130,6 @@ class NackRequester final : public NackRequesterBase { // synchronized access. std::map> nack_list_ RTC_GUARDED_BY(worker_thread_); - std::set> keyframe_list_ - RTC_GUARDED_BY(worker_thread_); std::set> recovered_list_ RTC_GUARDED_BY(worker_thread_); video_coding::Histogram reordering_histogram_ RTC_GUARDED_BY(worker_thread_); diff --git a/modules/video_coding/nack_requester_unittest.cc b/modules/video_coding/nack_requester_unittest.cc index 6f11cb6e91..2f24df2aef 100644 --- a/modules/video_coding/nack_requester_unittest.cc +++ b/modules/video_coding/nack_requester_unittest.cc @@ -102,90 +102,25 @@ class TestNackRequester : public ::testing::Test, TEST_F(TestNackRequester, NackOnePacket) { NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(1, false, false); - nack_module.OnReceivedPacket(3, false, false); + nack_module.OnReceivedPacket(1); + nack_module.OnReceivedPacket(3); ASSERT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); } TEST_F(TestNackRequester, WrappingSeqNum) { NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(0xfffe, false, false); - nack_module.OnReceivedPacket(1, false, false); + nack_module.OnReceivedPacket(0xfffe); + nack_module.OnReceivedPacket(1); ASSERT_EQ(2u, sent_nacks_.size()); EXPECT_EQ(0xffff, sent_nacks_[0]); EXPECT_EQ(0, sent_nacks_[1]); } -TEST_F(TestNackRequester, WrappingSeqNumClearToKeyframe) { - NackRequester& nack_module = CreateNackModule(TimeDelta::Millis(10)); - nack_module.OnReceivedPacket(0xfffe, false, false); - nack_module.OnReceivedPacket(1, false, false); - ASSERT_EQ(2u, sent_nacks_.size()); - EXPECT_EQ(0xffff, sent_nacks_[0]); - EXPECT_EQ(0, sent_nacks_[1]); - - sent_nacks_.clear(); - nack_module.OnReceivedPacket(2, true, false); - ASSERT_EQ(0u, sent_nacks_.size()); - - nack_module.OnReceivedPacket(501, true, false); - ASSERT_EQ(498u, sent_nacks_.size()); - for (int seq_num = 3; seq_num < 501; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 3]); - - sent_nacks_.clear(); - nack_module.OnReceivedPacket(1001, false, false); - EXPECT_EQ(499u, sent_nacks_.size()); - for (int seq_num = 502; seq_num < 1001; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 502]); - - sent_nacks_.clear(); - clock_->AdvanceTimeMilliseconds(100); - ASSERT_TRUE(WaitForSendNack()); - ASSERT_EQ(999u, sent_nacks_.size()); - EXPECT_EQ(0xffff, sent_nacks_[0]); - EXPECT_EQ(0, sent_nacks_[1]); - for (int seq_num = 3; seq_num < 501; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 1]); - for (int seq_num = 502; seq_num < 1001; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 2]); - - // Adding packet 1004 will cause the nack list to reach it's max limit. - // It will then clear all nacks up to the next keyframe (seq num 2), - // thus removing 0xffff and 0 from the nack list. - sent_nacks_.clear(); - nack_module.OnReceivedPacket(1004, false, false); - ASSERT_EQ(2u, sent_nacks_.size()); - EXPECT_EQ(1002, sent_nacks_[0]); - EXPECT_EQ(1003, sent_nacks_[1]); - - sent_nacks_.clear(); - clock_->AdvanceTimeMilliseconds(100); - ASSERT_TRUE(WaitForSendNack()); - ASSERT_EQ(999u, sent_nacks_.size()); - for (int seq_num = 3; seq_num < 501; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 3]); - for (int seq_num = 502; seq_num < 1001; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 4]); - - // Adding packet 1007 will cause the nack module to overflow again, thus - // clearing everything up to 501 which is the next keyframe. - nack_module.OnReceivedPacket(1007, false, false); - sent_nacks_.clear(); - clock_->AdvanceTimeMilliseconds(100); - ASSERT_TRUE(WaitForSendNack()); - ASSERT_EQ(503u, sent_nacks_.size()); - for (int seq_num = 502; seq_num < 1001; ++seq_num) - EXPECT_EQ(seq_num, sent_nacks_[seq_num - 502]); - EXPECT_EQ(1005, sent_nacks_[501]); - EXPECT_EQ(1006, sent_nacks_[502]); -} - TEST_F(TestNackRequester, ResendNack) { NackRequester& nack_module = CreateNackModule(TimeDelta::Millis(1)); - nack_module.OnReceivedPacket(1, false, false); - nack_module.OnReceivedPacket(3, false, false); + nack_module.OnReceivedPacket(1); + nack_module.OnReceivedPacket(3); size_t expected_nacks_sent = 1; ASSERT_EQ(expected_nacks_sent, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); @@ -225,8 +160,8 @@ TEST_F(TestNackRequester, ResendNack) { TEST_F(TestNackRequester, ResendPacketMaxRetries) { NackRequester& nack_module = CreateNackModule(TimeDelta::Millis(1)); - nack_module.OnReceivedPacket(1, false, false); - nack_module.OnReceivedPacket(3, false, false); + nack_module.OnReceivedPacket(1); + nack_module.OnReceivedPacket(3); ASSERT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); @@ -246,37 +181,22 @@ TEST_F(TestNackRequester, ResendPacketMaxRetries) { TEST_F(TestNackRequester, TooLargeNackList) { NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(0, false, false); - nack_module.OnReceivedPacket(1001, false, false); + nack_module.OnReceivedPacket(0); + nack_module.OnReceivedPacket(1001); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(0, keyframes_requested_); - nack_module.OnReceivedPacket(1003, false, false); + nack_module.OnReceivedPacket(1003); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); - nack_module.OnReceivedPacket(1004, false, false); - EXPECT_EQ(1000u, sent_nacks_.size()); - EXPECT_EQ(1, keyframes_requested_); -} - -TEST_F(TestNackRequester, TooLargeNackListWithKeyFrame) { - NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(0, false, false); - nack_module.OnReceivedPacket(1, true, false); - nack_module.OnReceivedPacket(1001, false, false); - EXPECT_EQ(999u, sent_nacks_.size()); - EXPECT_EQ(0, keyframes_requested_); - nack_module.OnReceivedPacket(1003, false, false); - EXPECT_EQ(1000u, sent_nacks_.size()); - EXPECT_EQ(0, keyframes_requested_); - nack_module.OnReceivedPacket(1005, false, false); + nack_module.OnReceivedPacket(1004); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); } TEST_F(TestNackRequester, ClearUpTo) { NackRequester& nack_module = CreateNackModule(TimeDelta::Millis(1)); - nack_module.OnReceivedPacket(0, false, false); - nack_module.OnReceivedPacket(100, false, false); + nack_module.OnReceivedPacket(0); + nack_module.OnReceivedPacket(100); EXPECT_EQ(99u, sent_nacks_.size()); sent_nacks_.clear(); @@ -289,8 +209,8 @@ TEST_F(TestNackRequester, ClearUpTo) { TEST_F(TestNackRequester, ClearUpToWrap) { NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(0xfff0, false, false); - nack_module.OnReceivedPacket(0xf, false, false); + nack_module.OnReceivedPacket(0xfff0); + nack_module.OnReceivedPacket(0xf); EXPECT_EQ(30u, sent_nacks_.size()); sent_nacks_.clear(); @@ -303,13 +223,13 @@ TEST_F(TestNackRequester, ClearUpToWrap) { TEST_F(TestNackRequester, PacketNackCount) { NackRequester& nack_module = CreateNackModule(TimeDelta::Millis(1)); - EXPECT_EQ(0, nack_module.OnReceivedPacket(0, false, false)); - EXPECT_EQ(0, nack_module.OnReceivedPacket(2, false, false)); - EXPECT_EQ(1, nack_module.OnReceivedPacket(1, false, false)); + EXPECT_EQ(0, nack_module.OnReceivedPacket(0)); + EXPECT_EQ(0, nack_module.OnReceivedPacket(2)); + EXPECT_EQ(1, nack_module.OnReceivedPacket(1)); sent_nacks_.clear(); nack_module.UpdateRtt(100); - EXPECT_EQ(0, nack_module.OnReceivedPacket(5, false, false)); + EXPECT_EQ(0, nack_module.OnReceivedPacket(5)); clock_->AdvanceTimeMilliseconds(100); WaitForSendNack(); EXPECT_EQ(4u, sent_nacks_.size()); @@ -319,40 +239,24 @@ TEST_F(TestNackRequester, PacketNackCount) { EXPECT_EQ(6u, sent_nacks_.size()); - EXPECT_EQ(3, nack_module.OnReceivedPacket(3, false, false)); - EXPECT_EQ(3, nack_module.OnReceivedPacket(4, false, false)); - EXPECT_EQ(0, nack_module.OnReceivedPacket(4, false, false)); -} - -TEST_F(TestNackRequester, NackListFullAndNoOverlapWithKeyframes) { - NackRequester& nack_module = CreateNackModule(); - const int kMaxNackPackets = 1000; - const unsigned int kFirstGap = kMaxNackPackets - 20; - const unsigned int kSecondGap = 200; - uint16_t seq_num = 0; - nack_module.OnReceivedPacket(seq_num++, true, false); - seq_num += kFirstGap; - nack_module.OnReceivedPacket(seq_num++, true, false); - EXPECT_EQ(kFirstGap, sent_nacks_.size()); - sent_nacks_.clear(); - seq_num += kSecondGap; - nack_module.OnReceivedPacket(seq_num, true, false); - EXPECT_EQ(kSecondGap, sent_nacks_.size()); + EXPECT_EQ(3, nack_module.OnReceivedPacket(3)); + EXPECT_EQ(3, nack_module.OnReceivedPacket(4)); + EXPECT_EQ(0, nack_module.OnReceivedPacket(4)); } TEST_F(TestNackRequester, HandleFecRecoveredPacket) { NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(1, false, false); - nack_module.OnReceivedPacket(4, false, true); + nack_module.OnReceivedPacket(1); + nack_module.OnReceivedPacket(4, /*is_recovered=*/true); EXPECT_EQ(0u, sent_nacks_.size()); - nack_module.OnReceivedPacket(5, false, false); + nack_module.OnReceivedPacket(5); EXPECT_EQ(2u, sent_nacks_.size()); } TEST_F(TestNackRequester, SendNackWithoutDelay) { NackRequester& nack_module = CreateNackModule(); - nack_module.OnReceivedPacket(0, false, false); - nack_module.OnReceivedPacket(100, false, false); + nack_module.OnReceivedPacket(0); + nack_module.OnReceivedPacket(100); EXPECT_EQ(99u, sent_nacks_.size()); } @@ -389,14 +293,14 @@ class TestNackRequesterWithFieldTrial : public ::testing::Test, }; TEST_F(TestNackRequesterWithFieldTrial, SendNackWithDelay) { - nack_module_.OnReceivedPacket(0, false, false); - nack_module_.OnReceivedPacket(100, false, false); + nack_module_.OnReceivedPacket(0); + nack_module_.OnReceivedPacket(100); EXPECT_EQ(0u, sent_nacks_.size()); clock_->AdvanceTimeMilliseconds(10); - nack_module_.OnReceivedPacket(106, false, false); + nack_module_.OnReceivedPacket(106); EXPECT_EQ(99u, sent_nacks_.size()); clock_->AdvanceTimeMilliseconds(10); - nack_module_.OnReceivedPacket(109, false, false); + nack_module_.OnReceivedPacket(109); EXPECT_EQ(104u, sent_nacks_.size()); } } // namespace webrtc diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc index 9b8733f775..e405d77fa6 100644 --- a/video/rtp_video_stream_receiver2.cc +++ b/video/rtp_video_stream_receiver2.cc @@ -654,12 +654,8 @@ void RtpVideoStreamReceiver2::OnReceivedPayloadData( } if (nack_module_) { - const bool is_keyframe = - video_header.is_first_packet_in_frame && - video_header.frame_type == VideoFrameType::kVideoFrameKey; - packet->times_nacked = nack_module_->OnReceivedPacket( - rtp_packet.SequenceNumber(), is_keyframe, rtp_packet.recovered()); + rtp_packet.SequenceNumber(), rtp_packet.recovered()); } else { packet->times_nacked = -1; } @@ -1129,8 +1125,7 @@ void RtpVideoStreamReceiver2::NotifyReceiverOfEmptyPacket(uint16_t seq_num) { OnInsertedPacket(packet_buffer_.InsertPadding(seq_num)); if (nack_module_) { - nack_module_->OnReceivedPacket(seq_num, /* is_keyframe = */ false, - /* is _recovered = */ false); + nack_module_->OnReceivedPacket(seq_num, /*is_recovered=*/false); } if (loss_notification_controller_) { // TODO(bugs.webrtc.org/10336): Handle empty packets.