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}diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc
index f61f298..801c39e 100644
--- a/audio/channel_receive.cc
+++ b/audio/channel_receive.cc
@@ -521,8 +521,6 @@
media_transport_->SetReceiveAudioSink(nullptr);
}
- rtp_receive_statistics_->RegisterRtcpStatisticsCallback(NULL);
-
StopPlayout();
int error = audio_coding_->RegisterTransportCallback(NULL);
diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h
index ab68ee0..f905eb1 100644
--- a/modules/rtp_rtcp/include/receive_statistics.h
+++ b/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 @@
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 @@
// 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
diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc
index 30a0f36..0869d88 100644
--- a/modules/rtp_rtcp/source/receive_statistics_impl.cc
+++ b/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 @@
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 @@
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 @@
*statistics = CalculateRtcpStatistics();
}
- rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
+ if (rtcp_callback_)
+ rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
return true;
}
@@ -205,7 +209,8 @@
*statistics = CalculateRtcpStatistics();
}
- rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
+ if (rtcp_callback_)
+ rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
return true;
}
@@ -334,16 +339,23 @@
return time_diff_ms > rtp_time_stamp_diff_ms + max_delay_ms;
}
-ReceiveStatistics* ReceiveStatistics::Create(Clock* clock) {
- return new ReceiveStatisticsImpl(clock);
+std::unique_ptr<ReceiveStatistics> ReceiveStatistics::Create(
+ 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),
- rtp_stats_callback_(NULL) {}
+ rtcp_stats_callback_(rtcp_callback),
+ rtp_stats_callback_(rtp_callback) {}
ReceiveStatisticsImpl::~ReceiveStatisticsImpl() {
while (!statisticians_.empty()) {
@@ -362,7 +374,7 @@
} 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 @@
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 @@
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;
diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h
index f6aec69..2a9ea93 100644
--- a/modules/rtp_rtcp/source/receive_statistics_impl.h
+++ b/modules/rtp_rtcp/source/receive_statistics_impl.h
@@ -94,11 +94,11 @@
StreamDataCountersCallback* const rtp_callback_;
};
-class ReceiveStatisticsImpl : public ReceiveStatistics,
- public RtcpStatisticsCallback,
- public StreamDataCountersCallback {
+class ReceiveStatisticsImpl : public ReceiveStatistics {
public:
- explicit ReceiveStatisticsImpl(Clock* clock);
+ ReceiveStatisticsImpl(Clock* clock,
+ RtcpStatisticsCallback* rtcp_callback,
+ StreamDataCountersCallback* rtp_callback);
~ReceiveStatisticsImpl() override;
@@ -114,19 +114,7 @@
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 @@
std::map<uint32_t, StreamStatisticianImpl*> statisticians_
RTC_GUARDED_BY(receive_statistics_lock_);
- RtcpStatisticsCallback* rtcp_stats_callback_;
- StreamDataCountersCallback* rtp_stats_callback_;
+ RtcpStatisticsCallback* const rtcp_stats_callback_;
+ StreamDataCountersCallback* const rtp_stats_callback_;
};
} // namespace webrtc
#endif // MODULES_RTP_RTCP_SOURCE_RECEIVE_STATISTICS_IMPL_H_
diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc
index 578d81f..2539363 100644
--- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc
+++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc
@@ -71,7 +71,9 @@
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 @@
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 @@
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 @@
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 @@
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 =
diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index 3ebddf0..fdf02ff 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -117,8 +117,6 @@
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 "
diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc
index 4f2b9df..bba33aa 100644
--- a/video/video_receive_stream.cc
+++ b/video/video_receive_stream.cc
@@ -135,10 +135,11 @@
"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,
diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h
index e32aa19..5c9fd83 100644
--- a/video/video_receive_stream.h
+++ b/video/video_receive_stream.h
@@ -114,6 +114,7 @@
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 @@
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_;