From db7e82bdfbac68ad82372edaad6505210885b4ca Mon Sep 17 00:00:00 2001 From: Harry Terkelsen <1961493+harryterkelsen@users.noreply.github.com> Date: Thu, 6 Feb 2025 15:59:30 -0800 Subject: [PATCH] chore(canvaskit): remove SurfaceFrame from Surface (#162825) SurfaceFrame is no longer used. This is a cleanup in preparation for a larger refactor of `Surface` to improve performance. ## 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 | 71 ++++++---------- .../web_ui/test/canvaskit/surface_test.dart | 83 +++++++++++-------- 2 files changed, 74 insertions(+), 80 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 e058b5f5fc..3d8c16f443 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 @@ -23,27 +23,6 @@ import 'util.dart'; // removes the ability for disabling AA on Paint objects. const bool _kUsingMSAA = bool.fromEnvironment('flutter.canvaskit.msaa'); -typedef SubmitCallback = bool Function(SurfaceFrame, CkCanvas); - -/// A frame which contains a canvas to be drawn into. -class SurfaceFrame { - SurfaceFrame(this.skiaSurface, this.submitCallback) : _submitted = false; - - final CkSurface skiaSurface; - final SubmitCallback submitCallback; - final bool _submitted; - - /// Submit this frame to be drawn. - bool submit() { - if (_submitted) { - return false; - } - return submitCallback(this, skiaCanvas); - } - - CkCanvas get skiaCanvas => skiaSurface.getCanvas(); -} - /// A surface which can be drawn into by the compositor. /// /// The underlying representation is a [CkSurface], which can be reused by @@ -55,6 +34,19 @@ class Surface extends DisplayCanvas { CkSurface? _surface; + /// Returns the underlying CanvasKit Surface. Should only be used in tests. + CkSurface? debugGetCkSurface() { + bool assertsEnabled = false; + assert(() { + assertsEnabled = true; + return true; + }()); + if (!assertsEnabled) { + throw StateError('debugGetCkSurface() can only be used in tests'); + } + return _surface; + } + /// Whether or not to use an `OffscreenCanvas` to back this [Surface]. final bool useOffscreenCanvas; @@ -97,7 +89,17 @@ class Surface extends DisplayCanvas { DomOffscreenCanvas? _offscreenCanvas; /// Returns the underlying OffscreenCanvas. Should only be used in tests. - DomOffscreenCanvas? get debugOffscreenCanvas => _offscreenCanvas; + DomOffscreenCanvas? debugGetOffscreenCanvas() { + bool assertsEnabled = false; + assert(() { + assertsEnabled = true; + return true; + }()); + if (!assertsEnabled) { + throw StateError('debugGetOffscreenCanvas() can only be used in tests'); + } + return _offscreenCanvas; + } /// The backing this Surface in the case that OffscreenCanvas isn't /// supported. @@ -172,20 +174,6 @@ class Surface extends DisplayCanvas { } } - /// Acquire a frame of the given [size] containing a drawable canvas. - /// - /// The given [size] is in physical pixels. - SurfaceFrame acquireFrame(ui.Size size) { - final CkSurface surface = createOrUpdateSurface(BitmapSize.fromSize(size)); - - // ignore: prefer_function_declarations_over_variables - final SubmitCallback submitCallback = (SurfaceFrame surfaceFrame, CkCanvas canvas) { - return _presentSurface(); - }; - - return SurfaceFrame(surface, submitCallback); - } - BitmapSize? _currentCanvasPhysicalSize; /// Sets the CSS size of the canvas so that canvas pixels are 1:1 with device @@ -283,9 +271,9 @@ 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. - + // If we reached here, then this is the first frame and we haven't made a + // surface yet, we are forcing a new context, or the size of the surface + // has changed and we need to make a new one. _surface?.dispose(); _surface = null; @@ -489,11 +477,6 @@ class Surface extends DisplayCanvas { } } - bool _presentSurface() { - _surface!.flush(); - return true; - } - @override bool get isConnected => _canvasElement!.isConnected!; 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 a7875bfa68..d141f4053a 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 @@ -25,8 +25,9 @@ void testMain() { test('Surface allocates canvases efficiently', () { final Surface surface = Surface(); - final CkSurface originalSurface = surface.acquireFrame(const ui.Size(9, 19)).skiaSurface; - final DomOffscreenCanvas original = surface.debugOffscreenCanvas!; + surface.createOrUpdateSurface(const BitmapSize(9, 19)); + final CkSurface originalSurface = surface.debugGetCkSurface()!; + final DomOffscreenCanvas original = surface.debugGetOffscreenCanvas()!; // Expect exact requested dimensions. expect(original.width, 9); @@ -36,8 +37,9 @@ void testMain() { // 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!; + surface.createOrUpdateSurface(const BitmapSize(5, 15)); + final CkSurface shrunkSurface = surface.debugGetCkSurface()!; + final DomOffscreenCanvas shrunk = surface.debugGetOffscreenCanvas()!; expect(shrunk, same(original)); expect(shrunkSurface, isNot(same(originalSurface))); expect(shrunkSurface.width(), 5); @@ -45,9 +47,9 @@ void testMain() { // 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!; + surface.createOrUpdateSurface(const BitmapSize(10, 20)); + final CkSurface firstIncreaseSurface = surface.debugGetCkSurface()!; + final DomOffscreenCanvas firstIncrease = surface.debugGetOffscreenCanvas()!; expect(firstIncrease, same(original)); expect(firstIncreaseSurface, isNot(same(shrunkSurface))); @@ -58,17 +60,18 @@ void testMain() { expect(firstIncreaseSurface.height(), 20); // 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!; + surface.createOrUpdateSurface(const BitmapSize(11, 22)); + final CkSurface secondIncreaseSurface = surface.debugGetCkSurface()!; + final DomOffscreenCanvas secondIncrease = surface.debugGetOffscreenCanvas()!; expect(secondIncrease, same(firstIncrease)); expect(secondIncreaseSurface, isNot(same(firstIncreaseSurface))); expect(secondIncreaseSurface.width(), 11); expect(secondIncreaseSurface.height(), 22); // Increases beyond the 40% limit will cause a new allocation. - final CkSurface hugeSurface = surface.acquireFrame(const ui.Size(20, 40)).skiaSurface; - final DomOffscreenCanvas huge = surface.debugOffscreenCanvas!; + surface.createOrUpdateSurface(const BitmapSize(20, 40)); + final CkSurface hugeSurface = surface.debugGetCkSurface()!; + final DomOffscreenCanvas huge = surface.debugGetOffscreenCanvas()!; expect(huge, same(secondIncrease)); expect(hugeSurface, isNot(same(secondIncreaseSurface))); @@ -79,8 +82,9 @@ void testMain() { expect(hugeSurface.height(), 40); // Shrink again. Create a new surface. - final CkSurface shrunkSurface2 = surface.acquireFrame(const ui.Size(5, 15)).skiaSurface; - final DomOffscreenCanvas shrunk2 = surface.debugOffscreenCanvas!; + surface.createOrUpdateSurface(const BitmapSize(5, 15)); + final CkSurface shrunkSurface2 = surface.debugGetCkSurface()!; + final DomOffscreenCanvas shrunk2 = surface.debugGetOffscreenCanvas()!; expect(shrunk2, same(huge)); expect(shrunkSurface2, isNot(same(hugeSurface))); expect(shrunkSurface2.width(), 5); @@ -89,12 +93,13 @@ void testMain() { // Doubling the DPR should halve the CSS width, height, and translation of the canvas. // This tests https://github.com/flutter/flutter/issues/77084 EngineFlutterDisplay.instance.debugOverrideDevicePixelRatio(2.0); - final CkSurface dpr2Surface2 = surface.acquireFrame(const ui.Size(5, 15)).skiaSurface; - final DomOffscreenCanvas dpr2Canvas = surface.debugOffscreenCanvas!; + surface.createOrUpdateSurface(const BitmapSize(5, 15)); + final CkSurface dpr2Surface = surface.debugGetCkSurface()!; + final DomOffscreenCanvas dpr2Canvas = surface.debugGetOffscreenCanvas()!; expect(dpr2Canvas, same(huge)); - expect(dpr2Surface2, isNot(same(hugeSurface))); - expect(dpr2Surface2.width(), 5); - expect(dpr2Surface2.height(), 15); + expect(dpr2Surface, isNot(same(hugeSurface))); + expect(dpr2Surface.width(), 5); + expect(dpr2Surface.height(), 15); // Skipping on Firefox for now since Firefox headless doesn't support WebGL // This causes issues in the test since we create a Canvas-backed surface, @@ -195,17 +200,19 @@ void testMain() { () async { final Surface surface = Surface(); expect(surface.debugForceNewContext, isTrue); - final CkSurface before = surface.acquireFrame(const ui.Size(9, 19)).skiaSurface; + surface.createOrUpdateSurface(const BitmapSize(9, 19)); + final CkSurface before = surface.debugGetCkSurface()!; expect(surface.debugForceNewContext, isFalse); // Pump a timer to flush any microtasks. await Future.delayed(Duration.zero); - final CkSurface afterAcquireFrame = surface.acquireFrame(const ui.Size(9, 19)).skiaSurface; + surface.createOrUpdateSurface(const BitmapSize(9, 19)); + final CkSurface afterAcquireFrame = surface.debugGetCkSurface()!; // Existing context is reused. expect(afterAcquireFrame, same(before)); // Emulate WebGL context loss. - final DomOffscreenCanvas canvas = surface.debugOffscreenCanvas!; + final DomOffscreenCanvas canvas = surface.debugGetOffscreenCanvas()!; final Object ctx = canvas.getContext('webgl2')!; final Object loseContextExtension = js_util.callMethod(ctx, 'getExtension', [ 'WEBGL_lose_context', @@ -226,7 +233,8 @@ void testMain() { await Future.delayed(Duration.zero); expect(surface.debugForceNewContext, isTrue); - final CkSurface afterContextLost = surface.acquireFrame(const ui.Size(9, 19)).skiaSurface; + surface.createOrUpdateSurface(const BitmapSize(9, 19)); + final CkSurface afterContextLost = surface.debugGetCkSurface()!; // A new context is created. expect(afterContextLost, isNot(same(before))); }, @@ -239,39 +247,42 @@ void testMain() { 'updates canvas logical size when device-pixel ratio changes', () { final Surface surface = Surface(); - final CkSurface original = surface.acquireFrame(const ui.Size(10, 16)).skiaSurface; + surface.createOrUpdateSurface(const BitmapSize(10, 16)); + final CkSurface original = surface.debugGetCkSurface()!; expect(original.width(), 10); expect(original.height(), 16); - expect(surface.debugOffscreenCanvas!.width, 10); - expect(surface.debugOffscreenCanvas!.height, 16); + expect(surface.debugGetOffscreenCanvas()!.width, 10); + expect(surface.debugGetOffscreenCanvas()!.height, 16); // Increase device-pixel ratio: this makes CSS pixels bigger, so we need // fewer of them to cover the browser window. EngineFlutterDisplay.instance.debugOverrideDevicePixelRatio(2.0); - final CkSurface highDpr = surface.acquireFrame(const ui.Size(10, 16)).skiaSurface; + surface.createOrUpdateSurface(const BitmapSize(10, 16)); + final CkSurface highDpr = surface.debugGetCkSurface()!; expect(highDpr.width(), 10); expect(highDpr.height(), 16); - expect(surface.debugOffscreenCanvas!.width, 10); - expect(surface.debugOffscreenCanvas!.height, 16); + expect(surface.debugGetOffscreenCanvas()!.width, 10); + expect(surface.debugGetOffscreenCanvas()!.height, 16); // Decrease device-pixel ratio: this makes CSS pixels smaller, so we need // more of them to cover the browser window. EngineFlutterDisplay.instance.debugOverrideDevicePixelRatio(0.5); - final CkSurface lowDpr = surface.acquireFrame(const ui.Size(10, 16)).skiaSurface; + surface.createOrUpdateSurface(const BitmapSize(10, 16)); + final CkSurface lowDpr = surface.debugGetCkSurface()!; expect(lowDpr.width(), 10); expect(lowDpr.height(), 16); - expect(surface.debugOffscreenCanvas!.width, 10); - expect(surface.debugOffscreenCanvas!.height, 16); + expect(surface.debugGetOffscreenCanvas()!.width, 10); + expect(surface.debugGetOffscreenCanvas()!.height, 16); // See https://github.com/flutter/flutter/issues/77084#issuecomment-1120151172 EngineFlutterDisplay.instance.debugOverrideDevicePixelRatio(2.0); - final CkSurface changeRatioAndSize = - surface.acquireFrame(const ui.Size(9.9, 15.9)).skiaSurface; + surface.createOrUpdateSurface(BitmapSize.fromSize(const ui.Size(9.9, 15.9))); + final CkSurface changeRatioAndSize = surface.debugGetCkSurface()!; expect(changeRatioAndSize.width(), 10); expect(changeRatioAndSize.height(), 16); - expect(surface.debugOffscreenCanvas!.width, 10); - expect(surface.debugOffscreenCanvas!.height, 16); + expect(surface.debugGetOffscreenCanvas()!.width, 10); + expect(surface.debugGetOffscreenCanvas()!.height, 16); }, skip: !Surface.offscreenCanvasSupported, );