From fe25b0e928ea4e64aa134f5dc8012343320deec5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 6 Feb 2025 16:36:51 +0100 Subject: [PATCH] Report 'outbound-rtp.targetBitrate' correctly and per-RTP stream. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL fixes two issues with the old way targetBitrate was reported: 1. The target is per encoder, i.e. per SSRC, but the old way to report it was per sender and was approximately the sum of all encodings' targetBitrate in most cases. 2. The old value did not come directly from the VideoBitrateAllocation and tended to be greater than the sum of all targets (don't know why). We know the old value was wrong and the new value correct because the actual bytes produced by the encoder closely matches the configured target, which wasn't always the case with the old metric implementation. Tested with unit tests and manually in Chrome by going to https://henbos.github.io/codec-quality/src/index.html and ensuring target ~= actual bytes produced. It also matches the debug logging of video_stream_encoder.cc. Bug: webrtc:42225524, chromium:392424845 Change-Id: I7a6f69e053ebc3fd972c2c4b7712750e721c0acc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/376460 Reviewed-by: Danil Chapovalov Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#43854} --- call/BUILD.gn | 1 + call/video_send_stream.h | 11 +++++++-- media/BUILD.gn | 1 + media/base/media_channel.h | 3 ++- media/engine/webrtc_video_engine.cc | 2 +- media/engine/webrtc_voice_engine.cc | 5 ++++- pc/rtc_stats_collector.cc | 10 ++++----- pc/rtc_stats_collector_unittest.cc | 3 ++- video/send_statistics_proxy.cc | 20 +++++++++++++++++ video/send_statistics_proxy_unittest.cc | 30 +++++++++++++++++++++++++ 10 files changed, 74 insertions(+), 12 deletions(-) diff --git a/call/BUILD.gn b/call/BUILD.gn index 484b772e49..8833fe9ff4 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -438,6 +438,7 @@ rtc_library("video_send_stream_api") { "../api:transport_api", "../api/adaptation:resource_adaptation_api", "../api/crypto:options", + "../api/units:data_rate", "../api/video:video_frame", "../api/video:video_rtp_headers", "../api/video:video_stream_encoder", diff --git a/call/video_send_stream.h b/call/video_send_stream.h index 3bf13c5e74..02f3569dd2 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h @@ -25,6 +25,7 @@ #include "api/rtp_parameters.h" #include "api/rtp_sender_interface.h" #include "api/scoped_refptr.h" +#include "api/units/data_rate.h" #include "api/video/video_content_type.h" #include "api/video/video_frame.h" #include "api/video/video_source_interface.h" @@ -95,6 +96,9 @@ class VideoSendStream { uint64_t total_encoded_bytes_target = 0; uint32_t huge_frames_sent = 0; std::optional scalability_mode; + // The target bitrate is what we tell the encoder to produce. What the + // encoder actually produces is the sum of encoded bytes. + std::optional target_bitrate; }; struct Stats { @@ -118,8 +122,11 @@ class VideoSendStream { uint32_t frames_dropped_by_rate_limiter = 0; uint32_t frames_dropped_by_congestion_window = 0; uint32_t frames_dropped_by_encoder = 0; - // Bitrate the encoder is currently configured to use due to bandwidth - // limitations. + // Metric only used by legacy getStats()'s BWE. + // - Similar to `StreamStats::target_bitrate` except this is for the whole + // stream as opposed to being per substream (per SSRC). + // - Unlike what you would expect, it is not equal to the sum of all + // substream targets and may sometimes over-report e.g. webrtc:392424845. int target_media_bitrate_bps = 0; // Bitrate the encoder is actually producing. int media_bitrate_bps = 0; diff --git a/media/BUILD.gn b/media/BUILD.gn index 19c6cd0b22..0564eb745b 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -327,6 +327,7 @@ rtc_source_set("media_channel") { "../api/task_queue:pending_task_safety_flag", "../api/transport:datagram_transport_interface", "../api/transport/rtp:rtp_source", + "../api/units:data_rate", "../api/units:time_delta", "../api/units:timestamp", "../api/video:recordable_encoded_frame", diff --git a/media/base/media_channel.h b/media/base/media_channel.h index 55ec4ff2e9..c85d6a3c51 100644 --- a/media/base/media_channel.h +++ b/media/base/media_channel.h @@ -39,6 +39,7 @@ #include "api/rtp_sender_interface.h" #include "api/scoped_refptr.h" #include "api/transport/rtp/rtp_source.h" +#include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "api/video/recordable_encoded_frame.h" @@ -386,7 +387,7 @@ struct MediaSenderInfo { // https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-nackcount uint32_t nacks_received = 0; // https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-targetbitrate - std::optional target_bitrate; + std::optional target_bitrate; int packets_lost = 0; float fraction_lost = 0.0f; int64_t rtt_ms = 0; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index b3e25c6c5b..358f27d9a5 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2461,7 +2461,6 @@ WebRtcVideoSendChannel::WebRtcVideoSendStream::GetPerLayerVideoSenderInfos( common_info.quality_limitation_resolution_changes = stats.quality_limitation_resolution_changes; common_info.encoder_implementation_name = stats.encoder_implementation_name; - common_info.target_bitrate = stats.target_media_bitrate_bps; common_info.ssrc_groups = ssrc_groups_; common_info.frames = stats.frames; common_info.framerate_input = stats.input_frame_rate; @@ -2544,6 +2543,7 @@ WebRtcVideoSendChannel::WebRtcVideoSendStream::GetPerLayerVideoSenderInfos( info.total_encoded_bytes_target = stream_stats.total_encoded_bytes_target; info.huge_frames_sent = stream_stats.huge_frames_sent; info.scalability_mode = stream_stats.scalability_mode; + info.target_bitrate = stream_stats.target_bitrate; infos.push_back(info); } return infos; diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index f8c237173f..82d38f8a1c 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1807,7 +1807,10 @@ bool WebRtcVoiceSendChannel::GetStats(VoiceMediaSendInfo* info) { sinfo.packets_lost = stats.packets_lost; sinfo.fraction_lost = stats.fraction_lost; sinfo.nacks_received = stats.nacks_received; - sinfo.target_bitrate = stats.target_bitrate_bps; + sinfo.target_bitrate = stats.target_bitrate_bps > 0 + ? std::optional(webrtc::DataRate::BitsPerSec( + stats.target_bitrate_bps)) + : std::nullopt; sinfo.codec_name = stats.codec_name; sinfo.codec_payload_type = stats.codec_payload_type; sinfo.jitter_ms = stats.jitter_ms; diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 451590fced..6ff5dabbd6 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -739,9 +739,8 @@ CreateOutboundRTPStreamStatsFromVoiceSenderInfo( outbound_audio->transport_id = transport_id; outbound_audio->mid = mid; outbound_audio->kind = "audio"; - if (voice_sender_info.target_bitrate.has_value() && - *voice_sender_info.target_bitrate > 0) { - outbound_audio->target_bitrate = *voice_sender_info.target_bitrate; + if (voice_sender_info.target_bitrate.has_value()) { + outbound_audio->target_bitrate = voice_sender_info.target_bitrate->bps(); } if (voice_sender_info.codec_payload_type.has_value()) { auto codec_param_it = voice_media_info.send_codecs.find( @@ -791,9 +790,8 @@ CreateOutboundRTPStreamStatsFromVideoSenderInfo( static_cast(video_sender_info.plis_received); if (video_sender_info.qp_sum.has_value()) outbound_video->qp_sum = *video_sender_info.qp_sum; - if (video_sender_info.target_bitrate.has_value() && - *video_sender_info.target_bitrate > 0) { - outbound_video->target_bitrate = *video_sender_info.target_bitrate; + if (video_sender_info.target_bitrate.has_value()) { + outbound_video->target_bitrate = video_sender_info.target_bitrate->bps(); } outbound_video->frames_encoded = video_sender_info.frames_encoded; outbound_video->key_frames_encoded = video_sender_info.key_frames_encoded; diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 1737f26cc6..6d9dab4726 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -44,6 +44,7 @@ #include "api/stats/rtcstats_objects.h" #include "api/test/rtc_error_matchers.h" #include "api/transport/enums.h" +#include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "api/video/recordable_encoded_frame.h" @@ -2557,7 +2558,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRtpStreamStats_Audio) { voice_media_info.senders[0].header_and_padding_bytes_sent = 12; voice_media_info.senders[0].retransmitted_bytes_sent = 30; voice_media_info.senders[0].nacks_received = 31; - voice_media_info.senders[0].target_bitrate = 32000; + voice_media_info.senders[0].target_bitrate = DataRate::BitsPerSec(32'000); voice_media_info.senders[0].codec_payload_type = 42; voice_media_info.senders[0].active = true; diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc index 8c97a27081..412ff472f5 100644 --- a/video/send_statistics_proxy.cc +++ b/video/send_statistics_proxy.cc @@ -1241,6 +1241,26 @@ void SendStatisticsProxy::OnBitrateAllocationUpdated( bw_limited_layers_ = allocation.is_bw_limited(); UpdateAdaptationStats(); + // Store target bitrates per substream stats. + for (auto& [ssrc, substream] : stats_.substreams) { + std::optional simulcast_index; + for (size_t i = 0; i < rtp_config_.ssrcs.size(); ++i) { + if (rtp_config_.ssrcs[i] == ssrc) { + simulcast_index = i; + break; + } + } + if (!simulcast_index.has_value()) { + substream.target_bitrate = std::nullopt; + continue; + } + substream.target_bitrate = + DataRate::BitsPerSec(allocation.GetSpatialLayerSum(*simulcast_index)); + if (substream.target_bitrate == DataRate::Zero()) { + substream.target_bitrate = std::nullopt; + } + } + if (spatial_layers != last_spatial_layer_use_) { // If the number of spatial layers has changed, the resolution change is // not due to quality limitations, it is because the configuration diff --git a/video/send_statistics_proxy_unittest.cc b/video/send_statistics_proxy_unittest.cc index 0d022c7b20..f2a757c0ff 100644 --- a/video/send_statistics_proxy_unittest.cc +++ b/video/send_statistics_proxy_unittest.cc @@ -1508,6 +1508,36 @@ TEST_F(SendStatisticsProxyTest, 0u, statistics_proxy_->GetStats().quality_limitation_resolution_changes); } +TEST_F(SendStatisticsProxyTest, OnBitrateAllocationUpdatedSetsTargetBitrates) { + // We only update target bitrates for substreams that exist and these are + // created lazily in various places... calling OnInactiveSsrc() is one way to + // ensure the stats are reported. + statistics_proxy_->OnInactiveSsrc(kFirstSsrc); + statistics_proxy_->OnInactiveSsrc(kSecondSsrc); + + // Update target bitrates! + VideoBitrateAllocation allocation; + allocation.SetBitrate(0, 0, 123); + allocation.SetBitrate(1, 0, 321); + statistics_proxy_->OnBitrateAllocationUpdated(VideoCodec(), allocation); + EXPECT_EQ(statistics_proxy_->GetStats().substreams[kFirstSsrc].target_bitrate, + DataRate::BitsPerSec(123)); + EXPECT_EQ( + statistics_proxy_->GetStats().substreams[kSecondSsrc].target_bitrate, + DataRate::BitsPerSec(321)); + + // 0 bitrate = no target. + allocation.SetBitrate(0, 0, 0); + allocation.SetBitrate(1, 0, 0); + statistics_proxy_->OnBitrateAllocationUpdated(VideoCodec(), allocation); + EXPECT_FALSE(statistics_proxy_->GetStats() + .substreams[kFirstSsrc] + .target_bitrate.has_value()); + EXPECT_FALSE(statistics_proxy_->GetStats() + .substreams[kSecondSsrc] + .target_bitrate.has_value()); +} + TEST_F(SendStatisticsProxyTest, QualityLimitationResolutionDoesNotUpdateForSpatialLayerChanges) { VideoCodec codec;