diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index fba646ef9f..cf9af9fefd 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -211,6 +211,9 @@ std::vector CreateRtpStreamSenders( 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 @@ RtpVideoSender::RtpVideoSender( 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 8cee1bae82..579b2dfd8e 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -86,6 +86,16 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { 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 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // 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 8864df01db..1927e4af4a 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -149,10 +149,6 @@ class MockRtpRtcp : public RtpRtcp { 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 68e86a22c5..2670429255 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -161,9 +161,9 @@ RTCPReceiver::RTCPReceiver(const RtpRtcp::Configuration& config, 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 @@ void RTCPReceiver::HandleSdes(const CommonHeader& rtcp_block, 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 @@ void RTCPReceiver::NotifyTmmbrUpdated() { 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 @@ void RTCPReceiver::TriggerCallbacksFromRtcpPacket( } 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 3af43b3e89..ef41476903 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -113,11 +113,6 @@ class RTCPReceiver final { // 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 @@ class RTCPReceiver final { const uint32_t main_ssrc_; const std::set 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 @@ class RTCPReceiver final { // 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 30caf7b63c..f95219674b 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, InjectApp) { 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, TmmbrThreeConstraintsTimeOut) { 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 @@ TEST(RtcpReceiverTest, Callbacks) { 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, 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 ff301433ae..204bd8b2a3 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -670,24 +670,6 @@ bool ModuleRtpRtcpImpl::StorePackets() const { 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> 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 80488a8e1f..17875e803f 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -231,14 +231,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { 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> rtcp_packets) override; diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 85fb8862f2..576da68f9e 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -84,7 +84,7 @@ std::unique_ptr CreateRtpRtcpModule( 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 @@ std::unique_ptr CreateRtpRtcpModule( 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 rtp_rtcp = RtpRtcp::Create(configuration); @@ -256,9 +256,6 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( 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) {