From 87e3f9d116fd2a2098ee746d7a3d43c2d81c0a78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 27 May 2019 10:44:24 +0200 Subject: [PATCH] [video] Plumbing of ReportBlockData from RTCPReceiver to MediaSenderInfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is part of implementing RTCRemoteInboundRtpStreamStats. The CL was split up into smaller pieces for reviewability. Spec: https://w3c.github.io/webrtc-stats/#dom-rtcremoteinboundrtpstreamstats In [1], ReportBlockData was implemented and tested. This CL adds the plumbing to make it available in MediaSenderInfo for video streams, but the code is not wired up to make use of these stats. In follow-up CL [2], ReportBlockData will be used to implement RTCRemoteInboundRtpStreamStats. The follow-up CL will add integration tests exercising the full code path. [1] https://webrtc-review.googlesource.com/c/src/+/136584 [2] https://webrtc-review.googlesource.com/c/src/+/138067 Bug: webrtc:10456 Change-Id: Icd20452cb4b4908203b28ae9d9f52c25693cf91d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138065 Reviewed-by: Stefan Holmer Reviewed-by: Niels Moller Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/master@{#28071} --- call/rtp_transport_controller_send_interface.h | 2 ++ call/rtp_video_sender.cc | 2 ++ call/rtp_video_sender_unittest.cc | 7 +++++-- call/video_send_stream.h | 4 ++++ media/BUILD.gn | 1 + media/base/media_channel.h | 6 ++++++ media/engine/webrtc_video_engine.cc | 4 ++++ modules/rtp_rtcp/include/rtp_rtcp.h | 11 +++++++++++ modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 1 + modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 +++++ modules/rtp_rtcp/source/rtp_rtcp_impl.h | 1 + video/send_statistics_proxy.cc | 10 ++++++++++ video/send_statistics_proxy.h | 4 ++++ video/video_send_stream_impl.cc | 1 + 14 files changed, 57 insertions(+), 2 deletions(-) diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index 450b65b3f9..0db4f80919 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -25,6 +25,7 @@ #include "api/transport/bitrate_settings.h" #include "call/rtp_config.h" #include "logging/rtc_event_log/rtc_event_log.h" +#include "modules/rtp_rtcp/include/report_block_data.h" #include "modules/rtp_rtcp/include/rtcp_statistics.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" @@ -56,6 +57,7 @@ struct RtpSenderObservers { RtcpIntraFrameObserver* intra_frame_callback; RtcpLossNotificationObserver* rtcp_loss_notification_observer; RtcpStatisticsCallback* rtcp_stats; + ReportBlockDataObserver* report_block_data_observer; StreamDataCountersCallback* rtp_stats; BitrateStatisticsObserver* bitrate_observer; FrameCountObserver* frame_count_observer; diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 59817fb190..d67bf0873e 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -306,6 +306,8 @@ RtpVideoSender::RtpVideoSender( // 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->RegisterSendChannelRtpStatisticsCallback( observers.rtp_stats); stream.rtp_rtcp->SetMaxRtpPacketSize(rtp_config_.max_packet_size); diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index bd23bc6766..9622fbc10e 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -59,6 +59,7 @@ RtpSenderObservers CreateObservers( RtcpRttStats* rtcp_rtt_stats, RtcpIntraFrameObserver* intra_frame_callback, RtcpStatisticsCallback* rtcp_stats, + ReportBlockDataObserver* report_block_data_observer, StreamDataCountersCallback* rtp_stats, BitrateStatisticsObserver* bitrate_observer, FrameCountObserver* frame_count_observer, @@ -70,6 +71,7 @@ RtpSenderObservers CreateObservers( 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; observers.frame_count_observer = frame_count_observer; @@ -135,8 +137,9 @@ class RtpVideoSenderTestFixture { &clock_, suspended_ssrcs, suspended_payload_states, config_.rtp, config_.rtcp_report_interval_ms, &transport_, CreateObservers(&call_stats_, &encoder_feedback_, &stats_proxy_, - &stats_proxy_, &stats_proxy_, frame_count_observer, - &stats_proxy_, &stats_proxy_, &send_delay_stats_), + &stats_proxy_, &stats_proxy_, &stats_proxy_, + frame_count_observer, &stats_proxy_, &stats_proxy_, + &send_delay_stats_), &transport_controller_, &event_log_, &retransmission_rate_limiter_, absl::make_unique(&clock_), nullptr, CryptoOptions{}); diff --git a/call/video_send_stream.h b/call/video_send_stream.h index 42760bd300..929aa88398 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h @@ -28,6 +28,7 @@ #include "api/video/video_stream_encoder_settings.h" #include "api/video_codecs/video_encoder_config.h" #include "call/rtp_config.h" +#include "modules/rtp_rtcp/include/report_block_data.h" #include "modules/rtp_rtcp/include/rtcp_statistics.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" @@ -57,6 +58,9 @@ class VideoSendStream { StreamDataCounters rtp_stats; RtcpPacketTypeCounter rtcp_packet_type_counts; RtcpStatistics rtcp_stats; + // A snapshot of the most recent Report Block with additional data of + // interest to statistics. Used to implement RTCRemoteInboundRtpStreamStats. + absl::optional report_block_data; }; struct Stats { diff --git a/media/BUILD.gn b/media/BUILD.gn index a5a37e67ca..5cf4175a25 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -85,6 +85,7 @@ rtc_static_library("rtc_media_base") { "../call:call_interfaces", "../common_video", "../modules/audio_processing:audio_processing_statistics", + "../modules/rtp_rtcp:rtp_rtcp_format", "../rtc_base", "../rtc_base:checks", "../rtc_base:rtc_base_approved", diff --git a/media/base/media_channel.h b/media/base/media_channel.h index 8bdcd2b55e..3b9a54c17b 100644 --- a/media/base/media_channel.h +++ b/media/base/media_channel.h @@ -37,6 +37,7 @@ #include "media/base/media_constants.h" #include "media/base/stream_params.h" #include "modules/audio_processing/include/audio_processing_statistics.h" +#include "modules/rtp_rtcp/include/report_block_data.h" #include "rtc_base/async_packet_socket.h" #include "rtc_base/buffer.h" #include "rtc_base/copy_on_write_buffer.h" @@ -404,6 +405,11 @@ struct MediaSenderInfo { absl::optional codec_payload_type; std::vector local_stats; std::vector remote_stats; + // A snapshot of the most recent Report Block with additional data of interest + // to statistics. Used to implement RTCRemoteInboundRtpStreamStats. Within + // this list, the ReportBlockData::RTCPReportBlock::source_ssrc(), which is + // the SSRC of the corresponding outbound RTP stream, is unique. + std::vector report_block_datas; }; struct MediaReceiverInfo { diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 2e7e86e13b..5950204779 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2314,6 +2314,10 @@ VideoSenderInfo WebRtcVideoChannel::WebRtcVideoSendStream::GetVideoSenderInfo( info.firs_rcvd += stream_stats.rtcp_packet_type_counts.fir_packets; info.nacks_rcvd += stream_stats.rtcp_packet_type_counts.nack_packets; info.plis_rcvd += stream_stats.rtcp_packet_type_counts.pli_packets; + if (stream_stats.report_block_data.has_value() && !stream_stats.is_rtx && + !stream_stats.is_flexfec) { + info.report_block_datas.push_back(stream_stats.report_block_data.value()); + } } if (!stats.substreams.empty()) { diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index a207a11dfb..216f758d63 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -24,6 +24,7 @@ #include "modules/include/module.h" #include "modules/rtp_rtcp/include/flexfec_sender.h" #include "modules/rtp_rtcp/include/receive_statistics.h" +#include "modules/rtp_rtcp/include/report_block_data.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtp_sender.h" #include "rtc_base/constructor_magic.h" @@ -397,9 +398,19 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { 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(). virtual void RegisterRtcpStatisticsCallback( RtcpStatisticsCallback* callback) = 0; virtual RtcpStatisticsCallback* GetRtcpStatisticsCallback() = 0; + // TODO(https://crbug.com/webrtc/10680): When callbacks are registered at + // construction, remove this setter. + virtual void SetReportBlockDataObserver( + ReportBlockDataObserver* observer) = 0; // BWE feedback packets. bool SendFeedbackPacket(const rtcp::TransportFeedback& packet) override = 0; diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 95d3cb2122..eaad03d9a6 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -145,6 +145,7 @@ class MockRtpRtcp : public RtpRtcp { MOCK_CONST_METHOD0(StorePackets, bool()); MOCK_METHOD1(RegisterRtcpStatisticsCallback, void(RtcpStatisticsCallback*)); MOCK_METHOD0(GetRtcpStatisticsCallback, RtcpStatisticsCallback*()); + MOCK_METHOD1(SetReportBlockDataObserver, void(ReportBlockDataObserver*)); MOCK_METHOD1(SendFeedbackPacket, bool(const rtcp::TransportFeedback& packet)); MOCK_METHOD1(SetTargetSendBitrate, void(uint32_t bitrate_bps)); MOCK_METHOD1(SetKeyFrameRequestMethod, int32_t(KeyFrameRequestMethod method)); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 2e8e662b9f..1d7f048e4b 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -721,6 +721,11 @@ RtcpStatisticsCallback* ModuleRtpRtcpImpl::GetRtcpStatisticsCallback() { return rtcp_receiver_.GetRtcpStatisticsCallback(); } +void ModuleRtpRtcpImpl::SetReportBlockDataObserver( + ReportBlockDataObserver* observer) { + return rtcp_receiver_.SetReportBlockDataObserver(observer); +} + bool ModuleRtpRtcpImpl::SendFeedbackPacket( const rtcp::TransportFeedback& packet) { return rtcp_sender_.SendFeedbackPacket(packet); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 3fb1126beb..71a856cfe5 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -234,6 +234,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { void RegisterRtcpStatisticsCallback( RtcpStatisticsCallback* callback) override; RtcpStatisticsCallback* GetRtcpStatisticsCallback() override; + void SetReportBlockDataObserver(ReportBlockDataObserver* observer) override; bool SendFeedbackPacket(const rtcp::TransportFeedback& packet) override; // (APP) Application specific data. diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc index 27e6014854..0313316567 100644 --- a/video/send_statistics_proxy.cc +++ b/video/send_statistics_proxy.cc @@ -1136,6 +1136,16 @@ void SendStatisticsProxy::StatisticsUpdated(const RtcpStatistics& statistics, void SendStatisticsProxy::CNameChanged(const char* cname, uint32_t ssrc) {} +void SendStatisticsProxy::OnReportBlockDataUpdated( + ReportBlockData report_block_data) { + rtc::CritScope lock(&crit_); + VideoSendStream::StreamStats* stats = + GetStatsEntry(report_block_data.report_block().source_ssrc); + if (!stats) + return; + stats->report_block_data = std::move(report_block_data); +} + void SendStatisticsProxy::DataCountersUpdated( const StreamDataCounters& counters, uint32_t ssrc) { diff --git a/video/send_statistics_proxy.h b/video/send_statistics_proxy.h index 3b61f324c4..51d5b2f381 100644 --- a/video/send_statistics_proxy.h +++ b/video/send_statistics_proxy.h @@ -18,6 +18,7 @@ #include "api/video/video_stream_encoder_observer.h" #include "call/video_send_stream.h" +#include "modules/rtp_rtcp/include/report_block_data.h" #include "modules/video_coding/include/video_codec_interface.h" #include "modules/video_coding/include/video_coding_defines.h" #include "rtc_base/critical_section.h" @@ -32,6 +33,7 @@ namespace webrtc { class SendStatisticsProxy : public VideoStreamEncoderObserver, public RtcpStatisticsCallback, + public ReportBlockDataObserver, public RtcpPacketTypeCounterObserver, public StreamDataCountersCallback, public BitrateStatisticsObserver, @@ -93,6 +95,8 @@ class SendStatisticsProxy : public VideoStreamEncoderObserver, void StatisticsUpdated(const RtcpStatistics& statistics, uint32_t ssrc) override; void CNameChanged(const char* cname, uint32_t ssrc) override; + // From ReportBlockDataObserver. + void OnReportBlockDataUpdated(ReportBlockData report_block_data) override; // From RtcpPacketTypeCounterObserver. void RtcpPacketTypesCounterUpdated( uint32_t ssrc, diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 0b3d08114b..1dc6599b9b 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -151,6 +151,7 @@ RtpSenderObservers CreateObservers(CallStats* 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; observers.frame_count_observer = stats_proxy;