VideoAdapter: Interpret requested resolution as max restriction.
The `requested_resolution` API must not change aspect ratio, example:
- Frame is 60x30
- Requested is 30x30
- We expect 30x15 (not 30x30!) as to maintain aspect ratio.
This bug was previously fixed by making VideoAdapter unaware of the
requested resolution behind a flag: this seemed OK since the
VideoStreamEncoder ultimately decides the resolution, whether or not
the incoming frame is adapted.
But this is not desired for some non-Chrome use cases. This CL attempts
to make both Chrome and non-Chrome use cases happy by implementing the
aspect ratio preserving restriction inside VideoAdapter too.
This allows us to get rid of the "use_standard_requested_resolution"
flag and change the "VideoStreamEncoderResolutionTest" TEST_P to
TEST_F.
Bug: webrtc:366067962, webrtc:366284861
Change-Id: I1dfd10963274c5fdfd18d0f4443b2f209d2e9a4b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362720
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Henrik Andreassson <henrika@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43037}
diff --git a/api/video/video_stream_encoder_settings.h b/api/video/video_stream_encoder_settings.h
index e49305b..c8cb368 100644
--- a/api/video/video_stream_encoder_settings.h
+++ b/api/video/video_stream_encoder_settings.h
@@ -57,25 +57,6 @@
// 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/BUILD.gn b/media/BUILD.gn
index 08e924a..c39f75c 100644
--- a/media/BUILD.gn
+++ b/media/BUILD.gn
@@ -92,6 +92,7 @@
"../api/transport/rtp:rtp_source",
"../api/units:time_delta",
"../api/video:recordable_encoded_frame",
+ "../api/video:resolution",
"../api/video:video_bitrate_allocation",
"../api/video:video_bitrate_allocator_factory",
"../api/video:video_frame",
@@ -159,6 +160,7 @@
]
deps = [
":video_common",
+ "../api/video:resolution",
"../api/video:video_frame",
"../common_video",
"../rtc_base:checks",
diff --git a/media/base/video_adapter.cc b/media/base/video_adapter.cc
index bb1515b..35bd722 100644
--- a/media/base/video_adapter.cc
+++ b/media/base/video_adapter.cc
@@ -211,6 +211,28 @@
return false;
}
+ // Adjust input width and height without modifying aspect ratio as to be less
+ // than or equal to the `requested_resolution_`.
+ if (requested_resolution_.has_value()) {
+ // Make frame and requested resolution have matching orientation.
+ webrtc::Resolution requested_resolution = requested_resolution_.value();
+ if ((in_width < in_height) !=
+ (requested_resolution_->width < requested_resolution_->height)) {
+ requested_resolution = {.width = requested_resolution_->height,
+ .height = requested_resolution_->width};
+ }
+ // Downscale by smallest scaling factor, if necessary.
+ if (in_width > 0 && in_height > 0 &&
+ (requested_resolution.width < in_width ||
+ requested_resolution.height < in_height)) {
+ double scale_factor = std::min(
+ requested_resolution.width / static_cast<double>(in_width),
+ requested_resolution.height / static_cast<double>(in_height));
+ in_width = std::round(in_width * scale_factor);
+ in_height = std::round(in_height * scale_factor);
+ }
+ }
+
// Calculate how the input should be cropped.
if (!target_aspect_ratio || target_aspect_ratio->first <= 0 ||
target_aspect_ratio->second <= 0) {
@@ -341,6 +363,13 @@
max_framerate_request_ = sink_wants.max_framerate_fps;
resolution_alignment_ = cricket::LeastCommonMultiple(
source_resolution_alignment_, sink_wants.resolution_alignment);
+ // Convert from std::optional<rtc::VideoSinkWants::FrameSize> to
+ // std::optional<webrtc::Resolution>. Both are {int,int}.
+ requested_resolution_ = std::nullopt;
+ if (sink_wants.requested_resolution.has_value()) {
+ requested_resolution_ = {.width = sink_wants.requested_resolution->width,
+ .height = sink_wants.requested_resolution->height};
+ }
// If requested_resolution is used, and there are no active encoders
// that are NOT using requested_resolution (aka newapi), then override
@@ -371,7 +400,7 @@
}
if (!stashed_output_format_request_) {
- // The active output format request is about to be rewritten by
+ // The active output format request is about to be cleared due to
// request_resolution. We need to save it for later use in case the encoder
// which doesn't use request_resolution logic become active in the future.
stashed_output_format_request_ = output_format_request_;
@@ -379,22 +408,9 @@
<< stashed_output_format_request_->ToString();
}
- auto res = *sink_wants.requested_resolution;
- if (res.width < res.height) {
- // Adjust `res` to landscape mode.
- res.width = sink_wants.requested_resolution->height;
- res.height = sink_wants.requested_resolution->width;
- }
- auto pixel_count = res.width * res.height;
- output_format_request_.target_landscape_aspect_ratio =
- std::make_pair(res.width, res.height);
- output_format_request_.max_landscape_pixel_count = pixel_count;
- output_format_request_.target_portrait_aspect_ratio =
- std::make_pair(res.height, res.width);
- output_format_request_.max_portrait_pixel_count = pixel_count;
- output_format_request_.max_fps = max_framerate_request_;
- RTC_LOG(LS_INFO) << "Setting output_format_request_ based on sink_wants: "
- << output_format_request_.ToString();
+ // Clear the output format request. Requested resolution will be applied
+ // instead which happens inside AdaptFrameResolution().
+ output_format_request_ = {};
}
int VideoAdapter::GetTargetPixels() const {
diff --git a/media/base/video_adapter.h b/media/base/video_adapter.h
index 35ed672..4cc4a38 100644
--- a/media/base/video_adapter.h
+++ b/media/base/video_adapter.h
@@ -17,6 +17,7 @@
#include <string>
#include <utility>
+#include "api/video/resolution.h"
#include "api/video/video_source_interface.h"
#include "common_video/framerate_controller.h"
#include "media/base/video_common.h"
@@ -148,6 +149,8 @@
int resolution_request_target_pixel_count_ RTC_GUARDED_BY(mutex_);
int resolution_request_max_pixel_count_ RTC_GUARDED_BY(mutex_);
int max_framerate_request_ RTC_GUARDED_BY(mutex_);
+ std::optional<webrtc::Resolution> requested_resolution_
+ RTC_GUARDED_BY(mutex_);
// Stashed OutputFormatRequest that is used to save value of
// OnOutputFormatRequest in case all active encoders are using
diff --git a/media/base/video_adapter_unittest.cc b/media/base/video_adapter_unittest.cc
index 5011438..404d8e7 100644
--- a/media/base/video_adapter_unittest.cc
+++ b/media/base/video_adapter_unittest.cc
@@ -59,13 +59,8 @@
wants.resolution_alignment = 1;
wants.is_active = true;
if (requested_resolution) {
- wants.target_pixel_count = requested_resolution->PixelCount();
- wants.max_pixel_count = requested_resolution->PixelCount();
wants.requested_resolution.emplace(rtc::VideoSinkWants::FrameSize(
requested_resolution->width, requested_resolution->height));
- } else {
- wants.target_pixel_count = kWidth * kHeight;
- wants.max_pixel_count = kWidth * kHeight;
}
wants.aggregates.emplace(rtc::VideoSinkWants::Aggregates());
wants.aggregates->any_active_without_requested_resolution =
@@ -1306,6 +1301,19 @@
}
}
+TEST_P(VideoAdapterTest, RequestedResolutionMaintainsAspectRatio) {
+ // Request 720x720.
+ adapter_.OnSinkWants(
+ BuildSinkWants(Resolution{.width = 720, .height = 720},
+ /* any_active_without_requested_resolution= */ false));
+
+ // A 1280x720 frame restricted to 720x720 produces 720x405.
+ EXPECT_THAT(
+ AdaptFrameResolution(/* input frame */ {.width = 1280, .height = 720})
+ .first,
+ Eq(Resolution{.width = 720, .height = 405}));
+}
+
class VideoAdapterWithSourceAlignmentTest : public VideoAdapterTest {
protected:
static constexpr int kSourceResolutionAlignment = 7;
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 61b9483..da42b2a 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -891,18 +891,12 @@
}
}
}
- const bool signal_requested_resolution =
- !settings_.use_standard_requested_resolution;
- if ((signal_requested_resolution &&
- requested_resolution !=
- video_source_sink_controller_.requested_resolution()) ||
+ if (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)) {
- if (signal_requested_resolution) {
- video_source_sink_controller_.SetRequestedResolution(
- 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 bf56316..9e0626c 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -2530,56 +2530,14 @@
video_stream_encoder_->Stop();
}
-// 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 VideoStreamEncoderResolutionTest
- : public VideoStreamEncoderTest,
- public ::testing::WithParamInterface<RequestedResolutionVersion> {
- public:
- VideoStreamEncoderResolutionTest()
- : VideoStreamEncoderTest(), requested_resolution_version_(GetParam()) {}
+TEST_F(VideoStreamEncoderTest, RequestInSinkWantsBeforeFirstFrame) {
+ // 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);
- 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(
- VideoStreamEncoderResolutionTest,
- VideoStreamEncoderResolutionTest,
- ::testing::Values(
- RequestedResolutionVersion::kLegacyRequestedResolution,
- RequestedResolutionVersion::kStandardRequestedResolution));
-
-TEST_P(VideoStreamEncoderResolutionTest, RequestInSinkWantsBeforeFirstFrame) {
ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1));
video_encoder_config_.simulcast_layers[0].requested_resolution.emplace(
Resolution({.width = 320, .height = 160}));
@@ -2587,37 +2545,28 @@
video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
kMaxPayloadLength);
- // 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;
- }
+ EXPECT_EQ(video_source_.sink_wants().requested_resolution,
+ rtc::VideoSinkWants::FrameSize(320, 160));
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);
- 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;
- }
+ EXPECT_EQ(video_source_.sink_wants().requested_resolution,
+ rtc::VideoSinkWants::FrameSize(640, 320));
video_stream_encoder_->Stop();
}
-TEST_P(VideoStreamEncoderResolutionTest, RequestInWrongAspectRatioWithAdapter) {
+TEST_F(VideoStreamEncoderTest, RequestInWrongAspectRatioWithAdapter) {
+ // 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);
+
// Use a source that adapts resolution based on OnSinkWants.
AdaptingFrameForwarder source(&time_controller_);
source.set_adaptation_enabled(true);
@@ -2633,16 +2582,8 @@
// 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;
- }
+ // Expect 30x15, maintaining aspect ratio.
+ WaitForEncodedFrame(30, 15);
video_stream_encoder_->Stop();
}
