From 41fffaa6f4557402c8ba1422c991bc8e3422c9ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Fri, 23 Aug 2024 11:16:37 +0200 Subject: [PATCH] Fix requested_resolution bug where we get stuck with old restrictions. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Normally (scaleResolutionDownBy) restrictions are applied at the source which changes the input frame size which triggers reconfiguration with appropriate scaling factors. But when requested_resolution is used, encoder settings are by definition not relative to the input frame size. In order for restrictions to have an effect, they are applied inside ReconfigureEncoder(): you get the minimum between the requested resolution and the restricted resolution. ReconfigureEncoder() happens when you SetParameters(), but the bug here is that we don't do it again once the restrictions are updated. So if restrictions are 540p when you ask for 720p, you get 540p and after restrictions change to unlimited you're still stuck in 540p. The fix is to also trigger ReconfigureEncoder() inside OnVideoSourceRestrictionsUpdated() when the restricted resolution is changing and a requested_resolution is configured. To ensure reconfiguring the encoder "on the fly" like this does not reset initial frame dropping logic, InitialFrameDropper caring about input frame size changing is made conditional on not using requested_resolution. # Slow purple bots failing but they are not affected by this change. NOTRY=True Bug: webrtc:361477261 Change-Id: I1389aa16cf408b0d14e0b5b6f68c2442db955be9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360200 Commit-Queue: Henrik Boström Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#42882} --- pc/BUILD.gn | 1 + ...er_connection_encodings_integrationtest.cc | 73 +++++++++++++++++++ .../video_stream_encoder_resource_manager.cc | 12 ++- video/config/video_encoder_config.cc | 9 +++ video/config/video_encoder_config.h | 2 + video/video_stream_encoder.cc | 28 +++++-- video/video_stream_encoder_unittest.cc | 68 +++++++++++++++++ 7 files changed, 184 insertions(+), 9 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 4ba9883ec7..429c4debea 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -2394,6 +2394,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api/video:builtin_video_bitrate_allocator_factory", "../api/video:encoded_image", "../api/video:recordable_encoded_frame", + "../api/video:resolution", "../api/video:video_bitrate_allocator_factory", "../api/video:video_codec_constants", "../api/video:video_frame", diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index 0400b92182..fcbc9fcef9 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -37,6 +37,7 @@ #include "api/stats/rtcstats_objects.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" +#include "api/video/resolution.h" #include "pc/sdp_utils.h" #include "pc/session_description.h" #include "pc/simulcast_description.h" @@ -304,6 +305,23 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { return false; } + Resolution GetEncodingResolution( + rtc::scoped_refptr pc_wrapper, + std::string_view rid = "") { + rtc::scoped_refptr report = GetStats(pc_wrapper); + std::vector outbound_rtps = + report->GetStatsOfType(); + for (const auto* outbound_rtp : outbound_rtps) { + if (outbound_rtp->rid.value_or("") == rid) { + return { + .width = static_cast(outbound_rtp->frame_width.value_or(0)), + .height = static_cast(outbound_rtp->frame_height.value_or(0))}; + } + } + RTC_CHECK(false) << "Rid not found: " << rid; + return {}; + } + bool HasOutboundRtpWithRidAndScalabilityMode( rtc::scoped_refptr pc_wrapper, absl::string_view rid, @@ -2302,6 +2320,61 @@ TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest, EXPECT_LE(EncodedFrames(local_pc_wrapper, "f") - encoded_frames_f, 2); } +TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest, + RequestedResolutionDownscaleAndThenUpscale) { + rtc::scoped_refptr local_pc_wrapper = CreatePc(); + if (SkipTestDueToAv1Missing(local_pc_wrapper)) { + return; + } + rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + std::vector layers = + CreateLayers({"f"}, /*active=*/true); + + // This transceiver receives a 1280x720 source. + rtc::scoped_refptr transceiver = + AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, + layers); + std::vector codecs = + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, codec_name_); + transceiver->SetCodecPreferences(codecs); + + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // Request 640x360, which is the same as scaling down by 2. + rtc::scoped_refptr sender = transceiver->sender(); + RtpParameters parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(1)); + parameters.encodings[0].scalability_mode = "L1T3"; + parameters.encodings[0].requested_resolution = {.width = 640, .height = 360}; + sender->SetParameters(parameters); + // Confirm 640x360 is sent. + ASSERT_TRUE_WAIT(GetEncodingResolution(local_pc_wrapper) == + (Resolution{.width = 640, .height = 360}), + kLongTimeoutForRampingUp.ms()); + + // Test coverage for https://crbug.com/webrtc/361477261: + // Due initial frame dropping, OnFrameDroppedDueToSize() should have created + // some resolution restrictions by now. With 720p input frame, restriction is + // 540p which is not observable when sending 360p, but it prevents us from + // immediately sending 720p. Restrictions will be lifted after a few seconds + // (when good QP is reported by QualityScaler) and 720p should be sent. The + // bug was not reconfiguring the encoder when restrictions were updated so the + // restrictions at the time of the SetParameter() call were made indefinite. + + // Request the full 1280x720 resolution. + parameters = sender->GetParameters(); + parameters.encodings[0].requested_resolution = {.width = 1280, .height = 720}; + sender->SetParameters(parameters); + // Confirm 1280x720 is sent. + ASSERT_TRUE_WAIT(GetEncodingResolution(local_pc_wrapper) == + (Resolution{.width = 1280, .height = 720}), + kLongTimeoutForRampingUp.ms()); +} + INSTANTIATE_TEST_SUITE_P(StandardPath, PeerConnectionEncodingsIntegrationParameterizedTest, ::testing::Values("VP8", diff --git a/video/adaptation/video_stream_encoder_resource_manager.cc b/video/adaptation/video_stream_encoder_resource_manager.cc index e00e214aca..2bb6f297ab 100644 --- a/video/adaptation/video_stream_encoder_resource_manager.cc +++ b/video/adaptation/video_stream_encoder_resource_manager.cc @@ -161,18 +161,21 @@ class VideoStreamEncoderResourceManager::InitialFrameDropper { void OnEncoderSettingsUpdated( const VideoCodec& codec, - const VideoAdaptationCounters& adaptation_counters) { + const VideoAdaptationCounters& adaptation_counters, + bool has_requested_resolution) { 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. + // 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. 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_) || - source_resolution_changed) { + (!has_requested_resolution && 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 @@ -414,7 +417,8 @@ 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_->video_codec(), current_adaptation_counters_, + encoder_settings.encoder_config().HasRequestedResolution()); MaybeUpdateTargetFrameRate(); } diff --git a/video/config/video_encoder_config.cc b/video/config/video_encoder_config.cc index 5cecc45a5d..c3e81ec8a8 100644 --- a/video/config/video_encoder_config.cc +++ b/video/config/video_encoder_config.cc @@ -88,6 +88,15 @@ std::string VideoEncoderConfig::ToString() const { return ss.str(); } +bool VideoEncoderConfig::HasRequestedResolution() const { + for (const VideoStream& simulcast_layer : simulcast_layers) { + if (simulcast_layer.requested_resolution.has_value()) { + return true; + } + } + return false; +} + VideoEncoderConfig::VideoEncoderConfig(const VideoEncoderConfig&) = default; void VideoEncoderConfig::EncoderSpecificSettings::FillEncoderSpecificSettings( diff --git a/video/config/video_encoder_config.h b/video/config/video_encoder_config.h index d3d0621a1d..3f8f286bc6 100644 --- a/video/config/video_encoder_config.h +++ b/video/config/video_encoder_config.h @@ -166,6 +166,8 @@ class VideoEncoderConfig { ~VideoEncoderConfig(); std::string ToString() const; + bool HasRequestedResolution() const; + // TODO(bugs.webrtc.org/6883): Consolidate on one of these. VideoCodecType codec_type; SdpVideoFormat video_format; diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index a9391e0cea..45797023f2 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -734,6 +734,14 @@ void VideoStreamEncoder::Stop() { encoder_queue_->PostTask([this, shutdown = std::move(shutdown)] { RTC_DCHECK_RUN_ON(encoder_queue_.get()); if (resource_adaptation_processor_) { + // We're no longer interested in restriction updates, which may get + // triggered as part of removing resources. + video_stream_adapter_->RemoveRestrictionsListener(this); + video_stream_adapter_->RemoveRestrictionsListener( + &stream_resource_manager_); + resource_adaptation_processor_->RemoveResourceLimitationsListener( + &stream_resource_manager_); + // Stop and remove resources and delete adaptation processor. stream_resource_manager_.StopManagedResources(); for (auto* constraint : adaptation_constraints_) { video_stream_adapter_->RemoveAdaptationConstraint(constraint); @@ -742,11 +750,6 @@ void VideoStreamEncoder::Stop() { stream_resource_manager_.RemoveResource(resource); } additional_resources_.clear(); - video_stream_adapter_->RemoveRestrictionsListener(this); - video_stream_adapter_->RemoveRestrictionsListener( - &stream_resource_manager_); - resource_adaptation_processor_->RemoveResourceLimitationsListener( - &stream_resource_manager_); stream_resource_manager_.SetAdaptationProcessor(nullptr, nullptr); resource_adaptation_processor_.reset(); } @@ -2368,10 +2371,25 @@ void VideoStreamEncoder::OnVideoSourceRestrictionsUpdated( restrictions.max_frame_rate()); } + bool max_pixels_updated = + (latest_restrictions_.has_value() + ? latest_restrictions_->max_pixels_per_frame() + : absl::nullopt) != restrictions.max_pixels_per_frame(); + // TODO(webrtc:14451) Split video_source_sink_controller_ // so that ownership on restrictions/wants is kept on &encoder_queue_ latest_restrictions_ = restrictions; + // When the `requested_resolution` API is used, we need to reconfigure any + // time the restricted resolution is updated. When that API isn't used, the + // encoder settings are relative to the frame size and reconfiguration happens + // automatically on new frame size and we don't need to reconfigure here. + if (encoder_ && max_pixels_updated && + encoder_config_.HasRequestedResolution()) { + pending_encoder_reconfiguration_ = true; + ReconfigureEncoder(); + } + worker_queue_->PostTask(SafeTask( task_safety_.flag(), [this, restrictions = std::move(restrictions)]() { RTC_DCHECK_RUN_ON(worker_queue_); diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 9aade033d2..e208501c3e 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -6302,6 +6302,74 @@ TEST_F(VideoStreamEncoderTest, InitialFrameDropIsNotReactivatedWhenAdaptingUp) { video_stream_encoder_->Stop(); } +// Same test as above but setting resolution using `requested_resolution`, which +// does affect the number of ReconfigureEncoder() requiring extra care not to +// reactivate InitialFrameDrop. +TEST_F(VideoStreamEncoderTest, + InitialFrameDropIsNotReactivatedWhenAdaptingUp_RequestedResolution) { + const int kWidth = 640; + const int kHeight = 360; + // So that quality scaling doesn't happen by itself. + fake_encoder_.SetQp(kQpHigh); + + AdaptingFrameForwarder source(&time_controller_); + source.set_adaptation_enabled(true); + video_stream_encoder_->SetSource( + &source, webrtc::DegradationPreference::MAINTAIN_FRAMERATE); + + int timestamp = 1; + + // By using the `requested_resolution` API, ReconfigureEncoder() gets + // triggered from VideoStreamEncoder::OnVideoSourceRestrictionsUpdated(). + ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1)); + video_encoder_config_.simulcast_layers[0].requested_resolution.emplace( + Resolution({.width = kWidth, .height = kHeight})); + video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(), + kMaxPayloadLength); + + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + kTargetBitrate, kTargetBitrate, kTargetBitrate, 0, 0, 0); + source.IncomingCapturedFrame(CreateFrame(timestamp, kWidth, kHeight)); + WaitForEncodedFrame(timestamp); + timestamp += 9000; + // Long pause to disable all first BWE drop logic. + AdvanceTime(TimeDelta::Millis(1000)); + + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + kLowTargetBitrate, kLowTargetBitrate, kLowTargetBitrate, 0, 0, 0); + source.IncomingCapturedFrame(CreateFrame(timestamp, kWidth, kHeight)); + // Not dropped frame, as initial frame drop is disabled by now. + WaitForEncodedFrame(timestamp); + timestamp += 9000; + AdvanceTime(TimeDelta::Millis(100)); + + // Quality adaptation down. + video_stream_encoder_->TriggerQualityLow(); + + // Adaptation has an effect. + EXPECT_TRUE_WAIT(source.sink_wants().max_pixel_count < kWidth * kHeight, + 5000); + + // Frame isn't dropped as initial frame dropper is disabled. + source.IncomingCapturedFrame(CreateFrame(timestamp, kWidth, kHeight)); + WaitForEncodedFrame(timestamp); + timestamp += 9000; + AdvanceTime(TimeDelta::Millis(100)); + + // Quality adaptation up. + video_stream_encoder_->TriggerQualityHigh(); + + // Adaptation has an effect. + EXPECT_TRUE_WAIT(source.sink_wants().max_pixel_count > kWidth * kHeight, + 5000); + + source.IncomingCapturedFrame(CreateFrame(timestamp, kWidth, kHeight)); + // Frame should not be dropped, as initial framedropper is off. + WaitForEncodedFrame(timestamp); + + video_stream_encoder_->Stop(); +} + TEST_F(VideoStreamEncoderTest, FrameDroppedWhenResolutionIncreasesAndLinkAllocationIsLow) { const int kMinStartBps360p = 222000;