From d4c5843bae2db419aa035abac0b2edfcba8aab70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 5 Sep 2024 13:02:43 +0200 Subject: [PATCH] Undo recent changes to initial frame dropper, fixing a regression. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A recent bugfix[1] introduced having to reconfigure the encoder in response to restrictions updating for the sake of not getting stuck at the wrong resolution in some cases. But the newly added ReconfigureEncoder() calls happened too early/often, and the initial frame dropper got confused thinking resolution changes were not in response to adaptation (restrictions) when they in fact were. The CL wrongly disabled the "reset initial frame dropper" logic, when the correct solution to this problem should have been to delay the ReconfigurationEncoder() call to the next frame (using `pending_encoder_reconfiguration_ = true`). With this delay the initial frame dropper is not confused and we can restore the old "reset" logic, fixing the regression. [1] https://webrtc-review.googlesource.com/c/src/+/360200 Bug: b/364252657 Change-Id: I6b93f4cc44eb12b1bbbda0d8d1e9906c29b615a2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/361740 Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Sergey Silkin Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#42961} --- .../video_stream_encoder_resource_manager.cc | 12 ++++-------- video/video_stream_encoder.cc | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/video/adaptation/video_stream_encoder_resource_manager.cc b/video/adaptation/video_stream_encoder_resource_manager.cc index 96b7f89aa2..7581c2c0c9 100644 --- a/video/adaptation/video_stream_encoder_resource_manager.cc +++ b/video/adaptation/video_stream_encoder_resource_manager.cc @@ -161,21 +161,18 @@ class VideoStreamEncoderResourceManager::InitialFrameDropper { void OnEncoderSettingsUpdated( const VideoCodec& codec, - const VideoAdaptationCounters& adaptation_counters, - bool has_requested_resolution) { + const VideoAdaptationCounters& adaptation_counters) { last_stream_configuration_changed_ = false; std::vector active_flags = GetActiveLayersFlags(codec); // Check if the source resolution has changed for the external reasons, - // i.e. without any adaptation from WebRTC. This only matters when encoding - // resolution is relative to input frame, which it isn't if the - // `requested_resolution` API is used. + // i.e. without any adaptation from WebRTC. const bool source_resolution_changed = (last_input_width_ != codec.width || last_input_height_ != codec.height) && adaptation_counters.resolution_adaptations == last_adaptation_counters_.resolution_adaptations; if (!EqualFlags(active_flags, last_active_flags_) || - (!has_requested_resolution && source_resolution_changed)) { + source_resolution_changed) { // Streams configuration has changed. last_stream_configuration_changed_ = true; // Initial frame drop must be enabled because BWE might be way too low @@ -417,8 +414,7 @@ void VideoStreamEncoderResourceManager::SetEncoderSettings( encoder_settings_ = std::move(encoder_settings); bitrate_constraint_->OnEncoderSettingsUpdated(encoder_settings_); initial_frame_dropper_->OnEncoderSettingsUpdated( - encoder_settings_->video_codec(), current_adaptation_counters_, - encoder_settings.encoder_config().HasRequestedResolution()); + encoder_settings_->video_codec(), current_adaptation_counters_); MaybeUpdateTargetFrameRate(); } diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 821a1db554..da42b2a137 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -2386,8 +2386,8 @@ void VideoStreamEncoder::OnVideoSourceRestrictionsUpdated( // automatically on new frame size and we don't need to reconfigure here. if (encoder_ && max_pixels_updated && encoder_config_.HasRequestedResolution()) { + // The encoder will be reconfigured on the next frame. pending_encoder_reconfiguration_ = true; - ReconfigureEncoder(); } worker_queue_->PostTask(SafeTask(