Avoid signaling requested_resolution back to the adapting source.
When requested_resolution uses a different aspect ratio than the source
the encoder will restrict the frame without changing its aspect ratio,
e.g. a 60x30 input frame that is restricted to 30x30 results in 30x15,
not 30x30.
While this logic works correctly in isolation, if the source also adapts
the frame size based on the sink_wants.requested_resolution that is
signaled back to the source, then the source will produce stretched
30x30 prior to the encoder which happily sends 30x30 not knowing any
wiser.
This is incompatible with the spec[1] and makes this WPT[2] fail. The
correct behavior is to NOT signal the requested_resolution back to the
source, the encoder already configures the correct resolution so this
isn't actually needed and the source shouldn't need to know this API.
In order not to break downstream projects, the new behavior is landed
behind a flag and both behaviors are tested with TEST_P.
This unblocks launching scaleResolutionDownTo API on Web. Migrating
from old to new code path and deleting the flag is a follow-up AI:
webrtc:366284861.
[1] https://w3c.github.io/webrtc-extensions/#dom-rtcrtpencodingparameters-scaleresolutiondownto
[2] https://chromium-review.googlesource.com/c/chromium/src/+/5853944
# Relying on previous green runs for confidence due to purple bots atm,
# see b/367211396
NOTRY=True
NOPRESUBMIT=True
Bug: webrtc:366067962, webrtc:366284861
Change-Id: I7fd1016e9cc6f0b0b9b8c23b0708e521f8e12642
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362541
Reviewed-by: Henrik Andreassson <henrika@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43024}
diff --git a/api/video/video_stream_encoder_settings.h b/api/video/video_stream_encoder_settings.h
index c8cb368..e49305b 100644
--- a/api/video/video_stream_encoder_settings.h
+++ b/api/video/video_stream_encoder_settings.h
@@ -57,6 +57,25 @@
// Enables the frame instrumentation generator that is required for automatic
// corruption detection.
bool enable_frame_instrumentation_generator = false;
+
+ // According to spec, `requested_resolution` (called scaleResolutionDownTo in
+ // the web API) MUST NOT modify the aspect ratio of the frame, e.g. a 1280x720
+ // frame being restricted to maxWidth by maxHeight 720x720 should result in
+ // 720x405. In order for this to work, the video source must not adapt the
+ // input frame to the value of `requested_resolution`, as that would result in
+ // stretched 720x720.
+ //
+ // In order not to break backwards compatibility with C++ usage of this API,
+ // when `use_standard_requested_resolution` is false, the
+ // `requested_resolution` is signaled back to the video source. This works as
+ // long as the aspect ratio is the same, but breaks the web API use case.
+ //
+ // Spec:
+ // https://w3c.github.io/webrtc-extensions/#dom-rtcrtpencodingparameters-scaleresolutiondownto
+ //
+ // TODO(https://crbug.com/webrtc/366284861): Change the default to true,
+ // delete this flag and any code handling the legacy behavior.
+ bool use_standard_requested_resolution = false;
};
} // namespace webrtc
diff --git a/media/base/video_adapter.cc b/media/base/video_adapter.cc
index 487845d..bb1515b 100644
--- a/media/base/video_adapter.cc
+++ b/media/base/video_adapter.cc
@@ -342,12 +342,6 @@
resolution_alignment_ = cricket::LeastCommonMultiple(
source_resolution_alignment_, sink_wants.resolution_alignment);
- if (!sink_wants.aggregates) {
- RTC_LOG(LS_WARNING)
- << "These should always be created by VideoBroadcaster!";
- return;
- }
-
// If requested_resolution is used, and there are no active encoders
// that are NOT using requested_resolution (aka newapi), then override
// calls to OnOutputFormatRequest and use values from requested_resolution
@@ -365,7 +359,14 @@
return;
}
- if (sink_wants.aggregates->any_active_without_requested_resolution) {
+ // The code below is only needed when `requested_resolution` is signalled back
+ // to the video source which only happens if
+ // `VideoStreamEncoderSettings::use_standard_requested_resolution` is false.
+ // TODO(https://crbug.com/webrtc/366284861): Delete the code below as part of
+ // deleting this flag and only supporting the standard behavior.
+
+ if (sink_wants.aggregates.has_value() &&
+ sink_wants.aggregates->any_active_without_requested_resolution) {
return;
}
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index da42b2a..61b9483 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -891,12 +891,18 @@
}
}
}
- if (requested_resolution !=
- video_source_sink_controller_.requested_resolution() ||
+ const bool signal_requested_resolution =
+ !settings_.use_standard_requested_resolution;
+ if ((signal_requested_resolution &&
+ requested_resolution !=
+ video_source_sink_controller_.requested_resolution()) ||
active != video_source_sink_controller_.active() ||
max_framerate !=
video_source_sink_controller_.frame_rate_upper_limit().value_or(-1)) {
- video_source_sink_controller_.SetRequestedResolution(requested_resolution);
+ if (signal_requested_resolution) {
+ video_source_sink_controller_.SetRequestedResolution(
+ requested_resolution);
+ }
if (max_framerate >= 0) {
video_source_sink_controller_.SetFrameRateUpperLimit(max_framerate);
} else {
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index 573b6a5..f5c4212 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -2530,23 +2530,121 @@
video_stream_encoder_->Stop();
}
-TEST_F(VideoStreamEncoderTest,
- ResolutionLimitPropagatedToSinkWantsBeforeFirstFrame) {
+// Tests both the standard and non-standard version of `requested_resolution`
+// (also known as scaleResolutionDownTo) which behave differently with regards
+// to signaling `requested_resolution` back to the source.
+enum class RequestedResolutionVersion {
+ kLegacyRequestedResolution,
+ kStandardRequestedResolution,
+};
+class VideoStreamEncoderStandardOrLegacyRequestedResolutionTest
+ : public VideoStreamEncoderTest,
+ public ::testing::WithParamInterface<RequestedResolutionVersion> {
+ public:
+ VideoStreamEncoderStandardOrLegacyRequestedResolutionTest()
+ : VideoStreamEncoderTest(), requested_resolution_version_(GetParam()) {}
+
+ void SetUp() override {
+ VideoStreamEncoderTest::SetUp();
+ // Configure `use_standard_requested_resolution` based on test param.
+ switch (requested_resolution_version_) {
+ case RequestedResolutionVersion::kLegacyRequestedResolution:
+ video_send_config_.encoder_settings.use_standard_requested_resolution =
+ false;
+ break;
+ case RequestedResolutionVersion::kStandardRequestedResolution:
+ video_send_config_.encoder_settings.use_standard_requested_resolution =
+ true;
+ break;
+ }
+
+ // Use a real video stream factory or else `requested_resolution` is not
+ // applied correctly.
+ video_encoder_config_.video_stream_factory = nullptr;
+
+ ConfigureEncoder(video_encoder_config_.Copy());
+
+ video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources(
+ kTargetBitrate, kTargetBitrate, kTargetBitrate, 0, 0, 0);
+ }
+
+ protected:
+ const RequestedResolutionVersion requested_resolution_version_;
+};
+
+INSTANTIATE_TEST_SUITE_P(
+ VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
+ VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
+ ::testing::Values(
+ RequestedResolutionVersion::kLegacyRequestedResolution,
+ RequestedResolutionVersion::kStandardRequestedResolution));
+
+TEST_P(VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
+ ResolutionLimitMaybePropagatedToSinkWantsBeforeFirstFrame) {
ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1));
video_encoder_config_.simulcast_layers[0].requested_resolution.emplace(
Resolution({.width = 320, .height = 160}));
video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
kMaxPayloadLength);
- EXPECT_EQ(video_source_.sink_wants().requested_resolution,
- rtc::VideoSinkWants::FrameSize(320, 160));
+
+ // The legacy path signals `requested_resolution` back to the source. The
+ // standard path does not.
+ switch (requested_resolution_version_) {
+ case RequestedResolutionVersion::kLegacyRequestedResolution:
+ EXPECT_EQ(video_source_.sink_wants().requested_resolution,
+ rtc::VideoSinkWants::FrameSize(320, 160));
+ break;
+ case RequestedResolutionVersion::kStandardRequestedResolution:
+ EXPECT_FALSE(video_source_.sink_wants().requested_resolution.has_value());
+ break;
+ }
video_encoder_config_.simulcast_layers[0].requested_resolution->height = 320;
video_encoder_config_.simulcast_layers[0].requested_resolution->width = 640;
video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
kMaxPayloadLength);
- EXPECT_EQ(video_source_.sink_wants().requested_resolution,
- rtc::VideoSinkWants::FrameSize(640, 320));
+
+ switch (requested_resolution_version_) {
+ case RequestedResolutionVersion::kLegacyRequestedResolution:
+ EXPECT_EQ(video_source_.sink_wants().requested_resolution,
+ rtc::VideoSinkWants::FrameSize(640, 320));
+ break;
+ case RequestedResolutionVersion::kStandardRequestedResolution:
+ EXPECT_FALSE(video_source_.sink_wants().requested_resolution.has_value());
+ break;
+ }
+
+ video_stream_encoder_->Stop();
+}
+
+TEST_P(VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
+ RequestedResolutionInWrongAspectRatioAndSourceIsAdapting) {
+ // Use a source that adapts resolution based on OnSinkWants.
+ AdaptingFrameForwarder source(&time_controller_);
+ source.set_adaptation_enabled(true);
+ video_stream_encoder_->SetSource(
+ &source, webrtc::DegradationPreference::MAINTAIN_FRAMERATE);
+
+ ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1));
+ video_encoder_config_.simulcast_layers[0].requested_resolution = {
+ .width = 30, .height = 30};
+ video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
+ kMaxPayloadLength);
+
+ // Capture a 60x30 frame.
+ source.IncomingCapturedFrame(CreateFrame(1, 60, 30));
+ // The 60x30 frame does not fit inside the 30x30 restrictions.
+ // - The non-standard path produces 30x30 due to OnSinkWants() signaling.
+ // - The standard path produces 30x15, maintaining aspect ratio.
+ switch (requested_resolution_version_) {
+ case RequestedResolutionVersion::kLegacyRequestedResolution:
+ WaitForEncodedFrame(30, 30);
+ break;
+ case RequestedResolutionVersion::kStandardRequestedResolution:
+ WaitForEncodedFrame(30, 15);
+ break;
+ }
video_stream_encoder_->Stop();
}
