From 57ec58b82d0ec7c4a025b2521868ea9d74cb661f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 2 Oct 2024 09:36:06 +0200 Subject: [PATCH] VideoAdapter: Fix zooming issue with requested_resolution API. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When AdaptFrameResolution() applies the requested resolution as a restriction (max width and max height) it does so on the "input" size rather than on the "output" size. While this results in the correct output size anyway, it also produces cropping which results in the image looking zoomed in (see https://crbug.com/webrtc/369865055 for repro). To fix this issue the restrict logic is moved and applied on the "output" instead. The logic is updated to take alignment into account since the resulting size is the final output. Bug: webrtc:369865055 Change-Id: I2d5476929432c45173a57c0f4964ab9a38518189 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/364163 Commit-Queue: Henrik Boström Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#43138} --- media/base/video_adapter.cc | 51 +++++++++++++++------------- media/base/video_adapter_unittest.cc | 31 ++++++++++++++--- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/media/base/video_adapter.cc b/media/base/video_adapter.cc index 35bd722fbd..22ce693cbe 100644 --- a/media/base/video_adapter.cc +++ b/media/base/video_adapter.cc @@ -211,28 +211,6 @@ bool VideoAdapter::AdaptFrameResolution(int in_width, 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(in_width), - requested_resolution.height / static_cast(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) { @@ -259,12 +237,39 @@ bool VideoAdapter::AdaptFrameResolution(int in_width, RTC_DCHECK_EQ(0, *cropped_width % scale.denominator); RTC_DCHECK_EQ(0, *cropped_height % scale.denominator); - // Calculate final output size. + // Calculate output size. *out_width = *cropped_width / scale.denominator * scale.numerator; *out_height = *cropped_height / scale.denominator * scale.numerator; RTC_DCHECK_EQ(0, *out_width % resolution_alignment_); RTC_DCHECK_EQ(0, *out_height % resolution_alignment_); + // Lastly, make the output size fit within the resolution restrictions as + // specified by `requested_resolution_`. This does not modify aspect ratio or + // cropping, only `out_width` and `out_height`. + if (requested_resolution_.has_value()) { + // Make frame and requested resolution have matching orientation. + webrtc::Resolution requested_resolution = requested_resolution_.value(); + if ((*out_width < *out_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 (*out_width > 0 && *out_height > 0 && + (requested_resolution.width < *out_width || + requested_resolution.height < *out_height)) { + double scale_factor = std::min( + requested_resolution.width / static_cast(*out_width), + requested_resolution.height / static_cast(*out_height)); + *out_width = roundUp(std::round(*out_width * scale_factor), + resolution_alignment_, requested_resolution.width); + *out_height = roundUp(std::round(*out_height * scale_factor), + resolution_alignment_, requested_resolution.height); + RTC_DCHECK_EQ(0, *out_width % resolution_alignment_); + RTC_DCHECK_EQ(0, *out_height % resolution_alignment_); + } + } + ++frames_out_; if (scale.numerator != scale.denominator) ++frames_scaled_; diff --git a/media/base/video_adapter_unittest.cc b/media/base/video_adapter_unittest.cc index 404d8e7f7d..98dd5b9b27 100644 --- a/media/base/video_adapter_unittest.cc +++ b/media/base/video_adapter_unittest.cc @@ -1308,10 +1308,14 @@ TEST_P(VideoAdapterTest, RequestedResolutionMaintainsAspectRatio) { /* 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})); + EXPECT_TRUE(adapter_.AdaptFrameResolution(1280, 720, /*in_timestamp_ns=*/0, + &cropped_width_, &cropped_height_, + &out_width_, &out_height_)); + EXPECT_EQ(out_width_, 720); + EXPECT_EQ(out_height_, 405); + // No cropping needed (coverage for https://crbug.com/webrtc/369865055). + EXPECT_EQ(cropped_width_, 1280); + EXPECT_EQ(cropped_height_, 720); } class VideoAdapterWithSourceAlignmentTest : public VideoAdapterTest { @@ -1361,6 +1365,25 @@ TEST_P(VideoAdapterWithSourceAlignmentTest, AdaptResolutionWithSinkAlignment) { EXPECT_EQ(out_height_ % kSinkResolutionAlignment, 0); } +TEST_P(VideoAdapterWithSourceAlignmentTest, + RequestedResolutionMaintainsAspectRatioWithAlignment) { + // Request 720x720. + adapter_.OnSinkWants( + BuildSinkWants(Resolution{.width = 720, .height = 720}, + /* any_active_without_requested_resolution= */ false)); + + // A 1280x720 frame restricted to 720x720 produces 720x405 but this is not a + // multiple of `kSourceResolutionAlignment` (= 7), the rounded up multiple of + // this value that is less than the restrictions (720) is 714x406. + EXPECT_TRUE(adapter_.AdaptFrameResolution(1280, 720, /*in_timestamp_ns=*/0, + &cropped_width_, &cropped_height_, + &out_width_, &out_height_)); + EXPECT_EQ(out_width_, 714); + EXPECT_EQ(out_height_, 406); + EXPECT_EQ(out_width_ % kSourceResolutionAlignment, 0); + EXPECT_EQ(out_height_ % kSourceResolutionAlignment, 0); +} + INSTANTIATE_TEST_SUITE_P(OnOutputFormatRequests, VideoAdapterWithSourceAlignmentTest, ::testing::Values(true, false));