From 1b095a030d7037d9b3a31cad7b2149e368260991 Mon Sep 17 00:00:00 2001 From: Harry Terkelsen <1961493+harryterkelsen@users.noreply.github.com> Date: Thu, 6 Feb 2025 12:11:37 -0800 Subject: [PATCH] [canvaskit] Resize to exactly the requested dimensions (#162708) Adds a `SurfaceResizeStrategy` to `Surface`. The default option, `onlyGrow`, implements the current behavior of only re-allocating the Surface when the requested size is larger than the currently allocated Surface. This PR adds the option of `SurfaceResizeStrategy.exact` which always reallocates the Surface and resizes the underlying OffscreenCanvas. This options performs better in practice since having the OffScreenCanvas larger than the actual rendered size causes calls to `createImageBitmap` to be slower than if the OffscreenCanvas is exactly the size of the bitmap being created. Fixes https://github.com/flutter/flutter/issues/162700 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../lib/src/engine/canvaskit/surface.dart | 25 +++--- .../web_ui/test/canvaskit/surface_test.dart | 88 +++++++++---------- 2 files changed, 56 insertions(+), 57 deletions(-) diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/surface.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/surface.dart index 69041aee75..e058b5f5fc 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/surface.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/surface.dart @@ -237,7 +237,8 @@ class Surface extends DisplayCanvas { } // TODO(jonahwilliams): this is somewhat wasteful. We should probably // eagerly setup this surface instead of delaying until the first frame? - // Or at least cache the estimated window sizeThis is the first frame we have rendered with this canvas. + // Or at least cache the estimated window size. + // This is the first frame we have rendered with this canvas. createOrUpdateSurface(size); } @@ -261,16 +262,13 @@ class Surface extends DisplayCanvas { return _surface!; } - final BitmapSize? previousCanvasSize = _currentCanvasPhysicalSize; - // Initialize a new, larger, canvas. If the size is growing, then make the - // new canvas larger than required to avoid many canvas creations. - if (previousCanvasSize != null && - (size.width > previousCanvasSize.width || size.height > previousCanvasSize.height)) { - final BitmapSize newSize = BitmapSize.fromSize(size.toSize() * 1.4); + if (_currentCanvasPhysicalSize != null && + (size.width != _currentCanvasPhysicalSize!.width || + size.height != _currentCanvasPhysicalSize!.height)) { _surface?.dispose(); _surface = null; - _pixelWidth = newSize.width; - _pixelHeight = newSize.height; + _pixelWidth = size.width; + _pixelHeight = size.height; if (useOffscreenCanvas) { _offscreenCanvas!.width = _pixelWidth.toDouble(); _offscreenCanvas!.height = _pixelHeight.toDouble(); @@ -285,10 +283,14 @@ class Surface extends DisplayCanvas { } } + // If we reached here, then either we are forcing a new context, or + // the size of the surface has changed so we need to make a new one. + + _surface?.dispose(); + _surface = null; + // Either a new context is being forced or we've never had one. if (_forceNewContext || _currentCanvasPhysicalSize == null) { - _surface?.dispose(); - _surface = null; _grContext?.releaseResourcesAndAbandonContext(); _grContext?.delete(); _grContext = null; @@ -297,7 +299,6 @@ class Surface extends DisplayCanvas { _currentCanvasPhysicalSize = size; } - _surface?.dispose(); return _surface = _createNewSurface(size); } diff --git a/engine/src/flutter/lib/web_ui/test/canvaskit/surface_test.dart b/engine/src/flutter/lib/web_ui/test/canvaskit/surface_test.dart index 8861d8a1fe..a7875bfa68 100644 --- a/engine/src/flutter/lib/web_ui/test/canvaskit/surface_test.dart +++ b/engine/src/flutter/lib/web_ui/test/canvaskit/surface_test.dart @@ -34,8 +34,8 @@ void testMain() { expect(originalSurface.width(), 9); expect(originalSurface.height(), 19); - // Shrinking reuses the existing canvas but translates it so - // Skia renders into the visible area. + // Shrinking causes the surface to create a new canvas with the exact + // size requested. final CkSurface shrunkSurface = surface.acquireFrame(const ui.Size(5, 15)).skiaSurface; final DomOffscreenCanvas shrunk = surface.debugOffscreenCanvas!; expect(shrunk, same(original)); @@ -43,21 +43,21 @@ void testMain() { expect(shrunkSurface.width(), 5); expect(shrunkSurface.height(), 15); - // The first increase will allocate a new surface, but will overallocate - // by 40% to accommodate future increases. + // The first increase will allocate a new surface to exactly the + // requested size. final CkSurface firstIncreaseSurface = surface.acquireFrame(const ui.Size(10, 20)).skiaSurface; final DomOffscreenCanvas firstIncrease = surface.debugOffscreenCanvas!; expect(firstIncrease, same(original)); expect(firstIncreaseSurface, isNot(same(shrunkSurface))); - // Expect overallocated dimensions - expect(firstIncrease.width, 14); - expect(firstIncrease.height, 28); + // Expect exact dimensions + expect(firstIncrease.width, 10); + expect(firstIncrease.height, 20); expect(firstIncreaseSurface.width(), 10); expect(firstIncreaseSurface.height(), 20); - // Subsequent increases within 40% reuse the old canvas. + // Subsequent increases within 40% will still allocate a new canvas. final CkSurface secondIncreaseSurface = surface.acquireFrame(const ui.Size(11, 22)).skiaSurface; final DomOffscreenCanvas secondIncrease = surface.debugOffscreenCanvas!; @@ -72,13 +72,13 @@ void testMain() { expect(huge, same(secondIncrease)); expect(hugeSurface, isNot(same(secondIncreaseSurface))); - // Also over-allocated - expect(huge.width, 28); - expect(huge.height, 56); + // Also exactly-allocated + expect(huge.width, 20); + expect(huge.height, 40); expect(hugeSurface.width(), 20); expect(hugeSurface.height(), 40); - // Shrink again. Reuse the last allocated surface. + // Shrink again. Create a new surface. final CkSurface shrunkSurface2 = surface.acquireFrame(const ui.Size(5, 15)).skiaSurface; final DomOffscreenCanvas shrunk2 = surface.debugOffscreenCanvas!; expect(shrunk2, same(huge)); @@ -116,64 +116,62 @@ void testMain() { expect(canvasSize.width, 9); expect(canvasSize.height, 19); - // Shrinking reuses the existing canvas but translates it so - // Skia renders into the visible area. + // Shrinking causes us to resize the canvas. surface.createOrUpdateSurface(const BitmapSize(5, 15)); final DomCanvasElement shrunk = getDisplayCanvas(surface); canvasSize = getCssSize(surface); - expect(shrunk.width, 9); - expect(shrunk.height, 19); - expect(canvasSize.width, 9); - expect(canvasSize.height, 19); + expect(shrunk.width, 5); + expect(shrunk.height, 15); + expect(canvasSize.width, 5); + expect(canvasSize.height, 15); - // The first increase will allocate a new surface, but will overallocate - // by 40% to accommodate future increases. + // Increasing the size causes us to resize the canvas. surface.createOrUpdateSurface(const BitmapSize(10, 20)); final DomCanvasElement firstIncrease = getDisplayCanvas(surface); canvasSize = getCssSize(surface); expect(firstIncrease, same(original)); - // Expect overallocated dimensions - expect(firstIncrease.width, 14); - expect(firstIncrease.height, 28); - expect(canvasSize.width, 14); - expect(canvasSize.height, 28); + // Expect exact dimensions + expect(firstIncrease.width, 10); + expect(firstIncrease.height, 20); + expect(canvasSize.width, 10); + expect(canvasSize.height, 20); - // Subsequent increases within 40% reuse the old canvas. + // Subsequent increases also cause canvas resizing. surface.createOrUpdateSurface(const BitmapSize(11, 22)); final DomCanvasElement secondIncrease = getDisplayCanvas(surface); canvasSize = getCssSize(surface); expect(secondIncrease, same(firstIncrease)); - expect(secondIncrease.width, 14); - expect(secondIncrease.height, 28); - expect(canvasSize.width, 14); - expect(canvasSize.height, 28); + expect(secondIncrease.width, 11); + expect(secondIncrease.height, 22); + expect(canvasSize.width, 11); + expect(canvasSize.height, 22); - // Increases beyond the 40% limit will cause a new allocation. + // Increases beyond the 40% limit will cause a canvas resize. surface.createOrUpdateSurface(const BitmapSize(20, 40)); final DomCanvasElement huge = getDisplayCanvas(surface); canvasSize = getCssSize(surface); expect(huge, same(secondIncrease)); - // Also over-allocated - expect(huge.width, 28); - expect(huge.height, 56); - expect(canvasSize.width, 28); - expect(canvasSize.height, 56); + // Also exact + expect(huge.width, 20); + expect(huge.height, 40); + expect(canvasSize.width, 20); + expect(canvasSize.height, 40); - // Shrink again. Reuse the last allocated surface. + // Shrink again. Resize the canvas. surface.createOrUpdateSurface(const BitmapSize(5, 15)); final DomCanvasElement shrunk2 = getDisplayCanvas(surface); canvasSize = getCssSize(surface); expect(shrunk2, same(huge)); - expect(shrunk2.width, 28); - expect(shrunk2.height, 56); - expect(canvasSize.width, 28); - expect(canvasSize.height, 56); + expect(shrunk2.width, 5); + expect(shrunk2.height, 15); + expect(canvasSize.width, 5); + expect(canvasSize.height, 15); // Doubling the DPR should halve the CSS width, height, and translation of the canvas. // This tests https://github.com/flutter/flutter/issues/77084 @@ -183,12 +181,12 @@ void testMain() { canvasSize = getCssSize(surface); expect(dpr2Canvas, same(huge)); - expect(dpr2Canvas.width, 28); - expect(dpr2Canvas.height, 56); + expect(dpr2Canvas.width, 5); + expect(dpr2Canvas.height, 15); // Canvas is half the size in logical pixels because device pixel ratio is // 2.0. - expect(canvasSize.width, 14); - expect(canvasSize.height, 28); + expect(canvasSize.width, 2.5); + expect(canvasSize.height, 7.5); // Skip on wasm since same() doesn't work for JSValues. }, skip: isWasm);