diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index 5f59275027..25f4ac8a94 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -794,11 +794,12 @@ void ChannelReceive::SetNACKStatus(bool enable, int max_packets) { RTC_DCHECK(worker_thread_checker_.IsCurrent()); // None of these functions can fail. if (enable) { - rtp_receive_statistics_->SetMaxReorderingThreshold(max_packets); + rtp_receive_statistics_->SetMaxReorderingThreshold(remote_ssrc_, + max_packets); audio_coding_->EnableNack(max_packets); } else { rtp_receive_statistics_->SetMaxReorderingThreshold( - kDefaultMaxReorderingThreshold); + remote_ssrc_, kDefaultMaxReorderingThreshold); audio_coding_->DisableNack(); } } diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h index c299ea69d6..36e7b2496c 100644 --- a/modules/rtp_rtcp/include/receive_statistics.h +++ b/modules/rtp_rtcp/include/receive_statistics.h @@ -71,8 +71,14 @@ class ReceiveStatistics : public ReceiveStatisticsProvider, // Returns a pointer to the statistician of an ssrc. virtual StreamStatistician* GetStatistician(uint32_t ssrc) const = 0; - // Sets the max reordering threshold in number of packets. + // TODO(bugs.webrtc.org/10669): Deprecated, delete as soon as downstream + // projects are updated. This method sets the max reordering threshold of all + // current and future streams. virtual void SetMaxReorderingThreshold(int max_reordering_threshold) = 0; + + // Sets the max reordering threshold in number of packets. + virtual void SetMaxReorderingThreshold(uint32_t ssrc, + int max_reordering_threshold) = 0; // Detect retransmissions, enabling updates of the retransmitted counters. The // default is false. virtual void EnableRetransmitDetection(uint32_t ssrc, bool enable) = 0; diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index a224b1e010..0a9bc9667e 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -33,7 +33,6 @@ StreamStatistician::~StreamStatistician() {} StreamStatisticianImpl::StreamStatisticianImpl( uint32_t ssrc, Clock* clock, - bool enable_retransmit_detection, int max_reordering_threshold, RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback) @@ -42,7 +41,7 @@ StreamStatisticianImpl::StreamStatisticianImpl( incoming_bitrate_(kStatisticsProcessIntervalMs, RateStatistics::kBpsScale), max_reordering_threshold_(max_reordering_threshold), - enable_retransmit_detection_(enable_retransmit_detection), + enable_retransmit_detection_(false), jitter_q4_(0), cumulative_loss_(0), last_receive_time_ms_(0), @@ -368,48 +367,42 @@ ReceiveStatisticsImpl::~ReceiveStatisticsImpl() { } void ReceiveStatisticsImpl::OnRtpPacket(const RtpPacketReceived& packet) { - StreamStatisticianImpl* impl; - { - rtc::CritScope cs(&receive_statistics_lock_); - auto it = statisticians_.find(packet.Ssrc()); - if (it != statisticians_.end()) { - impl = it->second; - } else { - impl = new StreamStatisticianImpl( - packet.Ssrc(), clock_, /* enable_retransmit_detection = */ false, - max_reordering_threshold_, rtcp_stats_callback_, rtp_stats_callback_); - statisticians_[packet.Ssrc()] = impl; - } - } // StreamStatisticianImpl instance is created once and only destroyed when // this whole ReceiveStatisticsImpl is destroyed. StreamStatisticianImpl has // it's own locking so don't hold receive_statistics_lock_ (potential // deadlock). - impl->OnRtpPacket(packet); + GetOrCreateStatistician(packet.Ssrc())->OnRtpPacket(packet); } void ReceiveStatisticsImpl::FecPacketReceived(const RtpPacketReceived& packet) { - StreamStatisticianImpl* impl; - { - rtc::CritScope cs(&receive_statistics_lock_); - auto it = statisticians_.find(packet.Ssrc()); - // Ignore FEC if it is the first packet. - if (it == statisticians_.end()) - return; - impl = it->second; + StreamStatisticianImpl* impl = GetStatistician(packet.Ssrc()); + // Ignore FEC if it is the first packet. + if (impl) { + impl->FecPacketReceived(packet); } - impl->FecPacketReceived(packet); } -StreamStatistician* ReceiveStatisticsImpl::GetStatistician( +StreamStatisticianImpl* ReceiveStatisticsImpl::GetStatistician( uint32_t ssrc) const { rtc::CritScope cs(&receive_statistics_lock_); - auto it = statisticians_.find(ssrc); + const auto& it = statisticians_.find(ssrc); if (it == statisticians_.end()) return NULL; return it->second; } +StreamStatisticianImpl* ReceiveStatisticsImpl::GetOrCreateStatistician( + uint32_t ssrc) { + rtc::CritScope cs(&receive_statistics_lock_); + StreamStatisticianImpl*& impl = statisticians_[ssrc]; + if (impl == nullptr) { // new element + impl = + new StreamStatisticianImpl(ssrc, clock_, max_reordering_threshold_, + rtcp_stats_callback_, rtp_stats_callback_); + } + return impl; +} + void ReceiveStatisticsImpl::SetMaxReorderingThreshold( int max_reordering_threshold) { std::map statisticians; @@ -423,21 +416,16 @@ void ReceiveStatisticsImpl::SetMaxReorderingThreshold( } } +void ReceiveStatisticsImpl::SetMaxReorderingThreshold( + uint32_t ssrc, + int max_reordering_threshold) { + GetOrCreateStatistician(ssrc)->SetMaxReorderingThreshold( + max_reordering_threshold); +} + void ReceiveStatisticsImpl::EnableRetransmitDetection(uint32_t ssrc, bool enable) { - StreamStatisticianImpl* impl; - { - rtc::CritScope cs(&receive_statistics_lock_); - StreamStatisticianImpl*& impl_ref = statisticians_[ssrc]; - if (impl_ref == nullptr) { // new element - impl_ref = new StreamStatisticianImpl( - ssrc, clock_, enable, max_reordering_threshold_, rtcp_stats_callback_, - rtp_stats_callback_); - return; - } - impl = impl_ref; - } - impl->EnableRetransmitDetection(enable); + GetOrCreateStatistician(ssrc)->EnableRetransmitDetection(enable); } std::vector ReceiveStatisticsImpl::RtcpReportBlocks( diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index c153281d2b..3ab116044e 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -30,7 +30,6 @@ class StreamStatisticianImpl : public StreamStatistician, public: StreamStatisticianImpl(uint32_t ssrc, Clock* clock, - bool enable_retransmit_detection, int max_reordering_threshold, RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback); @@ -125,11 +124,16 @@ class ReceiveStatisticsImpl : public ReceiveStatistics { // Implements ReceiveStatistics. void FecPacketReceived(const RtpPacketReceived& packet) override; - StreamStatistician* GetStatistician(uint32_t ssrc) const override; + // Note: More specific return type for use in the implementation. + StreamStatisticianImpl* GetStatistician(uint32_t ssrc) const override; void SetMaxReorderingThreshold(int max_reordering_threshold) override; + void SetMaxReorderingThreshold(uint32_t ssrc, + int max_reordering_threshold) override; void EnableRetransmitDetection(uint32_t ssrc, bool enable) override; private: + StreamStatisticianImpl* GetOrCreateStatistician(uint32_t ssrc); + Clock* const clock_; rtc::CriticalSection receive_statistics_lock_; uint32_t last_returned_ssrc_; diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index 2c6dc3851a..7840b9755f 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -383,7 +383,7 @@ TEST_F(ReceiveStatisticsTest, LossComputationWithSequenceNumberWrapping) { TEST_F(ReceiveStatisticsTest, StreamRestartDoesntCountAsLoss) { RtcpStatistics statistics; - receive_statistics_->SetMaxReorderingThreshold(200); + receive_statistics_->SetMaxReorderingThreshold(kSsrc1, 200); packet1_.SetSequenceNumber(0); receive_statistics_->OnRtpPacket(packet1_); @@ -407,7 +407,7 @@ TEST_F(ReceiveStatisticsTest, StreamRestartDoesntCountAsLoss) { TEST_F(ReceiveStatisticsTest, CountsLossAfterStreamRestart) { RtcpStatistics statistics; - receive_statistics_->SetMaxReorderingThreshold(200); + receive_statistics_->SetMaxReorderingThreshold(kSsrc1, 200); packet1_.SetSequenceNumber(0); receive_statistics_->OnRtpPacket(packet1_); @@ -428,7 +428,7 @@ TEST_F(ReceiveStatisticsTest, CountsLossAfterStreamRestart) { TEST_F(ReceiveStatisticsTest, StreamCanRestartAtSequenceNumberWrapAround) { RtcpStatistics statistics; - receive_statistics_->SetMaxReorderingThreshold(200); + receive_statistics_->SetMaxReorderingThreshold(kSsrc1, 200); packet1_.SetSequenceNumber(0xffff - 401); receive_statistics_->OnRtpPacket(packet1_); @@ -449,7 +449,7 @@ TEST_F(ReceiveStatisticsTest, StreamCanRestartAtSequenceNumberWrapAround) { TEST_F(ReceiveStatisticsTest, StreamRestartNeedsTwoConsecutivePackets) { RtcpStatistics statistics; - receive_statistics_->SetMaxReorderingThreshold(200); + receive_statistics_->SetMaxReorderingThreshold(kSsrc1, 200); packet1_.SetSequenceNumber(400); receive_statistics_->OnRtpPacket(packet1_); @@ -494,7 +494,7 @@ TEST_F(ReceiveStatisticsTest, WrapsAroundExtendedHighestSequenceNumber) { EXPECT_EQ(0x10001u, statistics.extended_highest_sequence_number); // Receive a couple packets then wrap around again. - receive_statistics_->SetMaxReorderingThreshold(200); + receive_statistics_->SetMaxReorderingThreshold(kSsrc1, 200); for (int i = 10; i < 0xffff; i += 150) { packet1_.SetSequenceNumber(i); receive_statistics_->OnRtpPacket(packet1_); diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index e5e15c3dd3..641af8ae2c 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -148,8 +148,16 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( const int max_reordering_threshold = (config_.rtp.nack.rtp_history_ms > 0) ? kMaxPacketAgeToNack : kDefaultMaxReorderingThreshold; - rtp_receive_statistics_->SetMaxReorderingThreshold(max_reordering_threshold); - + rtp_receive_statistics_->SetMaxReorderingThreshold(config_.rtp.remote_ssrc, + max_reordering_threshold); + // TODO(nisse): For historic reasons, we applied the above + // max_reordering_threshold also for RTX stats, which makes little sense since + // we don't NACK rtx packets. Consider deleting the below block, and rely on + // the default threshold. + if (config_.rtp.rtx_ssrc) { + rtp_receive_statistics_->SetMaxReorderingThreshold( + config_.rtp.rtx_ssrc, max_reordering_threshold); + } if (config_.rtp.rtcp_xr.receiver_reference_time_report) rtp_rtcp_->SetRtcpXrRrtrStatus(true);