In ReceiveStatistic require callbacks during construction

Remove RegisterRtcpStatisticsCallback callback functions
saving taking an extra lock when calling callbacks.

Bug: None
Change-Id: Ib4537deffa0ab0abf597228e7c0fab7067614f6a
Reviewed-on: https://webrtc-review.googlesource.com/c/111821
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25779}

audio/channel_receive.cc

@@ -521,8 +521,6 @@ ChannelReceive::~ChannelReceive() {
     media_transport_->SetReceiveAudioSink(nullptr);
   }
 
-  rtp_receive_statistics_->RegisterRtcpStatisticsCallback(NULL);
-
   StopPlayout();
 
   int error = audio_coding_->RegisterTransportCallback(NULL);

modules/rtp_rtcp/include/receive_statistics.h

@@ -12,6 +12,7 @@
 #define MODULES_RTP_RTCP_INCLUDE_RECEIVE_STATISTICS_H_
 
 #include <map>
+#include <memory>
 #include <vector>
 
 #include "call/rtp_packet_sink_interface.h"
@@ -54,7 +55,14 @@ class ReceiveStatistics : public ReceiveStatisticsProvider,
  public:
   ~ReceiveStatistics() override = default;
 
-  static ReceiveStatistics* Create(Clock* clock);
+  static ReceiveStatistics* Create(Clock* clock) {
+    return Create(clock, nullptr, nullptr).release();
+  }
+
+  static std::unique_ptr<ReceiveStatistics> Create(
+      Clock* clock,
+      RtcpStatisticsCallback* rtcp_callback,
+      StreamDataCountersCallback* rtp_callback);
 
   // Increment counter for number of FEC packets received.
   virtual void FecPacketReceived(const RtpPacketReceived& packet) = 0;
@@ -67,14 +75,6 @@ class ReceiveStatistics : public ReceiveStatisticsProvider,
   // Detect retransmissions, enabling updates of the retransmitted counters. The
   // default is false.
   virtual void EnableRetransmitDetection(uint32_t ssrc, bool enable) = 0;
-
-  // Called on new RTCP stats creation.
-  virtual void RegisterRtcpStatisticsCallback(
-      RtcpStatisticsCallback* callback) = 0;
-
-  // Called on new RTP stats creation.
-  virtual void RegisterRtpStatisticsCallback(
-      StreamDataCountersCallback* callback) = 0;
 };
 
 }  // namespace webrtc

modules/rtp_rtcp/source/receive_statistics_impl.cc

@@ -11,10 +11,11 @@
 #include "modules/rtp_rtcp/source/receive_statistics_impl.h"
 
 #include <math.h>
-
 #include <cstdlib>
+#include <memory>
 #include <vector>
 
+#include "absl/memory/memory.h"
 #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h"
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "modules/rtp_rtcp/source/rtp_rtcp_config.h"
@@ -59,7 +60,8 @@ StreamStatisticianImpl::~StreamStatisticianImpl() = default;
 
 void StreamStatisticianImpl::OnRtpPacket(const RtpPacketReceived& packet) {
   StreamDataCounters counters = UpdateCounters(packet);
-  rtp_callback_->DataCountersUpdated(counters, ssrc_);
+  if (rtp_callback_)
+    rtp_callback_->DataCountersUpdated(counters, ssrc_);
 }
 
 StreamDataCounters StreamStatisticianImpl::UpdateCounters(
@@ -146,7 +148,8 @@ void StreamStatisticianImpl::FecPacketReceived(
     receive_counters_.fec.AddPacket(packet);
     counters = receive_counters_;
   }
-  rtp_callback_->DataCountersUpdated(counters, ssrc_);
+  if (rtp_callback_)
+    rtp_callback_->DataCountersUpdated(counters, ssrc_);
 }
 
 void StreamStatisticianImpl::SetMaxReorderingThreshold(
@@ -183,7 +186,8 @@ bool StreamStatisticianImpl::GetStatistics(RtcpStatistics* statistics,
     *statistics = CalculateRtcpStatistics();
   }
 
-  rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
+  if (rtcp_callback_)
+    rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
   return true;
 }
 
@@ -205,7 +209,8 @@ bool StreamStatisticianImpl::GetActiveStatisticsAndReset(
     *statistics = CalculateRtcpStatistics();
   }
 
-  rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
+  if (rtcp_callback_)
+    rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
   return true;
 }
 
@@ -334,16 +339,23 @@ bool StreamStatisticianImpl::IsRetransmitOfOldPacket(
   return time_diff_ms > rtp_time_stamp_diff_ms + max_delay_ms;
 }
 
-ReceiveStatistics* ReceiveStatistics::Create(Clock* clock) {
+std::unique_ptr<ReceiveStatistics> ReceiveStatistics::Create(
-  return new ReceiveStatisticsImpl(clock);
+    Clock* clock,
+    RtcpStatisticsCallback* rtcp_callback,
+    StreamDataCountersCallback* rtp_callback) {
+  return absl::make_unique<ReceiveStatisticsImpl>(clock, rtcp_callback,
+                                                  rtp_callback);
 }
 
-ReceiveStatisticsImpl::ReceiveStatisticsImpl(Clock* clock)
+ReceiveStatisticsImpl::ReceiveStatisticsImpl(
+    Clock* clock,
+    RtcpStatisticsCallback* rtcp_callback,
+    StreamDataCountersCallback* rtp_callback)
     : clock_(clock),
       last_returned_ssrc_(0),
       max_reordering_threshold_(kDefaultMaxReorderingThreshold),
-      rtcp_stats_callback_(NULL),
+      rtcp_stats_callback_(rtcp_callback),
-      rtp_stats_callback_(NULL) {}
+      rtp_stats_callback_(rtp_callback) {}
 
 ReceiveStatisticsImpl::~ReceiveStatisticsImpl() {
   while (!statisticians_.empty()) {
@@ -362,7 +374,7 @@ void ReceiveStatisticsImpl::OnRtpPacket(const RtpPacketReceived& packet) {
     } else {
       impl = new StreamStatisticianImpl(
           packet.Ssrc(), clock_, /* enable_retransmit_detection = */ false,
-          max_reordering_threshold_, this, this);
+          max_reordering_threshold_, rtcp_stats_callback_, rtp_stats_callback_);
       statisticians_[packet.Ssrc()] = impl;
     }
   }
@@ -416,7 +428,8 @@ void ReceiveStatisticsImpl::EnableRetransmitDetection(uint32_t ssrc,
     StreamStatisticianImpl*& impl_ref = statisticians_[ssrc];
     if (impl_ref == nullptr) {  // new element
       impl_ref = new StreamStatisticianImpl(
-          ssrc, clock_, enable, max_reordering_threshold_, this, this);
+          ssrc, clock_, enable, max_reordering_threshold_, rtcp_stats_callback_,
+          rtp_stats_callback_);
       return;
     }
     impl = impl_ref;
@@ -424,43 +437,6 @@ void ReceiveStatisticsImpl::EnableRetransmitDetection(uint32_t ssrc,
   impl->EnableRetransmitDetection(enable);
 }
 
-void ReceiveStatisticsImpl::RegisterRtcpStatisticsCallback(
-    RtcpStatisticsCallback* callback) {
-  rtc::CritScope cs(&receive_statistics_lock_);
-  if (callback != NULL)
-    assert(rtcp_stats_callback_ == NULL);
-  rtcp_stats_callback_ = callback;
-}
-
-void ReceiveStatisticsImpl::StatisticsUpdated(const RtcpStatistics& statistics,
-                                              uint32_t ssrc) {
-  rtc::CritScope cs(&receive_statistics_lock_);
-  if (rtcp_stats_callback_)
-    rtcp_stats_callback_->StatisticsUpdated(statistics, ssrc);
-}
-
-void ReceiveStatisticsImpl::CNameChanged(const char* cname, uint32_t ssrc) {
-  rtc::CritScope cs(&receive_statistics_lock_);
-  if (rtcp_stats_callback_)
-    rtcp_stats_callback_->CNameChanged(cname, ssrc);
-}
-
-void ReceiveStatisticsImpl::RegisterRtpStatisticsCallback(
-    StreamDataCountersCallback* callback) {
-  rtc::CritScope cs(&receive_statistics_lock_);
-  if (callback != NULL)
-    assert(rtp_stats_callback_ == NULL);
-  rtp_stats_callback_ = callback;
-}
-
-void ReceiveStatisticsImpl::DataCountersUpdated(const StreamDataCounters& stats,
-                                                uint32_t ssrc) {
-  rtc::CritScope cs(&receive_statistics_lock_);
-  if (rtp_stats_callback_) {
-    rtp_stats_callback_->DataCountersUpdated(stats, ssrc);
-  }
-}
-
 std::vector<rtcp::ReportBlock> ReceiveStatisticsImpl::RtcpReportBlocks(
     size_t max_blocks) {
   std::map<uint32_t, StreamStatisticianImpl*> statisticians;

modules/rtp_rtcp/source/receive_statistics_impl.h

@@ -94,11 +94,11 @@ class StreamStatisticianImpl : public StreamStatistician,
   StreamDataCountersCallback* const rtp_callback_;
 };
 
-class ReceiveStatisticsImpl : public ReceiveStatistics,
+class ReceiveStatisticsImpl : public ReceiveStatistics {
-                              public RtcpStatisticsCallback,
-                              public StreamDataCountersCallback {
  public:
-  explicit ReceiveStatisticsImpl(Clock* clock);
+  ReceiveStatisticsImpl(Clock* clock,
+                        RtcpStatisticsCallback* rtcp_callback,
+                        StreamDataCountersCallback* rtp_callback);
 
   ~ReceiveStatisticsImpl() override;
 
@@ -114,19 +114,7 @@ class ReceiveStatisticsImpl : public ReceiveStatistics,
   void SetMaxReorderingThreshold(int max_reordering_threshold) override;
   void EnableRetransmitDetection(uint32_t ssrc, bool enable) override;
 
-  void RegisterRtcpStatisticsCallback(
-      RtcpStatisticsCallback* callback) override;
-
-  void RegisterRtpStatisticsCallback(
-      StreamDataCountersCallback* callback) override;
-
  private:
-  void StatisticsUpdated(const RtcpStatistics& statistics,
-                         uint32_t ssrc) override;
-  void CNameChanged(const char* cname, uint32_t ssrc) override;
-  void DataCountersUpdated(const StreamDataCounters& counters,
-                           uint32_t ssrc) override;
-
   Clock* const clock_;
   rtc::CriticalSection receive_statistics_lock_;
   uint32_t last_returned_ssrc_;
@@ -134,8 +122,8 @@ class ReceiveStatisticsImpl : public ReceiveStatistics,
   std::map<uint32_t, StreamStatisticianImpl*> statisticians_
       RTC_GUARDED_BY(receive_statistics_lock_);
 
-  RtcpStatisticsCallback* rtcp_stats_callback_;
+  RtcpStatisticsCallback* const rtcp_stats_callback_;
-  StreamDataCountersCallback* rtp_stats_callback_;
+  StreamDataCountersCallback* const rtp_stats_callback_;
 };
 }  // namespace webrtc
 #endif  // MODULES_RTP_RTCP_SOURCE_RECEIVE_STATISTICS_IMPL_H_

modules/rtp_rtcp/source/receive_statistics_unittest.cc

@@ -71,7 +71,9 @@ void IncrementTimestamp(RtpPacketReceived* packet, uint32_t incr) {
 class ReceiveStatisticsTest : public ::testing::Test {
  public:
   ReceiveStatisticsTest()
-      : clock_(0), receive_statistics_(ReceiveStatistics::Create(&clock_)) {
+      : clock_(0),
+        receive_statistics_(
+            ReceiveStatistics::Create(&clock_, nullptr, nullptr)) {
     packet1_ = CreateRtpPacket(kSsrc1, kPacketSize1);
     packet2_ = CreateRtpPacket(kSsrc2, kPacketSize2);
   }
@@ -251,7 +253,7 @@ TEST_F(ReceiveStatisticsTest, RtcpCallbacks) {
     RtcpStatistics stats_;
   } callback;
 
-  receive_statistics_->RegisterRtcpStatisticsCallback(&callback);
+  receive_statistics_ = ReceiveStatistics::Create(&clock_, &callback, nullptr);
   receive_statistics_->EnableRetransmitDetection(kSsrc1, true);
 
   // Add some arbitrary data, with loss and jitter.
@@ -291,33 +293,6 @@ TEST_F(ReceiveStatisticsTest, RtcpCallbacks) {
   EXPECT_EQ(1, statistics.packets_lost);
   EXPECT_EQ(5u, statistics.extended_highest_sequence_number);
   EXPECT_EQ(177u, statistics.jitter);
-
-  receive_statistics_->RegisterRtcpStatisticsCallback(NULL);
-
-  // Add some more data.
-  packet1_.SetSequenceNumber(1);
-  clock_.AdvanceTimeMilliseconds(7);
-  IncrementTimestamp(&packet1_, 3);
-  receive_statistics_->OnRtpPacket(packet1_);
-  IncrementSequenceNumber(&packet1_, 2);
-  clock_.AdvanceTimeMilliseconds(9);
-  IncrementTimestamp(&packet1_, 9);
-  receive_statistics_->OnRtpPacket(packet1_);
-  IncrementSequenceNumber(&packet1_, -1);
-  clock_.AdvanceTimeMilliseconds(13);
-  IncrementTimestamp(&packet1_, 47);
-  receive_statistics_->OnRtpPacket(packet1_);
-  IncrementSequenceNumber(&packet1_, 3);
-  clock_.AdvanceTimeMilliseconds(11);
-  IncrementTimestamp(&packet1_, 17);
-  receive_statistics_->OnRtpPacket(packet1_);
-  IncrementSequenceNumber(&packet1_);
-
-  receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics,
-                                                              true);
-
-  // Should not have been called after deregister.
-  EXPECT_EQ(1u, callback.num_calls_);
 }
 
 class RtpTestCallback : public StreamDataCountersCallback {
@@ -358,7 +333,7 @@ class RtpTestCallback : public StreamDataCountersCallback {
 
 TEST_F(ReceiveStatisticsTest, RtpCallbacks) {
   RtpTestCallback callback;
-  receive_statistics_->RegisterRtpStatisticsCallback(&callback);
+  receive_statistics_ = ReceiveStatistics::Create(&clock_, nullptr, &callback);
   receive_statistics_->EnableRetransmitDetection(kSsrc1, true);
 
   const size_t kHeaderLength = 20;
@@ -417,19 +392,11 @@ TEST_F(ReceiveStatisticsTest, RtpCallbacks) {
   expected.fec.header_bytes = kHeaderLength;
   expected.fec.packets = 1;
   callback.Matches(5, kSsrc1, expected);
-
-  receive_statistics_->RegisterRtpStatisticsCallback(NULL);
-
-  // New stats, but callback should not be called.
-  IncrementSequenceNumber(&packet1);
-  clock_.AdvanceTimeMilliseconds(5);
-  receive_statistics_->OnRtpPacket(packet1);
-  callback.Matches(5, kSsrc1, expected);
 }
 
 TEST_F(ReceiveStatisticsTest, RtpCallbacksFecFirst) {
   RtpTestCallback callback;
-  receive_statistics_->RegisterRtpStatisticsCallback(&callback);
+  receive_statistics_ = ReceiveStatistics::Create(&clock_, nullptr, &callback);
 
   const uint32_t kHeaderLength = 20;
   RtpPacketReceived packet =

video/rtp_video_stream_receiver.cc

@@ -117,8 +117,6 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
       frame_decryptor_(frame_decryptor) {
   constexpr bool remb_candidate = true;
   packet_router_->AddReceiveRtpModule(rtp_rtcp_.get(), remb_candidate);
-  rtp_receive_statistics_->RegisterRtpStatisticsCallback(receive_stats_proxy);
-  rtp_receive_statistics_->RegisterRtcpStatisticsCallback(receive_stats_proxy);
 
   RTC_DCHECK(config_.rtp.rtcp_mode != RtcpMode::kOff)
       << "A stream should not be configured with RTCP disabled. This value is "

video/video_receive_stream.cc

@@ -135,10 +135,11 @@ VideoReceiveStream::VideoReceiveStream(
                      "DecodingThread",
                      rtc::kHighestPriority),
       call_stats_(call_stats),
-      rtp_receive_statistics_(ReceiveStatistics::Create(clock_)),
+      stats_proxy_(&config_, clock_),
+      rtp_receive_statistics_(
+          ReceiveStatistics::Create(clock_, &stats_proxy_, &stats_proxy_)),
       timing_(new VCMTiming(clock_)),
       video_receiver_(clock_, timing_.get(), this, this),
-      stats_proxy_(&config_, clock_),
       rtp_video_stream_receiver_(&transport_adapter_,
                                  call_stats,
                                  packet_router,

video/video_receive_stream.h

@@ -114,6 +114,7 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
 
   CallStats* const call_stats_;
 
+  ReceiveStatisticsProxy stats_proxy_;
   // Shared by media and rtx stream receivers, since the latter has no RtpRtcp
   // module of its own.
   const std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
@@ -121,7 +122,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
   std::unique_ptr<VCMTiming> timing_;  // Jitter buffer experiment.
   vcm::VideoReceiver video_receiver_;
   std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> incoming_video_stream_;
-  ReceiveStatisticsProxy stats_proxy_;
   RtpVideoStreamReceiver rtp_video_stream_receiver_;
   std::unique_ptr<VideoStreamDecoder> video_stream_decoder_;
   RtpStreamsSynchronizer rtp_stream_sync_;