Make ReceiveStatisticsImpl::SetMaxReorderingThreshold apply per ssrc

Bug: webrtc:10669
Change-Id: I9fec43fefe301b1e05eaea774a1453c93c4cc106
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138202
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28069}

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();
   }
 }

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;

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;
+  StreamStatisticianImpl* impl = GetStatistician(packet.Ssrc());
-  {
+  // Ignore FEC if it is the first packet.
-    rtc::CritScope cs(&receive_statistics_lock_);
+  if (impl) {
-    auto it = statisticians_.find(packet.Ssrc());
+    impl->FecPacketReceived(packet);
-    // Ignore FEC if it is the first packet.
-    if (it == statisticians_.end())
-      return;
-    impl = it->second;
   }
-  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<uint32_t, StreamStatisticianImpl*> 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;
+  GetOrCreateStatistician(ssrc)->EnableRetransmitDetection(enable);
-  {
-    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);
 }
 
 std::vector<rtcp::ReportBlock> ReceiveStatisticsImpl::RtcpReportBlocks(

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_;

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_);

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);