From 843a3173f26a823927ab4163f581ad2aeaf6b244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Tue, 3 Sep 2024 09:55:03 +0200 Subject: [PATCH] Fix requested_resolution orientation assumption in OnSinkWants(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The VideoAdapter is used to configure encoding resolutions based on requested_resolution in an orientation agnostic way[1]. This means that if you request 1280x720 and the input frame is 720x1280, there is no downscale happening. However in the same file there is one instance of VideoAdapter::OnSinkWants() where requested_resolution is assumed to be expressed in landscape mode. This breaks the case where the 720x1280 is requested but the frame is 1280x720 which causes inconsistent behavior and breaks symmetry. This would also break simulcast since this code path is only applied with the top layer's requested resolution while the lower layers are still scaled in an agnostic way. A new test is added to verify the fix. Prior to the fix, the first half of the test was passing, after the fix both parts of the test pass. [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/media/base/video_adapter.h;l=76;drc=02b5b024b66755a851a752b7851b124ba03f6cb6 Bug: webrtc:363019836 Change-Id: I564068e98c93cab89eb38a10b0f8378899438e5b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/361160 Commit-Queue: Henrik Boström Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#42923} --- media/base/video_adapter.cc | 5 +++++ media/base/video_adapter_unittest.cc | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/media/base/video_adapter.cc b/media/base/video_adapter.cc index 1d0beb5959..487845d2dc 100644 --- a/media/base/video_adapter.cc +++ b/media/base/video_adapter.cc @@ -379,6 +379,11 @@ void VideoAdapter::OnSinkWants(const rtc::VideoSinkWants& sink_wants) { } 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); diff --git a/media/base/video_adapter_unittest.cc b/media/base/video_adapter_unittest.cc index aab96dcf82..5011438363 100644 --- a/media/base/video_adapter_unittest.cc +++ b/media/base/video_adapter_unittest.cc @@ -1281,6 +1281,31 @@ TEST_P(VideoAdapterTest, UseRequestedResolutionInsteadOfOnOutputFormatRequest) { } } +TEST_P(VideoAdapterTest, RequestedResolutionIsOrientationAgnostic) { + // Request 1280x720 when frame is 720x1280. + { + adapter_.OnSinkWants( + BuildSinkWants(Resolution{.width = 1280, .height = 720}, + /* any_active_without_requested_resolution= */ false)); + + EXPECT_THAT( + AdaptFrameResolution(/* input frame */ {.width = 720, .height = 1280}) + .first, + Eq(Resolution{.width = 720, .height = 1280})); + } + // Request 720x1280 when frame is 1280x720. + { + adapter_.OnSinkWants( + BuildSinkWants(Resolution{.width = 720, .height = 1280}, + /* any_active_without_requested_resolution= */ false)); + + EXPECT_THAT( + AdaptFrameResolution(/* input frame */ {.width = 1280, .height = 720}) + .first, + Eq(Resolution{.width = 1280, .height = 720})); + } +} + class VideoAdapterWithSourceAlignmentTest : public VideoAdapterTest { protected: static constexpr int kSourceResolutionAlignment = 7;