From 08b343b90de80aeaa00cdcaedb91900f8e1181e4 Mon Sep 17 00:00:00 2001 From: Harry Terkelsen <1961493+harryterkelsen@users.noreply.github.com> Date: Thu, 23 May 2024 11:36:05 -0700 Subject: [PATCH] Move pictures from deleted canvases to second-to-last canvas instead of last. (flutter/engine#51397) Previously, when the number of display canvases exceeded the maximum amount, we would augment the rendering to reduce the number of canvases to the maximum amount, and move the pictures from the deleted canvases to the last canvas. However, this would cause ugly rendering in cases where pictures would render on top of the platform views they were supposed to be underneath. This is especially noticeable when reproducing this bug: https://github.com/flutter/flutter/issues/144854 This changes things slightly so that the pictures are moved to the second-to-last remaining canvas, so that platform views always render over the pictures they are supposed to. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../src/engine/canvaskit/embedded_views.dart | 18 ++-- .../test/canvaskit/embedded_views_test.dart | 98 ++++++++++++++++++- 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index f1a4228e7fb..1d96e2e1d09 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -63,6 +63,9 @@ class HtmlViewEmbedder { /// The most recent rendering. Rendering _activeRendering = Rendering(); + /// Returns the most recent rendering. Only used in tests. + Rendering get debugActiveRendering => _activeRendering; + DisplayCanvas? debugBoundsCanvas; /// The size of the frame, in physical pixels. @@ -489,7 +492,6 @@ class HtmlViewEmbedder { if (entity is RenderingRenderCanvas) { if (!sawLastCanvas) { sawLastCanvas = true; - picturesForLastCanvas.insertAll(0, entity.pictures); continue; } modifiedEntities.removeAt(i); @@ -500,14 +502,18 @@ class HtmlViewEmbedder { } } } - // Replace the pictures in the last canvas with all the pictures from the - // deleted canvases. + + // Add all the pictures from the deleted canvases to the second-to-last + // canvas. + sawLastCanvas = false; for (int i = modifiedEntities.length - 1; i > 0; i--) { final RenderingEntity entity = modifiedEntities[i]; if (entity is RenderingRenderCanvas) { - entity.pictures.clear(); - entity.pictures.addAll(picturesForLastCanvas); - break; + if (sawLastCanvas) { + entity.pictures.addAll(picturesForLastCanvas); + break; + } + sawLastCanvas = true; } } diff --git a/engine/src/flutter/lib/web_ui/test/canvaskit/embedded_views_test.dart b/engine/src/flutter/lib/web_ui/test/canvaskit/embedded_views_test.dart index 6338eb6ed42..a87d374c142 100644 --- a/engine/src/flutter/lib/web_ui/test/canvaskit/embedded_views_test.dart +++ b/engine/src/flutter/lib/web_ui/test/canvaskit/embedded_views_test.dart @@ -418,7 +418,8 @@ void testMain() { } on AssertionError catch (error) { expect( error.toString(), - contains('Cannot render platform views: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15. These views have not been created, or they have been deleted.'), + contains( + 'Cannot render platform views: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15. These views have not been created, or they have been deleted.'), ); } @@ -1219,6 +1220,101 @@ void testMain() { _overlay, _platformView, ]); + + debugOverrideJsConfiguration(null); + }); + + test( + 'correctly rearranges pictures to second-to-last canvas ' + 'when hitting canvas limit', () async { + final CkPicture testPicture = + paintPicture(const ui.Rect.fromLTRB(0, 0, 10, 10), (CkCanvas canvas) { + canvas.drawCircle(const ui.Offset(5, 5), 5, CkPaint()); + }); + + // Initialize all platform views to be used in the test. + final List platformViewIds = []; + for (int i = 0; i < 20; i++) { + ui_web.platformViewRegistry.registerViewFactory( + 'test-platform-view', + (int viewId) => createDomHTMLDivElement()..id = 'view-$i', + ); + await createPlatformView(i, 'test-platform-view'); + platformViewIds.add(i); + } + + Future renderTestScene(List views) async { + final LayerSceneBuilder sb = LayerSceneBuilder(); + sb.pushOffset(0, 0); + for (final int view in views) { + sb.addPicture(ui.Offset.zero, testPicture); + sb.addPlatformView(view, width: 10, height: 10); + } + await renderScene(sb.build()); + } + + // Render scene with 20 pictures. Check that the second-to-last canvas + // contains the pictures from the canvases that were deleted. + await renderTestScene([ + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + ]); + _expectSceneMatches(<_EmbeddedViewMarker>[ + _overlay, + _platformView, + _overlay, + _platformView, + _overlay, + _platformView, + _overlay, + _platformView, + _overlay, + _platformView, + _overlay, + _platformView, + _overlay, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _platformView, + _overlay, + _platformView, + ]); + + // The second-to-last canvas should have all the extra pictures. + final Rendering rendering = CanvasKitRenderer.instance + .debugGetRasterizerForView(implicitView)! + .viewEmbedder + .debugActiveRendering; + final List numPicturesPerCanvas = rendering.canvases + .map((RenderingRenderCanvas canvas) => canvas.pictures.length) + .toList(); + expect(numPicturesPerCanvas, [1, 1, 1, 1, 1, 1, 12, 1]); }); }); }