Delete RtcpStatisticsCallback in favor of ReportBlockDataObserver
Bug: webrtc:10678
Change-Id: Ie016cbc47dbba15176fc5e7ad7d01a438db7dfb3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/218842
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34013}
diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h
index 5926344..2aa6d73 100644
--- a/call/rtp_transport_controller_send_interface.h
+++ b/call/rtp_transport_controller_send_interface.h
@@ -52,7 +52,6 @@
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 022c97d..c2a6a56 100644
--- a/call/rtp_video_sender.cc
+++ b/call/rtp_video_sender.cc
@@ -216,7 +216,6 @@
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 88cbeed..85934cc 100644
--- a/call/rtp_video_sender_unittest.cc
+++ b/call/rtp_video_sender_unittest.cc
@@ -61,7 +61,6 @@
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 @@
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 @@
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<FecControllerDefault>(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 e26c475..8a63324 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 @@
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 104b61b..526acf5 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver.cc
@@ -142,7 +142,6 @@
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 @@
}
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 370a875..429df55 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver.h
+++ b/modules/rtp_rtcp/source/rtcp_receiver.h
@@ -340,11 +340,7 @@
// 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 068a110..c71bbcc 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
@@ -87,14 +87,6 @@
(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 @@
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 5ab48c9..457a993 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h
@@ -77,13 +77,10 @@
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 6752abe..971e42f 100644
--- a/video/send_statistics_proxy.cc
+++ b/video/send_statistics_proxy.cc
@@ -1285,17 +1285,6 @@
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 @@
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 0de7df2..bfb221f 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 @@
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 71b84c9..aa13302 100644
--- a/video/send_statistics_proxy_unittest.cc
+++ b/video/send_statistics_proxy_unittest.cc
@@ -174,29 +174,51 @@
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 @@
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 @@
// 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 b4adc13..ebd4445 100644
--- a/video/video_send_stream_impl.cc
+++ b/video/video_send_stream_impl.cc
@@ -146,7 +146,6 @@
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;
