From 6c38b2ef0e5121e2c0baf777b17bae0cd3389547 Mon Sep 17 00:00:00 2001 From: Jordan Bayles Date: Fri, 23 Apr 2021 18:44:07 +0000 Subject: [PATCH] [Tab Capture] Honor capture size aspect ratio This patch changes the WCVCD preferred size calculation to honor the aspect ratio of the capture size. If the requested capture size is larger than the view/screen size, it is scaled to fit within the bounds of the screen but with the same resolution. Bug: 1194803 Change-Id: If74320f382643748983bcff9371b351922238e74 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2844952 Commit-Queue: Jordan Bayles Reviewed-by: mark a. foltz Cr-Commit-Position: refs/heads/master@{#875773} --- .../capture/web_contents_frame_tracker.cc | 28 ++++- .../web_contents_frame_tracker_unittest.cc | 111 +++++++++++++++++- 2 files changed, 130 insertions(+), 9 deletions(-) diff --git a/content/browser/media/capture/web_contents_frame_tracker.cc b/content/browser/media/capture/web_contents_frame_tracker.cc index 2ef5491cc4f0fe..02a991aa13b64a 100644 --- a/content/browser/media/capture/web_contents_frame_tracker.cc +++ b/content/browser/media/capture/web_contents_frame_tracker.cc @@ -35,7 +35,7 @@ namespace content { namespace { -// Note on lifetime: this context should be deleted when the web contents +// Note on lifetime: this context should be deleted when the WebContents // is destroyed. class WebContentsContext : public WebContentsFrameTracker::Context { public: @@ -81,7 +81,7 @@ class WebContentsContext : public WebContentsFrameTracker::Context { base::ScopedClosureRunner capture_handle_; - // The backing web contents. + // The backing WebContents. WebContents* contents_; }; @@ -138,6 +138,11 @@ void WebContentsFrameTracker::DidStopCapturingWebContents() { // the first capturer's preferred size is set. gfx::Size WebContentsFrameTracker::CalculatePreferredSize( const gfx::Size& capture_size) { + if (capture_size.IsEmpty()) { + // NOTE: An empty preferred size will cause the WebContents to keep its + // previous size preference. + return {}; + } gfx::Size preferred_size = capture_size; // If we know the available size of the screen, we don't want to exceed @@ -145,7 +150,24 @@ gfx::Size WebContentsFrameTracker::CalculatePreferredSize( if (context_) { const base::Optional screen_bounds = context_->GetScreenBounds(); if (screen_bounds) { - preferred_size.SetToMin(screen_bounds->size()); + if (screen_bounds->size().IsEmpty()) { + return {}; + } + + // We want to honor the aspect ratio of the capture size request while + // also limiting it to the screen bounds of the view. + // For motivation, see https://crbug.com/1194803. + const double x_ratio = static_cast(capture_size.width()) / + static_cast(screen_bounds->size().width()); + const double y_ratio = + static_cast(capture_size.height()) / + static_cast(screen_bounds->size().height()); + + const double scale_ratio = std::max(x_ratio, y_ratio); + if (scale_ratio > 1.0) { + preferred_size = gfx::ScaleToFlooredSize( + preferred_size, static_cast(1 / scale_ratio)); + } } } return preferred_size; diff --git a/content/browser/media/capture/web_contents_frame_tracker_unittest.cc b/content/browser/media/capture/web_contents_frame_tracker_unittest.cc index 8ddefa29f5c89a..d278e115de37c8 100644 --- a/content/browser/media/capture/web_contents_frame_tracker_unittest.cc +++ b/content/browser/media/capture/web_contents_frame_tracker_unittest.cc @@ -25,8 +25,11 @@ using testing::Invoke; using testing::NiceMock; using testing::StrictMock; -constexpr gfx::Size kSize720p = gfx::Size(1280, 720); -constexpr gfx::Size kSize1080p = gfx::Size(1920, 1080); +// Standardized screen resolutions to test common scenarios. +constexpr gfx::Size kSizeZero{0, 0}; +constexpr gfx::Size kSize720p{1280, 720}; +constexpr gfx::Size kSize1080p{1920, 1080}; +constexpr gfx::Size kSizeWsxgaPlus{1680, 1050}; class SimpleContext : public WebContentsFrameTracker::Context { public: @@ -103,6 +106,7 @@ class WebContentsFrameTrackerTest : public RenderViewHostTestHarness { void SetUpOnUIThread() { auto context = std::make_unique(); raw_context_ = context.get(); + SetScreenSize(kSize1080p); tracker_->SetWebContentsAndContextForTesting(web_contents_.get(), std::move(context)); SetFrameSinkId(viz::FrameSinkId(123, 456)); @@ -173,7 +177,6 @@ class WebContentsFrameTrackerTest : public RenderViewHostTestHarness { TEST_F(WebContentsFrameTrackerTest, CalculatesPreferredSizeClampsToView) { SetScreenSize(kSize720p); - RunAllTasksUntilIdle(); EXPECT_EQ(kSize720p, tracker()->CalculatePreferredSize(kSize720p)); EXPECT_EQ(kSize720p, tracker()->CalculatePreferredSize(kSize1080p)); } @@ -181,14 +184,109 @@ TEST_F(WebContentsFrameTrackerTest, CalculatesPreferredSizeClampsToView) { TEST_F(WebContentsFrameTrackerTest, CalculatesPreferredSizeNoLargerThanCaptureSize) { SetScreenSize(kSize1080p); - RunAllTasksUntilIdle(); - EXPECT_EQ(kSize720p, tracker()->CalculatePreferredSize(kSize720p)); EXPECT_EQ(kSize1080p, tracker()->CalculatePreferredSize(kSize1080p)); } -TEST_F(WebContentsFrameTrackerTest, UpdatesPreferredSizeOnWebContents) { +TEST_F(WebContentsFrameTrackerTest, + CalculatesPreferredSizeWithCorrectAspectRatio) { + SetScreenSize(kSizeWsxgaPlus); + + // 720P is strictly less than WSXGA+, so should be unchanged. + EXPECT_EQ(kSize720p, tracker()->CalculatePreferredSize(kSize720p)); + + // 1080P is larger, so should be scaled appropriately. + EXPECT_EQ((gfx::Size{1680, 945}), + tracker()->CalculatePreferredSize(kSize1080p)); + + // Wider capture size. + EXPECT_EQ((gfx::Size{1680, 525}), + tracker()->CalculatePreferredSize(gfx::Size{3360, 1050})); + + // Taller capture size. + EXPECT_EQ((gfx::Size{500, 1050}), + tracker()->CalculatePreferredSize(gfx::Size{1000, 2100})); +} + +TEST_F(WebContentsFrameTrackerTest, + CalculatesPreferredSizeAspectRatioWithNoOffByOneErrors) { + SetScreenSize(kSizeWsxgaPlus); + + // Wider capture size. + EXPECT_EQ((gfx::Size{1680, 525}), + tracker()->CalculatePreferredSize(gfx::Size{3360, 1050})); + EXPECT_EQ((gfx::Size{1680, 525}), + tracker()->CalculatePreferredSize(gfx::Size{3360, 1051})); + EXPECT_EQ((gfx::Size{1680, 526}), + tracker()->CalculatePreferredSize(gfx::Size{3360, 1052})); + EXPECT_EQ((gfx::Size{1680, 525}), + tracker()->CalculatePreferredSize(gfx::Size{3361, 1052})); + EXPECT_EQ((gfx::Size{1680, 666}), + tracker()->CalculatePreferredSize(gfx::Size{5897, 2339})); + + // Taller capture size. + EXPECT_EQ((gfx::Size{500, 1050}), + tracker()->CalculatePreferredSize(gfx::Size{1000, 2100})); + EXPECT_EQ((gfx::Size{499, 1050}), + tracker()->CalculatePreferredSize(gfx::Size{1000, 2101})); + EXPECT_EQ((gfx::Size{499, 1050}), + tracker()->CalculatePreferredSize(gfx::Size{1000, 2102})); + EXPECT_EQ((gfx::Size{500, 1050}), + tracker()->CalculatePreferredSize(gfx::Size{1001, 2102})); + EXPECT_EQ((gfx::Size{500, 1050}), + tracker()->CalculatePreferredSize(gfx::Size{1002, 2102})); + + // Some larger and prime factor cases to sanity check. + EXPECT_EQ((gfx::Size{1680, 565}), + tracker()->CalculatePreferredSize(gfx::Size{21841, 7351})); + EXPECT_EQ((gfx::Size{1680, 565}), + tracker()->CalculatePreferredSize(gfx::Size{21841, 7349})); + EXPECT_EQ((gfx::Size{1680, 565}), + tracker()->CalculatePreferredSize(gfx::Size{21839, 7351})); + EXPECT_EQ((gfx::Size{1680, 565}), + tracker()->CalculatePreferredSize(gfx::Size{21839, 7349})); + + EXPECT_EQ((gfx::Size{1680, 670}), + tracker()->CalculatePreferredSize(gfx::Size{139441, 55651})); + EXPECT_EQ((gfx::Size{1680, 670}), + tracker()->CalculatePreferredSize(gfx::Size{139439, 55651})); + EXPECT_EQ((gfx::Size{1680, 670}), + tracker()->CalculatePreferredSize(gfx::Size{139441, 55649})); + EXPECT_EQ((gfx::Size{1680, 670}), + tracker()->CalculatePreferredSize(gfx::Size{139439, 55649})); + + // Finally, just check for roundoff errors. + SetScreenSize(gfx::Size{1000, 1000}); + EXPECT_EQ((gfx::Size{1000, 333}), + tracker()->CalculatePreferredSize(gfx::Size{3000, 1000})); +} + +TEST_F(WebContentsFrameTrackerTest, + CalculatesPreferredSizeLeavesCaptureSizeIfSmallerThanScreen) { + // Smaller in both directions, but different aspect ratio, should be + // unchanged. SetScreenSize(kSize1080p); + EXPECT_EQ(kSizeWsxgaPlus, tracker()->CalculatePreferredSize(kSizeWsxgaPlus)); +} + +TEST_F(WebContentsFrameTrackerTest, + CalculatesPreferredSizeWithZeroValueProperly) { + // If a capture dimension is zero, the preferred size should be (0, 0). + EXPECT_EQ((kSizeZero), tracker()->CalculatePreferredSize(gfx::Size{0, 1000})); + EXPECT_EQ((kSizeZero), tracker()->CalculatePreferredSize(kSizeZero)); + EXPECT_EQ((kSizeZero), tracker()->CalculatePreferredSize(gfx::Size{1000, 0})); + + // If a screen dimension is zero, the preferred size should be (0, 0). This + // probably means the tab isn't being drawn anyway. + SetScreenSize(gfx::Size{1920, 0}); + EXPECT_EQ(kSizeZero, tracker()->CalculatePreferredSize(kSize720p)); + SetScreenSize(gfx::Size{0, 1080}); + EXPECT_EQ(kSizeZero, tracker()->CalculatePreferredSize(kSize720p)); + SetScreenSize(kSizeZero); + EXPECT_EQ(kSizeZero, tracker()->CalculatePreferredSize(kSize720p)); +} + +TEST_F(WebContentsFrameTrackerTest, UpdatesPreferredSizeOnWebContents) { StartTrackerOnUIThread(kSize720p); RunAllTasksUntilIdle(); @@ -196,6 +294,7 @@ TEST_F(WebContentsFrameTrackerTest, UpdatesPreferredSizeOnWebContents) { // so it should be used. EXPECT_EQ(kSize720p, context()->last_capture_size()); EXPECT_EQ(context()->capturer_count(), 1); + // When we stop the tracker, the web contents issues a preferred size change // of the "old" size--so it shouldn't change. StopTrackerOnUIThread();