Pass callbacks for RtcpReceiver at construction
Bug: webrtc:10680
Change-Id: Ic242008e63a5a86ac30ab5f4041a30dbdb7fc72b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/170236
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30773}
diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc
index fba646e..cf9af9f 100644
--- a/call/rtp_video_sender.cc
+++ b/call/rtp_video_sender.cc
@@ -211,6 +211,9 @@
configuration.rtt_stats = observers.rtcp_rtt_stats;
configuration.rtcp_packet_type_counter_observer =
observers.rtcp_type_observer;
+ configuration.rtcp_statistics_callback = observers.rtcp_stats;
+ configuration.report_block_data_observer =
+ observers.report_block_data_observer;
configuration.paced_sender = transport->packet_sender();
configuration.send_bitrate_observer = observers.bitrate_observer;
configuration.send_side_delay_observer = observers.send_delay_observer;
@@ -400,9 +403,6 @@
for (const RtpStreamSender& stream : rtp_streams_) {
// Simulcast has one module for each layer. Set the CNAME on all modules.
stream.rtp_rtcp->SetCNAME(rtp_config_.c_name.c_str());
- stream.rtp_rtcp->RegisterRtcpStatisticsCallback(observers.rtcp_stats);
- stream.rtp_rtcp->SetReportBlockDataObserver(
- observers.report_block_data_observer);
stream.rtp_rtcp->SetMaxRtpPacketSize(rtp_config_.max_packet_size);
stream.rtp_rtcp->RegisterSendPayloadFrequency(rtp_config_.payload_type,
kVideoPayloadTypeFrequency);
diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h
index 8cee1ba..579b2df 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp.h
@@ -86,6 +86,16 @@
VideoBitrateAllocationObserver* bitrate_allocation_observer = nullptr;
RtcpRttStats* rtt_stats = nullptr;
RtcpPacketTypeCounterObserver* rtcp_packet_type_counter_observer = nullptr;
+ // Called on receipt of RTCP report block from remote side.
+ // TODO(bugs.webrtc.org/10678): Remove RtcpStatisticsCallback in
+ // favor of ReportBlockDataObserver.
+ // TODO(bugs.webrtc.org/10679): Consider whether we want to use
+ // only getters or only callbacks. If we decide on getters, the
+ // ReportBlockDataObserver should also be removed in favor of
+ // GetLatestReportBlockData().
+ RtcpStatisticsCallback* rtcp_statistics_callback = nullptr;
+ RtcpCnameCallback* rtcp_cname_callback = nullptr;
+ ReportBlockDataObserver* report_block_data_observer = nullptr;
// Estimates the bandwidth available for a set of streams from the same
// client.
@@ -417,24 +427,6 @@
// Returns true if the module is configured to store packets.
virtual bool StorePackets() const = 0;
- // Called on receipt of RTCP report block from remote side.
- // TODO(https://crbug.com/webrtc/10678): Remove RtcpStatisticsCallback in
- // favor of ReportBlockDataObserver.
- // TODO(https://crbug.com/webrtc/10679): Consider whether we want to use only
- // getters or only callbacks. If we decide on getters, the
- // ReportBlockDataObserver should also be removed in favor of
- // GetLatestReportBlockData().
- // TODO(nisse): Replace RegisterRtcpStatisticsCallback and
- // RegisterRtcpCnameCallback with construction-time settings in
- // RtpRtcp::Configuration.
- virtual void RegisterRtcpStatisticsCallback(
- RtcpStatisticsCallback* callback) = 0;
- virtual RtcpStatisticsCallback* GetRtcpStatisticsCallback() = 0;
- virtual void RegisterRtcpCnameCallback(RtcpCnameCallback* callback) = 0;
- // TODO(https://crbug.com/webrtc/10680): When callbacks are registered at
- // construction, remove this setter.
- virtual void SetReportBlockDataObserver(
- ReportBlockDataObserver* observer) = 0;
virtual void SetVideoBitrateAllocation(
const VideoBitrateAllocation& bitrate) = 0;
diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
index 8864df0..1927e4a 100644
--- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
+++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
@@ -149,10 +149,6 @@
MOCK_METHOD2(SetStorePacketsStatus,
void(bool enable, uint16_t number_to_store));
MOCK_CONST_METHOD0(StorePackets, bool());
- MOCK_METHOD1(RegisterRtcpStatisticsCallback, void(RtcpStatisticsCallback*));
- MOCK_METHOD0(GetRtcpStatisticsCallback, RtcpStatisticsCallback*());
- MOCK_METHOD1(RegisterRtcpCnameCallback, void(RtcpCnameCallback*));
- MOCK_METHOD1(SetReportBlockDataObserver, void(ReportBlockDataObserver*));
MOCK_METHOD1(SendFeedbackPacket, bool(const rtcp::TransportFeedback& packet));
MOCK_METHOD1(SendNetworkStateEstimatePacket,
bool(const rtcp::RemoteEstimate& packet));
diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc
index 68e86a2..2670429 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver.cc
@@ -161,9 +161,9 @@
oldest_tmmbr_info_ms_(0),
last_received_rb_ms_(0),
last_increased_sequence_number_ms_(0),
- stats_callback_(nullptr),
- cname_callback_(nullptr),
- report_block_data_observer_(nullptr),
+ stats_callback_(config.rtcp_statistics_callback),
+ cname_callback_(config.rtcp_cname_callback),
+ report_block_data_observer_(config.report_block_data_observer),
packet_type_counter_observer_(config.rtcp_packet_type_counter_observer),
num_skipped_packets_(0),
last_skipped_packets_warning_ms_(clock_->TimeInMilliseconds()) {
@@ -662,11 +662,8 @@
for (const rtcp::Sdes::Chunk& chunk : sdes.chunks()) {
received_cnames_[chunk.ssrc] = chunk.cname;
- {
- rtc::CritScope lock(&feedbacks_lock_);
- if (cname_callback_)
- cname_callback_->OnCname(chunk.ssrc, chunk.cname);
- }
+ if (cname_callback_)
+ cname_callback_->OnCname(chunk.ssrc, chunk.cname);
}
packet_information->packet_type_flags |= kRtcpSdes;
}
@@ -989,28 +986,6 @@
rtp_rtcp_->SetTmmbn(std::move(bounding));
}
-void RTCPReceiver::RegisterRtcpStatisticsCallback(
- RtcpStatisticsCallback* callback) {
- rtc::CritScope cs(&feedbacks_lock_);
- stats_callback_ = callback;
-}
-
-RtcpStatisticsCallback* RTCPReceiver::GetRtcpStatisticsCallback() {
- rtc::CritScope cs(&feedbacks_lock_);
- return stats_callback_;
-}
-
-void RTCPReceiver::RegisterRtcpCnameCallback(RtcpCnameCallback* callback) {
- rtc::CritScope cs(&feedbacks_lock_);
- cname_callback_ = callback;
-}
-
-void RTCPReceiver::SetReportBlockDataObserver(
- ReportBlockDataObserver* observer) {
- rtc::CritScope cs(&feedbacks_lock_);
- report_block_data_observer_ = observer;
-}
-
// Holding no Critical section.
void RTCPReceiver::TriggerCallbacksFromRtcpPacket(
const PacketInformation& packet_information) {
@@ -1114,7 +1089,6 @@
}
if (!receiver_only_) {
- rtc::CritScope cs(&feedbacks_lock_);
if (stats_callback_) {
for (const auto& report_block : packet_information.report_blocks) {
RtcpStatistics stats;
diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h
index 3af43b3..ef41476 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver.h
+++ b/modules/rtp_rtcp/source/rtcp_receiver.h
@@ -113,11 +113,6 @@
// Set new bandwidth and notify remote clients about it.
void NotifyTmmbrUpdated();
- void RegisterRtcpStatisticsCallback(RtcpStatisticsCallback* callback);
- void RegisterRtcpCnameCallback(RtcpCnameCallback* callback);
- RtcpStatisticsCallback* GetRtcpStatisticsCallback();
- void SetReportBlockDataObserver(ReportBlockDataObserver* observer);
-
private:
struct PacketInformation;
struct TmmbrInformation;
@@ -220,7 +215,6 @@
const uint32_t main_ssrc_;
const std::set<uint32_t> registered_ssrcs_;
- rtc::CriticalSection feedbacks_lock_;
RtcpBandwidthObserver* const rtcp_bandwidth_observer_;
RtcpIntraFrameObserver* const rtcp_intra_frame_observer_;
RtcpLossNotificationObserver* const rtcp_loss_notification_observer_;
@@ -267,13 +261,12 @@
// delivered RTP packet to the remote side.
int64_t last_increased_sequence_number_ms_;
- RtcpStatisticsCallback* stats_callback_ RTC_GUARDED_BY(feedbacks_lock_);
- RtcpCnameCallback* cname_callback_ RTC_GUARDED_BY(feedbacks_lock_);
+ RtcpStatisticsCallback* const stats_callback_;
+ RtcpCnameCallback* const cname_callback_;
// TODO(hbos): Remove RtcpStatisticsCallback in favor of
// ReportBlockDataObserver; the ReportBlockData contains a superset of the
// RtcpStatistics data.
- ReportBlockDataObserver* report_block_data_observer_
- RTC_GUARDED_BY(feedbacks_lock_);
+ ReportBlockDataObserver* const report_block_data_observer_;
RtcpPacketTypeCounterObserver* const packet_type_counter_observer_;
RtcpPacketTypeCounter packet_type_counter_;
diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
index 30caf7b..f952196 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
@@ -635,12 +635,13 @@
TEST(RtcpReceiverTest, InjectSdesWithOneChunk) {
ReceiverMocks mocks;
- RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl);
+ MockCnameCallbackImpl callback;
+ RtpRtcp::Configuration config = DefaultConfiguration(&mocks);
+ config.rtcp_cname_callback = &callback;
+ RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl);
receiver.SetRemoteSSRC(kSenderSsrc);
const char kCname[] = "alice@host";
- MockCnameCallbackImpl callback;
- receiver.RegisterRtcpCnameCallback(&callback);
rtcp::Sdes sdes;
sdes.AddCName(kSenderSsrc, kCname);
@@ -1308,11 +1309,11 @@
TEST(RtcpReceiverTest, Callbacks) {
ReceiverMocks mocks;
- RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl);
- receiver.SetRemoteSSRC(kSenderSsrc);
-
MockRtcpCallbackImpl callback;
- receiver.RegisterRtcpStatisticsCallback(&callback);
+ RtpRtcp::Configuration config = DefaultConfiguration(&mocks);
+ config.rtcp_statistics_callback = &callback;
+ RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl);
+ receiver.SetRemoteSSRC(kSenderSsrc);
const uint8_t kFractionLoss = 3;
const uint32_t kCumulativeLoss = 7;
@@ -1341,35 +1342,16 @@
EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks);
EXPECT_CALL(mocks.bandwidth_observer, OnReceivedRtcpReceiverReport);
receiver.IncomingPacket(rr1.Build());
-
- receiver.RegisterRtcpStatisticsCallback(nullptr);
-
- // Add arbitrary numbers, callback should not be called.
- rtcp::ReportBlock rb2;
- rb2.SetMediaSsrc(kReceiverMainSsrc);
- rb2.SetExtHighestSeqNum(kSequenceNumber + 1);
- rb2.SetFractionLost(42);
- rb2.SetCumulativeLost(137);
- rb2.SetJitter(4711);
-
- rtcp::ReceiverReport rr2;
- rr2.SetSenderSsrc(kSenderSsrc);
- rr2.AddReportBlock(rb2);
-
- EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks);
- EXPECT_CALL(mocks.bandwidth_observer, OnReceivedRtcpReceiverReport);
- EXPECT_CALL(callback, StatisticsUpdated).Times(0);
- receiver.IncomingPacket(rr2.Build());
}
TEST(RtcpReceiverTest,
VerifyBlockAndTimestampObtainedFromReportBlockDataObserver) {
ReceiverMocks mocks;
- RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl);
- receiver.SetRemoteSSRC(kSenderSsrc);
-
MockReportBlockDataObserverImpl observer;
- receiver.SetReportBlockDataObserver(&observer);
+ RtpRtcp::Configuration config = DefaultConfiguration(&mocks);
+ config.report_block_data_observer = &observer;
+ RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl);
+ receiver.SetRemoteSSRC(kSenderSsrc);
const uint8_t kFractionLoss = 3;
const uint32_t kCumulativeLoss = 7;
@@ -1414,11 +1396,11 @@
TEST(RtcpReceiverTest, VerifyRttObtainedFromReportBlockDataObserver) {
ReceiverMocks mocks;
- RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl);
- receiver.SetRemoteSSRC(kSenderSsrc);
-
MockReportBlockDataObserverImpl observer;
- receiver.SetReportBlockDataObserver(&observer);
+ RtpRtcp::Configuration config = DefaultConfiguration(&mocks);
+ config.report_block_data_observer = &observer;
+ RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl);
+ receiver.SetRemoteSSRC(kSenderSsrc);
const int64_t kRttMs = 120;
const uint32_t kDelayNtp = 123000;
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index ff30143..204bd8b 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -670,24 +670,6 @@
RtpPacketHistory::StorageMode::kDisabled;
}
-void ModuleRtpRtcpImpl::RegisterRtcpStatisticsCallback(
- RtcpStatisticsCallback* callback) {
- rtcp_receiver_.RegisterRtcpStatisticsCallback(callback);
-}
-
-RtcpStatisticsCallback* ModuleRtpRtcpImpl::GetRtcpStatisticsCallback() {
- return rtcp_receiver_.GetRtcpStatisticsCallback();
-}
-
-void ModuleRtpRtcpImpl::RegisterRtcpCnameCallback(RtcpCnameCallback* callback) {
- rtcp_receiver_.RegisterRtcpCnameCallback(callback);
-}
-
-void ModuleRtpRtcpImpl::SetReportBlockDataObserver(
- ReportBlockDataObserver* observer) {
- return rtcp_receiver_.SetReportBlockDataObserver(observer);
-}
-
void ModuleRtpRtcpImpl::SendCombinedRtcpPacket(
std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets) {
rtcp_sender_.SendCombinedRtcpPacket(std::move(rtcp_packets));
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
index 80488a8..17875e8 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -231,14 +231,6 @@
bool StorePackets() const override;
- // Called on receipt of RTCP report block from remote side.
- void RegisterRtcpStatisticsCallback(
- RtcpStatisticsCallback* callback) override;
- RtcpStatisticsCallback* GetRtcpStatisticsCallback() override;
- void RegisterRtcpCnameCallback(RtcpCnameCallback* callback) override;
-
- void SetReportBlockDataObserver(ReportBlockDataObserver* observer) override;
-
void SendCombinedRtcpPacket(
std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets) override;
diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index 85fb886..576da68 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -84,7 +84,7 @@
ReceiveStatistics* receive_statistics,
Transport* outgoing_transport,
RtcpRttStats* rtt_stats,
- RtcpPacketTypeCounterObserver* rtcp_packet_type_counter_observer,
+ ReceiveStatisticsProxy* rtcp_statistics_observer,
uint32_t local_ssrc) {
RtpRtcp::Configuration configuration;
configuration.clock = clock;
@@ -93,8 +93,8 @@
configuration.receive_statistics = receive_statistics;
configuration.outgoing_transport = outgoing_transport;
configuration.rtt_stats = rtt_stats;
- configuration.rtcp_packet_type_counter_observer =
- rtcp_packet_type_counter_observer;
+ configuration.rtcp_packet_type_counter_observer = rtcp_statistics_observer;
+ configuration.rtcp_cname_callback = rtcp_statistics_observer;
configuration.local_media_ssrc = local_ssrc;
std::unique_ptr<RtpRtcp> rtp_rtcp = RtpRtcp::Create(configuration);
@@ -256,9 +256,6 @@
if (config_.rtp.rtcp_xr.receiver_reference_time_report)
rtp_rtcp_->SetRtcpXrRrtrStatus(true);
- // Stats callback for CNAME changes.
- rtp_rtcp_->RegisterRtcpCnameCallback(receive_stats_proxy);
-
process_thread_->RegisterModule(rtp_rtcp_.get(), RTC_FROM_HERE);
if (config_.rtp.lntf.enabled) {