From 1a830c2c66a5f9e25e081e07694090468dcf4743 Mon Sep 17 00:00:00 2001 From: philipel Date: Fri, 13 May 2016 11:12:00 +0200 Subject: [PATCH] Nack count returned on OnReceivedPacket. OnReceivedPacket now return the number of times the packet has been nacked. Also some minor refactoring. BUG=webrtc:5514 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/1972123002 . Cr-Commit-Position: refs/heads/master@{#12717} --- webrtc/modules/video_coding/nack_module.cc | 68 +++++++++++-------- webrtc/modules/video_coding/nack_module.h | 15 ++-- .../video_coding/nack_module_unittest.cc | 24 +++++++ 3 files changed, 68 insertions(+), 39 deletions(-) diff --git a/webrtc/modules/video_coding/nack_module.cc b/webrtc/modules/video_coding/nack_module.cc index 1b12afe0f0..43244321ea 100644 --- a/webrtc/modules/video_coding/nack_module.cc +++ b/webrtc/modules/video_coding/nack_module.cc @@ -49,17 +49,17 @@ NackModule::NackModule(Clock* clock, running_(true), initialized_(false), rtt_ms_(kDefaultRttMs), - last_seq_num_(0), + newest_seq_num_(0), next_process_time_ms_(-1) { RTC_DCHECK(clock_); RTC_DCHECK(nack_sender_); RTC_DCHECK(keyframe_request_sender_); } -void NackModule::OnReceivedPacket(const VCMPacket& packet) { +int NackModule::OnReceivedPacket(const VCMPacket& packet) { rtc::CritScope lock(&crit_); if (!running_) - return; + return -1; uint16_t seq_num = packet.seqNum; // TODO(philipel): When the packet includes information whether it is // retransmitted or not, use that value instead. For @@ -69,40 +69,48 @@ void NackModule::OnReceivedPacket(const VCMPacket& packet) { bool is_keyframe = packet.isFirstPacket && packet.frameType == kVideoFrameKey; if (!initialized_) { - last_seq_num_ = seq_num; + newest_seq_num_ = seq_num; if (is_keyframe) keyframe_list_.insert(seq_num); initialized_ = true; - return; + return 0; } - if (seq_num == last_seq_num_) - return; + // Since the |newest_seq_num_| is a packet we have actually received we know + // that packet has never been Nacked. + if (seq_num == newest_seq_num_) + return 0; - if (AheadOf(last_seq_num_, seq_num)) { + if (AheadOf(newest_seq_num_, seq_num)) { // An out of order packet has been received. - nack_list_.erase(seq_num); + auto nack_list_it = nack_list_.find(seq_num); + int nacks_sent_for_packet = 0; + if (nack_list_it != nack_list_.end()) { + nacks_sent_for_packet = nack_list_it->second.retries; + nack_list_.erase(nack_list_it); + } if (!is_retransmitted) UpdateReorderingStatistics(seq_num); - return; - } else { - AddPacketsToNack(last_seq_num_ + 1, seq_num); - last_seq_num_ = seq_num; - - // 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); - - // Are there any nacks that are waiting for this seq_num. - std::vector nack_batch = GetNackBatch(kSeqNumOnly); - if (!nack_batch.empty()) - nack_sender_->SendNack(nack_batch); + 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) + 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); + + // Are there any nacks that are waiting for this seq_num. + std::vector nack_batch = GetNackBatch(kSeqNumOnly); + if (!nack_batch.empty()) + nack_sender_->SendNack(nack_batch); + + return 0; } void NackModule::ClearUpTo(uint16_t seq_num) { @@ -215,7 +223,7 @@ std::vector NackModule::GetNackBatch(NackFilterOptions options) { auto it = nack_list_.begin(); while (it != nack_list_.end()) { if (consider_seq_num && it->second.sent_at_time == -1 && - AheadOrAt(last_seq_num_, it->second.send_at_seq_num)) { + 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; @@ -248,8 +256,8 @@ std::vector NackModule::GetNackBatch(NackFilterOptions options) { } void NackModule::UpdateReorderingStatistics(uint16_t seq_num) { - RTC_DCHECK(AheadOf(last_seq_num_, seq_num)); - uint16_t diff = ReverseDiff(last_seq_num_, seq_num); + RTC_DCHECK(AheadOf(newest_seq_num_, seq_num)); + uint16_t diff = ReverseDiff(newest_seq_num_, seq_num); reordering_histogram_.Add(diff); } diff --git a/webrtc/modules/video_coding/nack_module.h b/webrtc/modules/video_coding/nack_module.h index 7163a8e905..58d6cfa985 100644 --- a/webrtc/modules/video_coding/nack_module.h +++ b/webrtc/modules/video_coding/nack_module.h @@ -32,7 +32,7 @@ class NackModule : public Module { NackSender* nack_sender, KeyFrameRequestSender* keyframe_request_sender); - void OnReceivedPacket(const VCMPacket& packet); + int OnReceivedPacket(const VCMPacket& packet); void ClearUpTo(uint16_t seq_num); void UpdateRtt(int64_t rtt_ms); void Clear(); @@ -59,11 +59,6 @@ class NackModule : public Module { int64_t sent_at_time; int retries; }; - - struct SeqNumComparator { - bool operator()(uint16_t s1, uint16_t s2) const { return AheadOf(s2, s1); } - }; - void AddPacketsToNack(uint16_t seq_num_start, uint16_t seq_num_end) EXCLUSIVE_LOCKS_REQUIRED(crit_); @@ -87,13 +82,15 @@ class NackModule : public Module { NackSender* const nack_sender_; KeyFrameRequestSender* const keyframe_request_sender_; - std::map nack_list_ GUARDED_BY(crit_); - std::set keyframe_list_ GUARDED_BY(crit_); + std::map> nack_list_ + GUARDED_BY(crit_); + std::set> keyframe_list_ + GUARDED_BY(crit_); video_coding::Histogram reordering_histogram_ GUARDED_BY(crit_); bool running_ GUARDED_BY(crit_); bool initialized_ GUARDED_BY(crit_); int64_t rtt_ms_ GUARDED_BY(crit_); - uint16_t last_seq_num_ GUARDED_BY(crit_); + uint16_t newest_seq_num_ GUARDED_BY(crit_); int64_t next_process_time_ms_ GUARDED_BY(crit_); }; diff --git a/webrtc/modules/video_coding/nack_module_unittest.cc b/webrtc/modules/video_coding/nack_module_unittest.cc index 3870742016..9c2eb4ac0c 100644 --- a/webrtc/modules/video_coding/nack_module_unittest.cc +++ b/webrtc/modules/video_coding/nack_module_unittest.cc @@ -290,4 +290,28 @@ TEST_F(TestNackModule, ClearUpToWrap) { EXPECT_EQ(0, sent_nacks_[0]); } +TEST_F(TestNackModule, PacketNackCount) { + VCMPacket packet; + packet.seqNum = 0; + EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); + packet.seqNum = 2; + EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); + packet.seqNum = 1; + EXPECT_EQ(1, nack_module_.OnReceivedPacket(packet)); + + sent_nacks_.clear(); + nack_module_.UpdateRtt(100); + packet.seqNum = 5; + EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); + clock_->AdvanceTimeMilliseconds(100); + nack_module_.Process(); + clock_->AdvanceTimeMilliseconds(100); + nack_module_.Process(); + packet.seqNum = 3; + EXPECT_EQ(3, nack_module_.OnReceivedPacket(packet)); + packet.seqNum = 4; + EXPECT_EQ(3, nack_module_.OnReceivedPacket(packet)); + EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); +} + } // namespace webrtc