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 <danilchap@webrtc.org>
Commit-Queue: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41547}
This commit is contained in:
philipel 2024-01-16 15:55:44 +01:00 committed by WebRTC LUCI CQ
parent 1fee69cfff
commit d257cb7333
4 changed files with 43 additions and 192 deletions

View File

@ -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) {

View File

@ -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<uint16_t> GetNackBatch(NackFilterOptions options)
RTC_EXCLUSIVE_LOCKS_REQUIRED(worker_thread_);
@ -134,8 +130,6 @@ class NackRequester final : public NackRequesterBase {
// synchronized access.
std::map<uint16_t, NackInfo, DescendingSeqNumComp<uint16_t>> nack_list_
RTC_GUARDED_BY(worker_thread_);
std::set<uint16_t, DescendingSeqNumComp<uint16_t>> keyframe_list_
RTC_GUARDED_BY(worker_thread_);
std::set<uint16_t, DescendingSeqNumComp<uint16_t>> recovered_list_
RTC_GUARDED_BY(worker_thread_);
video_coding::Histogram reordering_histogram_ RTC_GUARDED_BY(worker_thread_);

View File

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

View File

@ -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.