From f01c2c96f21378b57c6fdf166f36cd70d1e6404d Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 14 May 2021 15:39:23 +0200 Subject: [PATCH] Delete RtcpStatisticsCallback in favor of ReportBlockDataObserver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:10678 Change-Id: Ie016cbc47dbba15176fc5e7ad7d01a438db7dfb3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/218842 Reviewed-by: Erik Språng Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#34013} --- .../rtp_transport_controller_send_interface.h | 1 - call/rtp_video_sender.cc | 1 - call/rtp_video_sender_unittest.cc | 7 +- modules/rtp_rtcp/include/rtcp_statistics.h | 10 +-- modules/rtp_rtcp/source/rtcp_receiver.cc | 13 ---- modules/rtp_rtcp/source/rtcp_receiver.h | 4 -- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 45 ------------ modules/rtp_rtcp/source/rtp_rtcp_interface.h | 3 - video/send_statistics_proxy.cc | 20 +++--- video/send_statistics_proxy.h | 4 -- video/send_statistics_proxy_unittest.cc | 70 +++++++++++++------ video/video_send_stream_impl.cc | 1 - 12 files changed, 62 insertions(+), 117 deletions(-) diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index 59263447a4..2aa6d739da 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -52,7 +52,6 @@ struct RtpSenderObservers { RtcpRttStats* rtcp_rtt_stats; RtcpIntraFrameObserver* intra_frame_callback; RtcpLossNotificationObserver* rtcp_loss_notification_observer; - RtcpStatisticsCallback* rtcp_stats; ReportBlockDataObserver* report_block_data_observer; StreamDataCountersCallback* rtp_stats; BitrateStatisticsObserver* bitrate_observer; diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 022c97dc3c..c2a6a564f4 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -216,7 +216,6 @@ 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(); diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 88cbeedbab..85934cc7ed 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -61,7 +61,6 @@ class MockRtcpIntraFrameObserver : public RtcpIntraFrameObserver { RtpSenderObservers CreateObservers( RtcpRttStats* rtcp_rtt_stats, RtcpIntraFrameObserver* intra_frame_callback, - RtcpStatisticsCallback* rtcp_stats, ReportBlockDataObserver* report_block_data_observer, StreamDataCountersCallback* rtp_stats, BitrateStatisticsObserver* bitrate_observer, @@ -73,7 +72,6 @@ RtpSenderObservers CreateObservers( observers.rtcp_rtt_stats = rtcp_rtt_stats; observers.intra_frame_callback = intra_frame_callback; observers.rtcp_loss_notification_observer = nullptr; - observers.rtcp_stats = rtcp_stats; observers.report_block_data_observer = report_block_data_observer; observers.rtp_stats = rtp_stats; observers.bitrate_observer = bitrate_observer; @@ -147,9 +145,8 @@ class RtpVideoSenderTestFixture { time_controller_.GetClock(), suspended_ssrcs, suspended_payload_states, config_.rtp, config_.rtcp_report_interval_ms, &transport_, CreateObservers(nullptr, &encoder_feedback_, &stats_proxy_, - &stats_proxy_, &stats_proxy_, &stats_proxy_, - frame_count_observer, &stats_proxy_, &stats_proxy_, - &send_delay_stats_), + &stats_proxy_, &stats_proxy_, frame_count_observer, + &stats_proxy_, &stats_proxy_, &send_delay_stats_), &transport_controller_, &event_log_, &retransmission_rate_limiter_, std::make_unique(time_controller_.GetClock()), nullptr, CryptoOptions{}, frame_transformer); diff --git a/modules/rtp_rtcp/include/rtcp_statistics.h b/modules/rtp_rtcp/include/rtcp_statistics.h index e26c475e31..8a633241c4 100644 --- a/modules/rtp_rtcp/include/rtcp_statistics.h +++ b/modules/rtp_rtcp/include/rtcp_statistics.h @@ -18,6 +18,8 @@ namespace webrtc { // Statistics for an RTCP channel +// TODO(bugs.webrtc.org/10678): Remove remaining usages of this struct in favor +// of RTCPReportBlock, rtcp::ReportBlock or other similar structs. struct RtcpStatistics { uint8_t fraction_lost = 0; int32_t packets_lost = 0; // Defined as a 24 bit signed integer in RTCP @@ -25,14 +27,6 @@ struct RtcpStatistics { uint32_t jitter = 0; }; -class RtcpStatisticsCallback { - public: - virtual ~RtcpStatisticsCallback() {} - - virtual void StatisticsUpdated(const RtcpStatistics& statistics, - uint32_t ssrc) = 0; -}; - // Statistics for RTCP packet types. struct RtcpPacketTypeCounter { RtcpPacketTypeCounter() diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 104b61b935..526acf555e 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -142,7 +142,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, xr_rrtr_status_(config.non_sender_rtt_measurement), xr_rr_rtt_ms_(0), oldest_tmmbr_info_ms_(0), - 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), @@ -1105,18 +1104,6 @@ void RTCPReceiver::TriggerCallbacksFromRtcpPacket( } if (!receiver_only_) { - if (stats_callback_) { - for (const auto& report_block : packet_information.report_blocks) { - RtcpStatistics stats; - stats.packets_lost = report_block.packets_lost; - stats.extended_highest_sequence_number = - report_block.extended_highest_sequence_number; - stats.fraction_lost = report_block.fraction_lost; - stats.jitter = report_block.jitter; - - stats_callback_->StatisticsUpdated(stats, report_block.source_ssrc); - } - } if (report_block_data_observer_) { for (const auto& report_block_data : packet_information.report_block_datas) { diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index 370a875d37..429df55d49 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -340,11 +340,7 @@ class RTCPReceiver final { // delivered RTP packet to the remote side. Timestamp last_increased_sequence_number_ = Timestamp::PlusInfinity(); - 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* const report_block_data_observer_; RtcpPacketTypeCounterObserver* const packet_type_counter_observer_; diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 068a110771..c71bbcc9a8 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -87,14 +87,6 @@ class MockRtcpLossNotificationObserver : public RtcpLossNotificationObserver { (override)); }; -class MockRtcpCallbackImpl : public RtcpStatisticsCallback { - public: - MOCK_METHOD(void, - StatisticsUpdated, - (const RtcpStatistics&, uint32_t), - (override)); -}; - class MockCnameCallbackImpl : public RtcpCnameCallback { public: MOCK_METHOD(void, OnCname, (uint32_t, absl::string_view), (override)); @@ -1280,43 +1272,6 @@ TEST(RtcpReceiverTest, TmmbrThreeConstraintsTimeOut) { Property(&rtcp::TmmbItem::ssrc, Eq(kSenderSsrc + 2)))); } -TEST(RtcpReceiverTest, Callbacks) { - ReceiverMocks mocks; - MockRtcpCallbackImpl callback; - RtpRtcpInterface::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; - const uint32_t kJitter = 9; - const uint16_t kSequenceNumber = 1234; - - // First packet, all numbers should just propagate. - rtcp::ReportBlock rb1; - rb1.SetMediaSsrc(kReceiverMainSsrc); - rb1.SetExtHighestSeqNum(kSequenceNumber); - rb1.SetFractionLost(kFractionLoss); - rb1.SetCumulativeLost(kCumulativeLoss); - rb1.SetJitter(kJitter); - - rtcp::ReceiverReport rr1; - rr1.SetSenderSsrc(kSenderSsrc); - rr1.AddReportBlock(rb1); - EXPECT_CALL(callback, - StatisticsUpdated( - AllOf(Field(&RtcpStatistics::fraction_lost, kFractionLoss), - Field(&RtcpStatistics::packets_lost, kCumulativeLoss), - Field(&RtcpStatistics::extended_highest_sequence_number, - kSequenceNumber), - Field(&RtcpStatistics::jitter, kJitter)), - kReceiverMainSsrc)); - EXPECT_CALL(mocks.rtp_rtcp_impl, OnReceivedRtcpReportBlocks); - EXPECT_CALL(mocks.bandwidth_observer, OnReceivedRtcpReceiverReport); - receiver.IncomingPacket(rr1.Build()); -} - TEST(RtcpReceiverTest, VerifyBlockAndTimestampObtainedFromReportBlockDataObserver) { ReceiverMocks mocks; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 5ab48c9ad4..457a993139 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -77,13 +77,10 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { 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; diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc index 6752abe11c..971e42f0e0 100644 --- a/video/send_statistics_proxy.cc +++ b/video/send_statistics_proxy.cc @@ -1285,17 +1285,6 @@ void SendStatisticsProxy::RtcpPacketTypesCounterUpdated( uma_container_->first_rtcp_stats_time_ms_ = clock_->TimeInMilliseconds(); } -void SendStatisticsProxy::StatisticsUpdated(const RtcpStatistics& statistics, - uint32_t ssrc) { - MutexLock lock(&mutex_); - VideoSendStream::StreamStats* stats = GetStatsEntry(ssrc); - if (!stats) - return; - - stats->rtcp_stats = statistics; - uma_container_->report_block_stats_.Store(ssrc, statistics); -} - void SendStatisticsProxy::OnReportBlockDataUpdated( ReportBlockData report_block_data) { MutexLock lock(&mutex_); @@ -1303,6 +1292,15 @@ void SendStatisticsProxy::OnReportBlockDataUpdated( GetStatsEntry(report_block_data.report_block().source_ssrc); if (!stats) return; + const RTCPReportBlock& report_block = report_block_data.report_block(); + stats->rtcp_stats.fraction_lost = report_block.fraction_lost; + stats->rtcp_stats.packets_lost = report_block.packets_lost; + stats->rtcp_stats.extended_highest_sequence_number = + report_block.extended_highest_sequence_number; + stats->rtcp_stats.jitter = report_block.jitter; + uma_container_->report_block_stats_.Store(report_block.source_ssrc, + stats->rtcp_stats); + stats->report_block_data = std::move(report_block_data); } diff --git a/video/send_statistics_proxy.h b/video/send_statistics_proxy.h index 0de7df290e..bfb221f65c 100644 --- a/video/send_statistics_proxy.h +++ b/video/send_statistics_proxy.h @@ -37,7 +37,6 @@ namespace webrtc { class SendStatisticsProxy : public VideoStreamEncoderObserver, - public RtcpStatisticsCallback, public ReportBlockDataObserver, public RtcpPacketTypeCounterObserver, public StreamDataCountersCallback, @@ -106,9 +105,6 @@ class SendStatisticsProxy : public VideoStreamEncoderObserver, int GetSendFrameRate() const; protected: - // From RtcpStatisticsCallback. - void StatisticsUpdated(const RtcpStatistics& statistics, - uint32_t ssrc) override; // From ReportBlockDataObserver. void OnReportBlockDataUpdated(ReportBlockData report_block_data) override; // From RtcpPacketTypeCounterObserver. diff --git a/video/send_statistics_proxy_unittest.cc b/video/send_statistics_proxy_unittest.cc index 71b84c9443..aa133022a4 100644 --- a/video/send_statistics_proxy_unittest.cc +++ b/video/send_statistics_proxy_unittest.cc @@ -174,29 +174,51 @@ class SendStatisticsProxyTest : public ::testing::Test { VideoSendStream::Stats expected_; }; -TEST_F(SendStatisticsProxyTest, RtcpStatistics) { - RtcpStatisticsCallback* callback = statistics_proxy_.get(); - for (const auto& ssrc : config_.rtp.ssrcs) { +TEST_F(SendStatisticsProxyTest, ReportBlockDataObserver) { + ReportBlockDataObserver* callback = statistics_proxy_.get(); + for (uint32_t ssrc : config_.rtp.ssrcs) { VideoSendStream::StreamStats& ssrc_stats = expected_.substreams[ssrc]; // Add statistics with some arbitrary, but unique, numbers. uint32_t offset = ssrc * sizeof(RtcpStatistics); - ssrc_stats.rtcp_stats.packets_lost = offset; - ssrc_stats.rtcp_stats.extended_highest_sequence_number = offset + 1; - ssrc_stats.rtcp_stats.fraction_lost = offset + 2; - ssrc_stats.rtcp_stats.jitter = offset + 3; - callback->StatisticsUpdated(ssrc_stats.rtcp_stats, ssrc); + RTCPReportBlock report_block; + report_block.source_ssrc = ssrc; + report_block.packets_lost = offset; + report_block.extended_highest_sequence_number = offset + 1; + report_block.fraction_lost = offset + 2; + report_block.jitter = offset + 3; + + ssrc_stats.rtcp_stats.packets_lost = report_block.packets_lost; + ssrc_stats.rtcp_stats.extended_highest_sequence_number = + report_block.extended_highest_sequence_number; + ssrc_stats.rtcp_stats.fraction_lost = report_block.fraction_lost; + ssrc_stats.rtcp_stats.jitter = report_block.jitter; + ReportBlockData data; + data.SetReportBlock(report_block, 0); + + callback->OnReportBlockDataUpdated(data); } - for (const auto& ssrc : config_.rtp.rtx.ssrcs) { + for (uint32_t ssrc : config_.rtp.rtx.ssrcs) { VideoSendStream::StreamStats& ssrc_stats = expected_.substreams[ssrc]; // Add statistics with some arbitrary, but unique, numbers. uint32_t offset = ssrc * sizeof(RtcpStatistics); - ssrc_stats.rtcp_stats.packets_lost = offset; - ssrc_stats.rtcp_stats.extended_highest_sequence_number = offset + 1; - ssrc_stats.rtcp_stats.fraction_lost = offset + 2; - ssrc_stats.rtcp_stats.jitter = offset + 3; - callback->StatisticsUpdated(ssrc_stats.rtcp_stats, ssrc); + RTCPReportBlock report_block; + report_block.source_ssrc = ssrc; + report_block.packets_lost = offset; + report_block.extended_highest_sequence_number = offset + 1; + report_block.fraction_lost = offset + 2; + report_block.jitter = offset + 3; + + ssrc_stats.rtcp_stats.packets_lost = report_block.packets_lost; + ssrc_stats.rtcp_stats.extended_highest_sequence_number = + report_block.extended_highest_sequence_number; + ssrc_stats.rtcp_stats.fraction_lost = report_block.fraction_lost; + ssrc_stats.rtcp_stats.jitter = report_block.jitter; + ReportBlockData data; + data.SetReportBlock(report_block, 0); + + callback->OnReportBlockDataUpdated(data); } VideoSendStream::Stats stats = statistics_proxy_->GetStats(); ExpectEqual(expected_, stats); @@ -2171,10 +2193,13 @@ TEST_F(SendStatisticsProxyTest, NoSubstreams) { std::max(*absl::c_max_element(config_.rtp.ssrcs), *absl::c_max_element(config_.rtp.rtx.ssrcs)) + 1; - // From RtcpStatisticsCallback. - RtcpStatistics rtcp_stats; - RtcpStatisticsCallback* rtcp_callback = statistics_proxy_.get(); - rtcp_callback->StatisticsUpdated(rtcp_stats, excluded_ssrc); + // From ReportBlockDataObserver. + ReportBlockDataObserver* rtcp_callback = statistics_proxy_.get(); + RTCPReportBlock report_block; + report_block.source_ssrc = excluded_ssrc; + ReportBlockData data; + data.SetReportBlock(report_block, 0); + rtcp_callback->OnReportBlockDataUpdated(data); // From BitrateStatisticsObserver. uint32_t total = 0; @@ -2221,9 +2246,12 @@ TEST_F(SendStatisticsProxyTest, EncodedResolutionTimesOut) { // Update the first SSRC with bogus RTCP stats to make sure that encoded // resolution still times out (no global timeout for all stats). - RtcpStatistics rtcp_statistics; - RtcpStatisticsCallback* rtcp_stats = statistics_proxy_.get(); - rtcp_stats->StatisticsUpdated(rtcp_statistics, config_.rtp.ssrcs[0]); + ReportBlockDataObserver* rtcp_callback = statistics_proxy_.get(); + RTCPReportBlock report_block; + report_block.source_ssrc = config_.rtp.ssrcs[0]; + ReportBlockData data; + data.SetReportBlock(report_block, 0); + rtcp_callback->OnReportBlockDataUpdated(data); // Report stats for second SSRC to make sure it's not outdated along with the // first SSRC. diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index b4adc135ec..ebd4445004 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -146,7 +146,6 @@ RtpSenderObservers CreateObservers(RtcpRttStats* call_stats, observers.rtcp_rtt_stats = call_stats; observers.intra_frame_callback = encoder_feedback; observers.rtcp_loss_notification_observer = encoder_feedback; - observers.rtcp_stats = stats_proxy; observers.report_block_data_observer = stats_proxy; observers.rtp_stats = stats_proxy; observers.bitrate_observer = stats_proxy;