diff --git a/modules/video_coding/nack_module.cc b/modules/video_coding/nack_module.cc index 7fbcf34b41..e3eeb84d9f 100644 --- a/modules/video_coding/nack_module.cc +++ b/modules/video_coding/nack_module.cc @@ -56,6 +56,12 @@ NackModule::NackModule(Clock* clock, } int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) { + return OnReceivedPacket(seq_num, is_keyframe, false); +} + +int NackModule::OnReceivedPacket(uint16_t seq_num, + bool is_keyframe, + bool is_recovered) { rtc::CritScope lock(&crit_); // TODO(philipel): When the packet includes information whether it is // retransmitted or not, use that value instead. For @@ -88,8 +94,6 @@ int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) { UpdateReorderingStatistics(seq_num); return nacks_sent_for_packet; } - AddPacketsToNack(newest_seq_num_ + 1, seq_num); - newest_seq_num_ = seq_num; // Keep track of new keyframes. if (is_keyframe) @@ -100,6 +104,21 @@ int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) { if (it != keyframe_list_.begin()) keyframe_list_.erase(keyframe_list_.begin(), it); + if (is_recovered) { + recovered_list_.insert(seq_num); + + // Remove old ones so we don't accumulate recovered packets. + auto it = recovered_list_.lower_bound(seq_num - kMaxPacketAge); + if (it != recovered_list_.begin()) + recovered_list_.erase(recovered_list_.begin(), it); + + // Do not send nack for packets recovered by FEC or RTX. + return 0; + } + + AddPacketsToNack(newest_seq_num_ + 1, seq_num); + newest_seq_num_ = seq_num; + // Are there any nacks that are waiting for this seq_num. std::vector nack_batch = GetNackBatch(kSeqNumOnly); if (!nack_batch.empty()) @@ -113,6 +132,8 @@ void NackModule::ClearUpTo(uint16_t seq_num) { 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)); } void NackModule::UpdateRtt(int64_t rtt_ms) { @@ -124,6 +145,7 @@ void NackModule::Clear() { rtc::CritScope lock(&crit_); nack_list_.clear(); keyframe_list_.clear(); + recovered_list_.clear(); } int64_t NackModule::TimeUntilNextProcess() { @@ -200,6 +222,9 @@ void NackModule::AddPacketsToNack(uint16_t seq_num_start, } for (uint16_t seq_num = seq_num_start; seq_num != seq_num_end; ++seq_num) { + // 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)); RTC_DCHECK(nack_list_.find(seq_num) == nack_list_.end()); nack_list_[seq_num] = nack_info; diff --git a/modules/video_coding/nack_module.h b/modules/video_coding/nack_module.h index 44340e9d4c..1720df9c3c 100644 --- a/modules/video_coding/nack_module.h +++ b/modules/video_coding/nack_module.h @@ -32,6 +32,8 @@ class NackModule : public Module { KeyFrameRequestSender* keyframe_request_sender); int OnReceivedPacket(uint16_t seq_num, bool is_keyframe); + int OnReceivedPacket(uint16_t seq_num, bool is_keyframe, bool is_recovered); + void ClearUpTo(uint16_t seq_num); void UpdateRtt(int64_t rtt_ms); void Clear(); @@ -87,6 +89,8 @@ class NackModule : public Module { RTC_GUARDED_BY(crit_); std::set> keyframe_list_ RTC_GUARDED_BY(crit_); + std::set> recovered_list_ + RTC_GUARDED_BY(crit_); video_coding::Histogram reordering_histogram_ RTC_GUARDED_BY(crit_); bool initialized_ RTC_GUARDED_BY(crit_); int64_t rtt_ms_ RTC_GUARDED_BY(crit_); diff --git a/modules/video_coding/nack_module_unittest.cc b/modules/video_coding/nack_module_unittest.cc index 8a5f46bf7f..208a6f3a34 100644 --- a/modules/video_coding/nack_module_unittest.cc +++ b/modules/video_coding/nack_module_unittest.cc @@ -40,38 +40,38 @@ class TestNackModule : public ::testing::Test, }; TEST_F(TestNackModule, NackOnePacket) { - nack_module_.OnReceivedPacket(1, false); - nack_module_.OnReceivedPacket(3, false); + nack_module_.OnReceivedPacket(1, false, false); + nack_module_.OnReceivedPacket(3, false, false); EXPECT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); } TEST_F(TestNackModule, WrappingSeqNum) { - nack_module_.OnReceivedPacket(0xfffe, false); - nack_module_.OnReceivedPacket(1, false); + nack_module_.OnReceivedPacket(0xfffe, false, false); + nack_module_.OnReceivedPacket(1, false, false); EXPECT_EQ(2u, sent_nacks_.size()); EXPECT_EQ(0xffff, sent_nacks_[0]); EXPECT_EQ(0, sent_nacks_[1]); } TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) { - nack_module_.OnReceivedPacket(0xfffe, false); - nack_module_.OnReceivedPacket(1, false); + nack_module_.OnReceivedPacket(0xfffe, false, false); + nack_module_.OnReceivedPacket(1, false, false); EXPECT_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); + nack_module_.OnReceivedPacket(2, true, false); EXPECT_EQ(0u, sent_nacks_.size()); - nack_module_.OnReceivedPacket(501, true); + nack_module_.OnReceivedPacket(501, true, false); EXPECT_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); + 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]); @@ -91,7 +91,7 @@ TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) { // 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); + nack_module_.OnReceivedPacket(1004, false, false); EXPECT_EQ(2u, sent_nacks_.size()); EXPECT_EQ(1002, sent_nacks_[0]); EXPECT_EQ(1003, sent_nacks_[1]); @@ -107,7 +107,7 @@ TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) { // 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); + nack_module_.OnReceivedPacket(1007, false, false); sent_nacks_.clear(); clock_->AdvanceTimeMilliseconds(100); nack_module_.Process(); @@ -146,8 +146,8 @@ TEST_F(TestNackModule, DontBurstOnTimeSkip) { } TEST_F(TestNackModule, ResendNack) { - nack_module_.OnReceivedPacket(1, false); - nack_module_.OnReceivedPacket(3, false); + nack_module_.OnReceivedPacket(1, false, false); + nack_module_.OnReceivedPacket(3, false, false); EXPECT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); @@ -169,15 +169,15 @@ TEST_F(TestNackModule, ResendNack) { nack_module_.Process(); EXPECT_EQ(4u, sent_nacks_.size()); - nack_module_.OnReceivedPacket(2, false); + nack_module_.OnReceivedPacket(2, false, false); clock_->AdvanceTimeMilliseconds(50); nack_module_.Process(); EXPECT_EQ(4u, sent_nacks_.size()); } TEST_F(TestNackModule, ResendPacketMaxRetries) { - nack_module_.OnReceivedPacket(1, false); - nack_module_.OnReceivedPacket(3, false); + nack_module_.OnReceivedPacket(1, false, false); + nack_module_.OnReceivedPacket(3, false, false); EXPECT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); @@ -193,35 +193,35 @@ TEST_F(TestNackModule, ResendPacketMaxRetries) { } TEST_F(TestNackModule, TooLargeNackList) { - nack_module_.OnReceivedPacket(0, false); - nack_module_.OnReceivedPacket(1001, false); + nack_module_.OnReceivedPacket(0, false, false); + nack_module_.OnReceivedPacket(1001, false, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(0, keyframes_requested_); - nack_module_.OnReceivedPacket(1003, false); + nack_module_.OnReceivedPacket(1003, false, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); - nack_module_.OnReceivedPacket(1004, false); + nack_module_.OnReceivedPacket(1004, false, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); } TEST_F(TestNackModule, TooLargeNackListWithKeyFrame) { - nack_module_.OnReceivedPacket(0, false); - nack_module_.OnReceivedPacket(1, true); - nack_module_.OnReceivedPacket(1001, false); + 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); + nack_module_.OnReceivedPacket(1003, false, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(0, keyframes_requested_); - nack_module_.OnReceivedPacket(1005, false); + nack_module_.OnReceivedPacket(1005, false, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); } TEST_F(TestNackModule, ClearUpTo) { - nack_module_.OnReceivedPacket(0, false); - nack_module_.OnReceivedPacket(100, false); + nack_module_.OnReceivedPacket(0, false, false); + nack_module_.OnReceivedPacket(100, false, false); EXPECT_EQ(99u, sent_nacks_.size()); sent_nacks_.clear(); @@ -233,8 +233,8 @@ TEST_F(TestNackModule, ClearUpTo) { } TEST_F(TestNackModule, ClearUpToWrap) { - nack_module_.OnReceivedPacket(0xfff0, false); - nack_module_.OnReceivedPacket(0xf, false); + nack_module_.OnReceivedPacket(0xfff0, false, false); + nack_module_.OnReceivedPacket(0xf, false, false); EXPECT_EQ(30u, sent_nacks_.size()); sent_nacks_.clear(); @@ -246,20 +246,20 @@ TEST_F(TestNackModule, ClearUpToWrap) { } TEST_F(TestNackModule, PacketNackCount) { - EXPECT_EQ(0, nack_module_.OnReceivedPacket(0, false)); - EXPECT_EQ(0, nack_module_.OnReceivedPacket(2, false)); - EXPECT_EQ(1, nack_module_.OnReceivedPacket(1, false)); + 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)); sent_nacks_.clear(); nack_module_.UpdateRtt(100); - EXPECT_EQ(0, nack_module_.OnReceivedPacket(5, false)); + EXPECT_EQ(0, nack_module_.OnReceivedPacket(5, false, false)); clock_->AdvanceTimeMilliseconds(100); nack_module_.Process(); clock_->AdvanceTimeMilliseconds(100); nack_module_.Process(); - EXPECT_EQ(3, nack_module_.OnReceivedPacket(3, false)); - EXPECT_EQ(3, nack_module_.OnReceivedPacket(4, false)); - EXPECT_EQ(0, nack_module_.OnReceivedPacket(4, false)); + 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(TestNackModule, NackListFullAndNoOverlapWithKeyframes) { @@ -267,14 +267,22 @@ TEST_F(TestNackModule, NackListFullAndNoOverlapWithKeyframes) { const unsigned int kFirstGap = kMaxNackPackets - 20; const unsigned int kSecondGap = 200; uint16_t seq_num = 0; - nack_module_.OnReceivedPacket(seq_num++, true); + nack_module_.OnReceivedPacket(seq_num++, true, false); seq_num += kFirstGap; - nack_module_.OnReceivedPacket(seq_num++, true); + 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); + nack_module_.OnReceivedPacket(seq_num, true, false); EXPECT_EQ(kSecondGap, sent_nacks_.size()); } +TEST_F(TestNackModule, HandleFecRecoveredPacket) { + nack_module_.OnReceivedPacket(1, false, false); + nack_module_.OnReceivedPacket(4, false, true); + EXPECT_EQ(0u, sent_nacks_.size()); + nack_module_.OnReceivedPacket(5, false, false); + EXPECT_EQ(2u, sent_nacks_.size()); +} + } // namespace webrtc diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index c6b297da25..54956688ad 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -218,14 +218,15 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( size_t payload_size, const WebRtcRTPHeader* rtp_header) { return OnReceivedPayloadData(payload_data, payload_size, rtp_header, - absl::nullopt); + absl::nullopt, false); } int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( const uint8_t* payload_data, size_t payload_size, const WebRtcRTPHeader* rtp_header, - const absl::optional& generic_descriptor) { + const absl::optional& generic_descriptor, + bool is_recovered) { WebRtcRTPHeader rtp_header_with_ntp = *rtp_header; rtp_header_with_ntp.ntp_time_ms = ntp_estimator_.Estimate(rtp_header->header.timestamp); @@ -237,7 +238,8 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( rtp_header->frameType == kVideoFrameKey; packet.timesNacked = nack_module_->OnReceivedPacket( - rtp_header->header.sequenceNumber, is_keyframe); + rtp_header->header.sequenceNumber, is_keyframe, is_recovered); + } else { packet.timesNacked = -1; } @@ -568,7 +570,8 @@ void RtpVideoStreamReceiver::ReceivePacket(const RtpPacketReceived& packet) { } OnReceivedPayloadData(parsed_payload.payload, parsed_payload.payload_length, - &webrtc_rtp_header, generic_descriptor_wire); + &webrtc_rtp_header, generic_descriptor_wire, + packet.recovered()); } void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader( @@ -600,7 +603,8 @@ void RtpVideoStreamReceiver::NotifyReceiverOfEmptyPacket(uint16_t seq_num) { reference_finder_->PaddingReceived(seq_num); packet_buffer_->PaddingReceived(seq_num); if (nack_module_) { - nack_module_->OnReceivedPacket(seq_num, /* is_keyframe = */ false); + nack_module_->OnReceivedPacket(seq_num, /* is_keyframe = */ false, + /* is _recovered = */ false); } } diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index ed5164769e..a2006d1bb2 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -104,7 +104,8 @@ class RtpVideoStreamReceiver : public RecoveredPacketReceiver, const uint8_t* payload_data, size_t payload_size, const WebRtcRTPHeader* rtp_header, - const absl::optional& generic_descriptor); + const absl::optional& generic_descriptor, + bool is_recovered); // Implements RecoveredPacketReceiver. void OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override;