From 73cf80a932e82f5fcc54347f00dd9a256fc44888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 24 Mar 2021 11:10:09 +0100 Subject: [PATCH] Fixes incorrect feedback to EncoderBitrateAdjuster [perf note] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the point where an EncodedImage is reported to the EncoderBitrateAdjuster (in order to estimate utilization), the image data has been cleared so the size is 0 - meaning the esimtated utilization is 0 so pushback is in effect only applied at the beginning before an estimate is available. This CL fixes that by explicitly using spatial/temporal id and size in bytes, rather than passing along the EncodedImage proxy. It is unclear when this broke, but the regression seems rather old. This CL will affect the encoded bitrate (and thus indirectly BWE ramp-up rate), but should avoid exessive delay at low bitrates. Perf bots will likely trigger alerts, this is expected. In case there are undesired side-effects, we can entirely disable the adjuster using existing field-trials. Bug: webrtc:12606 Change-Id: I936c2045f554696d8b4bb518eee6871ffc12c47d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/212900 Commit-Queue: Erik Språng Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#33550} --- video/encoder_bitrate_adjuster.cc | 9 ++++----- video/encoder_bitrate_adjuster.h | 2 +- video/encoder_bitrate_adjuster_unittest.cc | 9 +++------ video/video_stream_encoder.cc | 3 ++- video/video_stream_encoder_unittest.cc | 6 +----- 5 files changed, 11 insertions(+), 18 deletions(-) diff --git a/video/encoder_bitrate_adjuster.cc b/video/encoder_bitrate_adjuster.cc index 45d88875e3..6a2c99ffe3 100644 --- a/video/encoder_bitrate_adjuster.cc +++ b/video/encoder_bitrate_adjuster.cc @@ -314,15 +314,14 @@ void EncoderBitrateAdjuster::OnEncoderInfo( AdjustRateAllocation(current_rate_control_parameters_); } -void EncoderBitrateAdjuster::OnEncodedFrame(const EncodedImage& encoded_image, +void EncoderBitrateAdjuster::OnEncodedFrame(DataSize size, + int spatial_index, int temporal_index) { ++frames_since_layout_change_; // Detectors may not exist, for instance if ScreenshareLayers is used. - auto& detector = - overshoot_detectors_[encoded_image.SpatialIndex().value_or(0)] - [temporal_index]; + auto& detector = overshoot_detectors_[spatial_index][temporal_index]; if (detector) { - detector->OnEncodedFrame(encoded_image.size(), rtc::TimeMillis()); + detector->OnEncodedFrame(size.bytes(), rtc::TimeMillis()); } } diff --git a/video/encoder_bitrate_adjuster.h b/video/encoder_bitrate_adjuster.h index b142519b4e..74d0289ad0 100644 --- a/video/encoder_bitrate_adjuster.h +++ b/video/encoder_bitrate_adjuster.h @@ -47,7 +47,7 @@ class EncoderBitrateAdjuster { void OnEncoderInfo(const VideoEncoder::EncoderInfo& encoder_info); // Updates the overuse detectors according to the encoded image size. - void OnEncodedFrame(const EncodedImage& encoded_image, int temporal_index); + void OnEncodedFrame(DataSize size, int spatial_index, int temporal_index); void Reset(); diff --git a/video/encoder_bitrate_adjuster_unittest.cc b/video/encoder_bitrate_adjuster_unittest.cc index d8fcf382b2..c249a5cb79 100644 --- a/video/encoder_bitrate_adjuster_unittest.cc +++ b/video/encoder_bitrate_adjuster_unittest.cc @@ -160,15 +160,12 @@ class EncoderBitrateAdjusterTest : public ::testing::Test { int sequence_idx = sequence_idx_[si][ti]; sequence_idx_[si][ti] = (sequence_idx_[si][ti] + 1) % kSequenceLength; - const size_t frame_size_bytes = + const DataSize frame_size = DataSize::Bytes( (sequence_idx < kSequenceLength / 2) ? media_frame_size - network_frame_size_diff_bytes - : media_frame_size + network_frame_size_diff_bytes; + : media_frame_size + network_frame_size_diff_bytes); - EncodedImage image; - image.SetEncodedData(EncodedImageBuffer::Create(frame_size_bytes)); - image.SetSpatialIndex(si); - adjuster_->OnEncodedFrame(image, ti); + adjuster_->OnEncodedFrame(frame_size, si, ti); sequence_idx = ++sequence_idx % kSequenceLength; } } diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index c5a3b98756..07094e1a39 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -2159,7 +2159,8 @@ void VideoStreamEncoder::RunPostEncode(const EncodedImage& encoded_image, stream_resource_manager_.OnEncodeCompleted(encoded_image, time_sent_us, encode_duration_us); if (bitrate_adjuster_) { - bitrate_adjuster_->OnEncodedFrame(encoded_image, temporal_index); + bitrate_adjuster_->OnEncodedFrame( + frame_size, encoded_image.SpatialIndex().value_or(0), temporal_index); } } diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index fcfa6778ae..ebc1c29228 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -6953,11 +6953,7 @@ TEST_F(VideoStreamEncoderTest, DropsFramesWhenEncoderOvershoots) { // doesn't push back as hard so we don't need quite as much overshoot. // These numbers are unfortunately a bit magical but there's not trivial // way to algebraically infer them. - if (trials.BitrateAdjusterCanUseNetworkHeadroom()) { - overshoot_factor = 2.4; - } else { - overshoot_factor = 4.0; - } + overshoot_factor = 3.0; } fake_encoder_.SimulateOvershoot(overshoot_factor); video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources(