Fix requested_resolution bug where we get stuck with old restrictions.
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 <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42882}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 4ba9883..429c4de 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -2394,6 +2394,7 @@
"../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 0400b92..fcbc9fc 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 @@
return false;
}
+ Resolution GetEncodingResolution(
+ rtc::scoped_refptr<PeerConnectionTestWrapper> pc_wrapper,
+ std::string_view rid = "") {
+ rtc::scoped_refptr<const RTCStatsReport> report = GetStats(pc_wrapper);
+ std::vector<const RTCOutboundRtpStreamStats*> outbound_rtps =
+ report->GetStatsOfType<RTCOutboundRtpStreamStats>();
+ for (const auto* outbound_rtp : outbound_rtps) {
+ if (outbound_rtp->rid.value_or("") == rid) {
+ return {
+ .width = static_cast<int>(outbound_rtp->frame_width.value_or(0)),
+ .height = static_cast<int>(outbound_rtp->frame_height.value_or(0))};
+ }
+ }
+ RTC_CHECK(false) << "Rid not found: " << rid;
+ return {};
+ }
+
bool HasOutboundRtpWithRidAndScalabilityMode(
rtc::scoped_refptr<PeerConnectionTestWrapper> pc_wrapper,
absl::string_view rid,
@@ -2302,6 +2320,61 @@
EXPECT_LE(EncodedFrames(local_pc_wrapper, "f") - encoded_frames_f, 2);
}
+TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest,
+ RequestedResolutionDownscaleAndThenUpscale) {
+ rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
+ if (SkipTestDueToAv1Missing(local_pc_wrapper)) {
+ return;
+ }
+ rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper = CreatePc();
+ ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper);
+
+ std::vector<cricket::SimulcastLayer> layers =
+ CreateLayers({"f"}, /*active=*/true);
+
+ // This transceiver receives a 1280x720 source.
+ rtc::scoped_refptr<RtpTransceiverInterface> transceiver =
+ AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper,
+ layers);
+ std::vector<RtpCodecCapability> 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<RtpSenderInterface> 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 e00e214..2bb6f29 100644
--- a/video/adaptation/video_stream_encoder_resource_manager.cc
+++ b/video/adaptation/video_stream_encoder_resource_manager.cc
@@ -161,18 +161,21 @@
void OnEncoderSettingsUpdated(
const VideoCodec& codec,
- const VideoAdaptationCounters& adaptation_counters) {
+ const VideoAdaptationCounters& adaptation_counters,
+ bool has_requested_resolution) {
last_stream_configuration_changed_ = false;
std::vector<bool> 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 @@
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 5cecc45..c3e81ec 100644
--- a/video/config/video_encoder_config.cc
+++ b/video/config/video_encoder_config.cc
@@ -88,6 +88,15 @@
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 d3d0621..3f8f286 100644
--- a/video/config/video_encoder_config.h
+++ b/video/config/video_encoder_config.h
@@ -166,6 +166,8 @@
~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 a9391e0..4579702 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -734,6 +734,14 @@
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 @@
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 @@
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 9aade03..e208501 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -6302,6 +6302,74 @@
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;
