From a26ceed9149d6c8733468889fd8b0a903426a155 Mon Sep 17 00:00:00 2001 From: Harry Terkelsen Date: Thu, 7 Dec 2023 16:23:05 -0800 Subject: [PATCH 1/3] Make CanvasKitRenderer listen for view creation/disposal events --- .../src/engine/canvaskit/embedded_views.dart | 4 +- .../lib/src/engine/canvaskit/rasterizer.dart | 10 ++- .../canvaskit/render_canvas_factory.dart | 6 +- .../lib/src/engine/canvaskit/renderer.dart | 64 +++++++++++++------ .../view_embedder/flutter_view_manager.dart | 29 ++++++--- lib/web_ui/test/canvaskit/common.dart | 4 -- .../test/canvaskit/embedded_views_test.dart | 37 ++++++++++- .../test/canvaskit/multi_view_test.dart | 42 ++++++++++-- .../canvaskit/render_canvas_factory_test.dart | 2 +- .../flutter_view_manager_test.dart | 24 +++++-- 10 files changed, 169 insertions(+), 53 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index f7a8dcfaa5e65..cecfa354219c1 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -657,8 +657,8 @@ class HtmlViewEmbedder { element.remove(); } - /// Clears the state of this view embedder. Used in tests. - void debugClear() { + /// Disposes the state of this view embedder. + void dispose() { final Set allViews = PlatformViewManager.instance.debugClear(); disposeViews(allViews); _context = EmbedderFrameContext(); diff --git a/lib/web_ui/lib/src/engine/canvaskit/rasterizer.dart b/lib/web_ui/lib/src/engine/canvaskit/rasterizer.dart index 104d96109fa23..d9280f28b6d7e 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/rasterizer.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/rasterizer.dart @@ -43,8 +43,8 @@ class Rasterizer { final CkPictureRecorder pictureRecorder = CkPictureRecorder(); pictureRecorder.beginRecording(ui.Offset.zero & _currentFrameSize); pictureRecorder.recordingCanvas!.clear(const ui.Color(0x00000000)); - final Frame compositorFrame = context.acquireFrame( - pictureRecorder.recordingCanvas!, viewEmbedder); + final Frame compositorFrame = + context.acquireFrame(pictureRecorder.recordingCanvas!, viewEmbedder); compositorFrame.raster(layerTree, ignoreRasterCache: true); @@ -54,4 +54,10 @@ class Rasterizer { viewEmbedder.submitFrame(); } + + /// Disposes of this rasterizer. + void dispose() { + viewEmbedder.dispose(); + renderCanvasFactory.dispose(); + } } diff --git a/lib/web_ui/lib/src/engine/canvaskit/render_canvas_factory.dart b/lib/web_ui/lib/src/engine/canvaskit/render_canvas_factory.dart index 387b63156524b..e814c9e6fe7e9 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/render_canvas_factory.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/render_canvas_factory.dart @@ -9,7 +9,7 @@ import '../../engine.dart'; class RenderCanvasFactory { RenderCanvasFactory() { assert(() { - registerHotRestartListener(debugClear); + registerHotRestartListener(dispose); return true; }()); } @@ -102,8 +102,8 @@ class RenderCanvasFactory { return false; } - /// Dispose all canvases created by this factory. Used in tests. - void debugClear() { + /// Dispose all canvases created by this factory. + void dispose() { for (final RenderCanvas canvas in _cache) { canvas.dispose(); } diff --git a/lib/web_ui/lib/src/engine/canvaskit/renderer.dart b/lib/web_ui/lib/src/engine/canvaskit/renderer.dart index 1caf88ffaa80e..567fac6849883 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/renderer.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/renderer.dart @@ -55,6 +55,11 @@ class CanvasKitRenderer implements Renderer { /// is supported. final Surface pictureToImageSurface = Surface(); + // Listens for view creation events from the view manager. + StreamSubscription? _onViewCreatedListener; + // Listens for view disposal events from the view manager. + StreamSubscription? _onViewDisposedListener; + @override Future initialize() async { _initialized ??= () async { @@ -64,6 +69,20 @@ class CanvasKitRenderer implements Renderer { canvasKit = await downloadCanvasKit(); windowFlutterCanvasKit = canvasKit; } + // Views may have been registered before this renderer was initialized. + // Create rasterizers for them and then start listening for new view + // creation/disposal events. + final FlutterViewManager viewManager = + EnginePlatformDispatcher.instance.viewManager; + if (_onViewCreatedListener == null) { + for (final EngineFlutterView view in viewManager.views) { + _onViewCreated(view.viewId); + } + } + _onViewCreatedListener ??= + viewManager.onViewCreated.listen(_onViewCreated); + _onViewDisposedListener ??= + viewManager.onViewDisposed.listen(_onViewDisposed); _instance = this; }(); return _initialized; @@ -394,37 +413,44 @@ class CanvasKitRenderer implements Renderer { frameTimingsOnBuildFinish(); frameTimingsOnRasterStart(); - // TODO(harryterkelsen): Use `FlutterViewManager.onViewsChanged` to manage - // the lifecycle of Rasterizers, - // https://github.com/flutter/flutter/issues/137073. - final Rasterizer rasterizer = - _getRasterizerForView(view as EngineFlutterView); + assert(_rasterizers.containsKey(view.viewId), + "Unable to render to a view which hasn't been registered"); + final Rasterizer rasterizer = _rasterizers[view.viewId]!; rasterizer.draw((scene as LayerScene).layerTree); frameTimingsOnRasterFinish(); } - final Map _rasterizers = - {}; + // Map from view id to the associated Rasterizer for that view. + final Map _rasterizers = {}; - Rasterizer _getRasterizerForView(EngineFlutterView view) { - return _rasterizers.putIfAbsent(view, () { - return Rasterizer(view); - }); + void _onViewCreated(int viewId) { + final EngineFlutterView view = + EnginePlatformDispatcher.instance.viewManager[viewId]!; + _rasterizers[view.viewId] = Rasterizer(view); + } + + void _onViewDisposed(int viewId) { + // The view has already been disposed. + if (!_rasterizers.containsKey(viewId)) { + return; + } + final Rasterizer rasterizer = _rasterizers.remove(viewId)!; + rasterizer.dispose(); } - /// Returns the [Rasterizer] that has been created for the given [view]. - /// Used in tests. - Rasterizer debugGetRasterizerForView(EngineFlutterView view) { - return _getRasterizerForView(view); + Rasterizer? debugGetRasterizerForView(EngineFlutterView view) { + return _rasterizers[view.viewId]; } - /// Resets the state of the renderer. Used in tests. - void debugClear() { + /// Disposes this renderer. + void dispose() { + _onViewCreatedListener?.cancel(); + _onViewDisposedListener?.cancel(); for (final Rasterizer rasterizer in _rasterizers.values) { - rasterizer.renderCanvasFactory.debugClear(); - rasterizer.viewEmbedder.debugClear(); + rasterizer.dispose(); } + _rasterizers.clear(); } @override diff --git a/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart b/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart index 0024c059da4ed..de23077826b54 100644 --- a/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart +++ b/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart @@ -15,12 +15,18 @@ class FlutterViewManager { // A map of (optional) JsFlutterViewOptions, indexed by their viewId. final Map _jsViewOptions = {}; - // The controller of the [onViewsChanged] stream. - final StreamController _onViewsChangedController = - StreamController.broadcast(); + // The controller of the [onViewCreated] stream. + final StreamController _onViewCreatedController = + StreamController.broadcast(sync: true); + // The controller of the [onViewDisposed] stream. + final StreamController _onViewDisposedController = + StreamController.broadcast(sync: true); - /// A stream of `void` events that will fire when a view is registered/unregistered. - Stream get onViewsChanged => _onViewsChangedController.stream; + /// A stream of viewIds that will fire when a view is created. + Stream get onViewCreated => _onViewCreatedController.stream; + + /// A stream of viewIds that will fire when a view is disposed. + Stream get onViewDisposed => _onViewDisposedController.stream; /// Exposes all the [EngineFlutterView]s registered so far. Iterable get views => _viewData.values; @@ -33,7 +39,8 @@ class FlutterViewManager { EngineFlutterView createAndRegisterView( JsFlutterViewOptions jsViewOptions, ) { - final EngineFlutterView view = EngineFlutterView(_dispatcher, jsViewOptions.hostElement); + final EngineFlutterView view = + EngineFlutterView(_dispatcher, jsViewOptions.hostElement); registerView(view, jsViewOptions: jsViewOptions); return view; } @@ -53,7 +60,7 @@ class FlutterViewManager { if (jsViewOptions != null) { _jsViewOptions[viewId] = jsViewOptions; } - _onViewsChangedController.add(null); + _onViewCreatedController.add(viewId); return view; } @@ -74,7 +81,7 @@ class FlutterViewManager { JsFlutterViewOptions? unregisterView(int viewId) { _viewData.remove(viewId); // .dispose(); final JsFlutterViewOptions? jsViewOptions = _jsViewOptions.remove(viewId); - _onViewsChangedController.add(null); + _onViewDisposedController.add(viewId); return jsViewOptions; } @@ -90,8 +97,10 @@ class FlutterViewManager { // We need to call `toList()` in order to avoid concurrent modification // inside the loop. _viewData.keys.toList().forEach(disposeAndUnregisterView); + _viewData.clear(); // Let listeners receive the unregistration events from the loop above, then - // close the stream. - _onViewsChangedController.close(); + // close the streams. + _onViewCreatedController.close(); + _onViewDisposedController.close(); } } diff --git a/lib/web_ui/test/canvaskit/common.dart b/lib/web_ui/test/canvaskit/common.dart index d11dc2e04c871..72f5e64e652cb 100644 --- a/lib/web_ui/test/canvaskit/common.dart +++ b/lib/web_ui/test/canvaskit/common.dart @@ -22,10 +22,6 @@ void setUpCanvasKitTest() { setUpTestViewDimensions: false, ); - tearDown(() { - CanvasKitRenderer.instance.debugClear(); - }); - setUp(() => renderer.fontCollection.fontFallbackManager!.downloadQueue .fallbackFontUrlPrefixOverride = 'assets/fallback_fonts/'); tearDown(() => renderer.fontCollection.fontFallbackManager!.downloadQueue diff --git a/lib/web_ui/test/canvaskit/embedded_views_test.dart b/lib/web_ui/test/canvaskit/embedded_views_test.dart index 7884b85aeb8a8..d35bba6f8eff9 100644 --- a/lib/web_ui/test/canvaskit/embedded_views_test.dart +++ b/lib/web_ui/test/canvaskit/embedded_views_test.dart @@ -61,6 +61,8 @@ void testMain() { reason: 'The slot reenables pointer events.'); expect(contentsHost.getAttribute('slot'), slot.getAttribute('name'), reason: 'The contents and slot are correctly related.'); + + await disposePlatformView(0); }); test('clips platform views with RRects', () async { @@ -101,6 +103,8 @@ void testMain() { sceneHost.querySelectorAll('flt-clip').single.style.height, '100%', ); + + await disposePlatformView(0); }); test('correctly transforms platform views', () async { @@ -131,6 +135,8 @@ void testMain() { // 503 (5 * 100 + 3). 'matrix3d(5, 0, 0, 0, 0, 5, 0, 0, 0, 0, 5, 0, 515, 515, 0, 1)', ); + + await disposePlatformView(0); }); test('correctly offsets platform views', () async { @@ -157,6 +163,8 @@ void testMain() { expect(slotRect.top, 4); expect(slotRect.right, 8); expect(slotRect.bottom, 10); + + await disposePlatformView(0); }); // Returns the list of CSS transforms applied to the ancestor chain of @@ -189,8 +197,7 @@ void testMain() { CanvasKitRenderer.instance.renderScene(sb.build(), implicitView); // Transformations happen on the slot element. - DomElement slotHost = - sceneHost.querySelector('flt-platform-view-slot')!; + DomElement slotHost = sceneHost.querySelector('flt-platform-view-slot')!; expect( getTransformChain(slotHost), @@ -220,6 +227,8 @@ void testMain() { 'matrix(1, 0, 0, 1, 3, 3)', ], ); + + await disposePlatformView(0); }); test('converts device pixels to logical pixels (no clips)', () async { @@ -245,6 +254,8 @@ void testMain() { getTransformChain(slotHost), ['matrix(0.25, 0, 0, 0.25, 1.5, 1.5)'], ); + + await disposePlatformView(0); }); test('converts device pixels to logical pixels (with clips)', () async { @@ -276,6 +287,8 @@ void testMain() { 'matrix(0.25, 0, 0, 0.25, 0.75, 0.75)', ], ); + + await disposePlatformView(0); }); test('renders overlays on top of platform views', () async { @@ -428,6 +441,10 @@ void testMain() { await createPlatformView(0, 'test-platform-view'); renderTestScene(viewCount: 0); _expectSceneMatches(<_EmbeddedViewMarker>[_overlay]); + + for (int i = 0; i < 16; i++) { + await disposePlatformView(i); + } // TODO(yjbanov): skipped due to https://github.com/flutter/flutter/issues/73867 }, skip: isSafari); @@ -561,6 +578,10 @@ void testMain() { _overlay, ]); + for (int i = 0; i < 20; i++) { + await disposePlatformView(i); + } + // TODO(yjbanov): skipped due to https://github.com/flutter/flutter/issues/73867 }, skip: isSafari); @@ -641,6 +662,8 @@ void testMain() { implicitView.debugPhysicalSizeOverride = null; implicitView.debugForceResize(); + + await disposePlatformView(0); // ImageDecoder is not supported in Safari or Firefox. }, skip: isSafari || isFirefox); @@ -695,6 +718,9 @@ void testMain() { platformViewsHost.querySelectorAll('flt-platform-view'), hasLength(2), ); + + await disposePlatformView(0); + await disposePlatformView(1); }); test( @@ -729,6 +755,8 @@ void testMain() { await Future.delayed(Duration.zero); renderTestScene(); expect(skPathDefs.childNodes, hasLength(1)); + + await disposePlatformView(0); }); test('does not crash when a prerolled platform view is not composited', @@ -749,6 +777,8 @@ void testMain() { _expectSceneMatches(<_EmbeddedViewMarker>[ _overlay, ]); + + await disposePlatformView(0); }); test('does not create overlays for invisible platform views', () async { @@ -951,6 +981,9 @@ void testMain() { _platformView, _overlay, ]); + for (int i = 0; i < 7; i++) { + await disposePlatformView(i); + } }); }); } diff --git a/lib/web_ui/test/canvaskit/multi_view_test.dart b/lib/web_ui/test/canvaskit/multi_view_test.dart index 2aa1815b67ea6..a5a67f473f4aa 100644 --- a/lib/web_ui/test/canvaskit/multi_view_test.dart +++ b/lib/web_ui/test/canvaskit/multi_view_test.dart @@ -7,6 +7,7 @@ import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; +import '../common/matchers.dart'; import 'common.dart'; void main() { @@ -17,22 +18,55 @@ void testMain() { group('CanvasKit', () { setUpCanvasKitTest(); - test('can render into arbitrary views', () async { + late LayerScene scene; + + setUp(() { + // Create a scene to use in tests. final CkPicture picture = paintPicture(const ui.Rect.fromLTRB(0, 0, 60, 60), (CkCanvas canvas) { canvas.drawRect(const ui.Rect.fromLTRB(0, 0, 60, 60), CkPaint()..style = ui.PaintingStyle.fill); }); - final LayerSceneBuilder sb = LayerSceneBuilder(); - sb.addPicture(ui.Offset.zero, picture); - final LayerScene scene = sb.build(); + scene = sb.build(); + }); + + test('can render into arbitrary views', () async { CanvasKitRenderer.instance.renderScene(scene, implicitView); final EngineFlutterView anotherView = EngineFlutterView( EnginePlatformDispatcher.instance, createDomElement('another-view')); + EnginePlatformDispatcher.instance.viewManager.registerView(anotherView); + CanvasKitRenderer.instance.renderScene(scene, anotherView); }); + + test('will error if trying to render into an unregistered view', () async { + final EngineFlutterView unregisteredView = EngineFlutterView( + EnginePlatformDispatcher.instance, + createDomElement('unregistered-view')); + expect( + () => CanvasKitRenderer.instance.renderScene(scene, unregisteredView), + throwsAssertionError, + ); + }); + + test('will dispose the Rasterizer for a disposed view', () async { + final EngineFlutterView view = EngineFlutterView( + EnginePlatformDispatcher.instance, createDomElement('multi-view')); + EnginePlatformDispatcher.instance.viewManager.registerView(view); + expect( + CanvasKitRenderer.instance.debugGetRasterizerForView(view), + isNotNull, + ); + + EnginePlatformDispatcher.instance.viewManager + .disposeAndUnregisterView(view.viewId); + expect( + CanvasKitRenderer.instance.debugGetRasterizerForView(view), + isNull, + ); + }); }); } diff --git a/lib/web_ui/test/canvaskit/render_canvas_factory_test.dart b/lib/web_ui/test/canvaskit/render_canvas_factory_test.dart index 2e85655a19825..a627158ffc72d 100644 --- a/lib/web_ui/test/canvaskit/render_canvas_factory_test.dart +++ b/lib/web_ui/test/canvaskit/render_canvas_factory_test.dart @@ -71,7 +71,7 @@ void testMain() { EnginePlatformDispatcher.instance.implicitView!; final RenderCanvasFactory originalFactory = CanvasKitRenderer.instance - .debugGetRasterizerForView(implicitView) + .debugGetRasterizerForView(implicitView)! .renderCanvasFactory; // Cause the surface and its canvas to be attached to the page diff --git a/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart b/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart index f7f154c69a176..04fb618e9dc6c 100644 --- a/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart +++ b/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart @@ -10,6 +10,7 @@ import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; import '../../common/matchers.dart'; +import '../../html/image_test.dart'; void main() { internalBootstrapBrowserTest(() => doTests); @@ -69,8 +70,13 @@ Future doTests() async { // Prepares a "timed-out" version of the onViewsChanged Stream so tests // can't hang "forever" waiting for this to complete. This stream will close // after 100ms of inactivity, regardless of what the test or the code do. - final Stream onViewsChanged = viewManager.onViewsChanged - .timeout(const Duration(milliseconds: 100), onTimeout: (EventSink sink) { + final Stream onViewCreated = viewManager.onViewCreated.timeout( + const Duration(milliseconds: 100), onTimeout: (EventSink sink) { + sink.close(); + }); + + final Stream onViewDisposed = viewManager.onViewDisposed.timeout( + const Duration(milliseconds: 100), onTimeout: (EventSink sink) { sink.close(); }); @@ -78,15 +84,21 @@ Future doTests() async { final EngineFlutterView view = EngineFlutterView(platformDispatcher, createDomElement('div')); final int viewId = view.viewId; - final Future> viewChangeEvents = onViewsChanged.toList(); + final Future> viewCreatedEvents = onViewCreated.toList(); + final Future> viewDisposedEvents = onViewDisposed.toList(); viewManager.registerView(view); viewManager.unregisterView(viewId); - expect(viewChangeEvents, completes); + expect(viewCreatedEvents, completes); + expect(viewDisposedEvents, completes); - final List events = await viewChangeEvents; + final List createdViewsList = await viewCreatedEvents; + final List disposedViewsList = await viewCreatedEvents; - expect(events, hasLength(2), reason: 'Should fire once per viewManager register/unregister call.'); + expect(createdViewsList, listEqual([viewId]), + reason: 'Should fire creation event for view'); + expect(disposedViewsList, listEqual([viewId]), + reason: 'Should fire dispose event for view'); }); }); }); From 563f3cebe630101c6a0201b7319a70aeb0dba816 Mon Sep 17 00:00:00 2001 From: Harry Terkelsen Date: Thu, 7 Dec 2023 16:28:12 -0800 Subject: [PATCH 2/3] Remove unnecessary clear (the map should already be cleared) --- .../lib/src/engine/view_embedder/flutter_view_manager.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart b/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart index de23077826b54..fc6cced9eefe0 100644 --- a/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart +++ b/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart @@ -97,7 +97,6 @@ class FlutterViewManager { // We need to call `toList()` in order to avoid concurrent modification // inside the loop. _viewData.keys.toList().forEach(disposeAndUnregisterView); - _viewData.clear(); // Let listeners receive the unregistration events from the loop above, then // close the streams. _onViewCreatedController.close(); From a2c7739bded03d11ba371a3ee88a5a7d5403b533 Mon Sep 17 00:00:00 2001 From: Harry Terkelsen Date: Mon, 11 Dec 2023 13:56:43 -0800 Subject: [PATCH 3/3] Unskip safari tests --- lib/web_ui/test/canvaskit/embedded_views_test.dart | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/web_ui/test/canvaskit/embedded_views_test.dart b/lib/web_ui/test/canvaskit/embedded_views_test.dart index d35bba6f8eff9..1f225d9498eaa 100644 --- a/lib/web_ui/test/canvaskit/embedded_views_test.dart +++ b/lib/web_ui/test/canvaskit/embedded_views_test.dart @@ -445,8 +445,7 @@ void testMain() { for (int i = 0; i < 16; i++) { await disposePlatformView(i); } - // TODO(yjbanov): skipped due to https://github.com/flutter/flutter/issues/73867 - }, skip: isSafari); + }); test('correctly reuses overlays', () async { final CkPicture testPicture = @@ -582,8 +581,7 @@ void testMain() { await disposePlatformView(i); } - // TODO(yjbanov): skipped due to https://github.com/flutter/flutter/issues/73867 - }, skip: isSafari); + }); test('embeds and disposes of a platform view', () async { ui_web.platformViewRegistry.registerViewFactory(