Revert "Reverts "[reland] Unify canvas creation and Surface code in S…kwasm and CanvasKit (#179473)" (#180152)" #180610
Conversation
…kwasm and CanvasKit (flutter#179473)" (flutter#180152)" This reverts commit 78a6a01.
There was a problem hiding this comment.
Code Review
This pull request refactors the CanvasKit rendering surfaces and rasterizers to improve context management and support for WebGL context loss and restoration. Key changes include introducing abstract Surface, OffscreenSurface, and OnscreenSurface interfaces, along with CanvasProvider and SurfaceProvider classes to manage canvas and surface lifecycles. The MultiSurfaceRasterizer and OffscreenCanvasRasterizer are moved to a new compositing directory and updated to use these new provider patterns, allowing for more flexible surface creation and handling of WebGL context events. The CkSurface implementation is heavily revised to centralize context creation, resizing, and loss handling, making setSize an asynchronous operation. Consequently, methods like _readPixelsFromImageViaSurface and toImageSync in CkImage and CkPicture are updated to correctly await these asynchronous surface operations, with toImageSync being flagged for potential renaming or removal due to its new asynchronous nature. Additionally, the isDisposed getter is added to CkPicture and used in LayerVisitor to handle disposed pictures gracefully during painting. Review comments highlight the need to await asynchronous setSize calls in CkImage and CkPicture to prevent race conditions, and suggest adding documentation to new public methods related to context loss handling in CkSurface.
engine/src/flutter/lib/web_ui/lib/src/engine/compositing/multi_surface_rasterizer.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/surface.dart
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the web engine's rendering layer. By unifying the Surface and Rasterizer implementations and introducing a CanvasProvider abstraction, the architecture has become more modular and maintainable. The changes also notably improve WebGL context loss handling for the Skwasm backend. The code is well-structured, and the addition of new tests for context loss and recovery is a great step towards ensuring the robustness of the new architecture. I have one minor suggestion for a performance micro-optimization in the Skwasm surface initialization logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the rendering layer by moving Surface and Rasterizer implementations from being renderer-specific to a shared compositing directory. A new CanvasProvider abstraction is introduced to manage the lifecycle of canvas elements, and Surface implementations are updated to use it, improving WebGL context loss handling. The Skwasm backend is updated to use this new architecture. Tests for context loss and bitmap-less rendering have been added.
| void dispose() { | ||
| viewEmbedder.dispose(); | ||
| displayFactory.dispose(); | ||
| } |
There was a problem hiding this comment.
The removal of displayFactory.dispose() from ViewRasterizer.dispose() may cause a memory leak. OffscreenCanvasViewRasterizer creates its own DisplayCanvasFactory which in turn creates RenderCanvas instances. These RenderCanvas instances create DOM elements and need to be disposed to be removed from the DOM. DisplayCanvasFactory.dispose() handles this, but it's no longer called.
MultiSurfaceViewRasterizer is unaffected as its DisplayCanvasFactory gets surfaces from a SurfaceProvider that is disposed separately.
A possible fix is to have OffscreenCanvasViewRasterizer override dispose and call displayFactory.dispose():
// In engine/src/flutter/lib/web_ui/lib/src/engine/compositing/offscreen_canvas_rasterizer.dart
class OffscreenCanvasViewRasterizer extends ViewRasterizer {
// ...
@override
void dispose() {
displayFactory.dispose();
super.dispose();
}
}| void dispose() { | ||
| _onViewCreatedListener.cancel(); | ||
| _onViewDisposedListener.cancel(); | ||
| for (final ViewRasterizer rasterizer in rasterizers.values) { | ||
| rasterizer.dispose(); | ||
| } | ||
| rasterizers.clear(); | ||
| rasterizer.dispose(); | ||
| pictureToImageSurface.dispose(); | ||
| } |
There was a problem hiding this comment.
The rasterizers map is not cleared in the dispose method, which can lead to a memory leak. While rasterizer.dispose() handles the disposal of the ViewRasterizer instances, the Renderer's rasterizers map still holds references to them.
I recommend clearing the map after disposing the rasterizer.
| void dispose() { | |
| _onViewCreatedListener.cancel(); | |
| _onViewDisposedListener.cancel(); | |
| for (final ViewRasterizer rasterizer in rasterizers.values) { | |
| rasterizer.dispose(); | |
| } | |
| rasterizers.clear(); | |
| rasterizer.dispose(); | |
| pictureToImageSurface.dispose(); | |
| } | |
| void dispose() { | |
| _onViewCreatedListener.cancel(); | |
| _onViewDisposedListener.cancel(); | |
| rasterizer.dispose(); | |
| rasterizers.clear(); | |
| pictureToImageSurface.dispose(); | |
| } |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
…ode in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610)
Roll Flutter from 01d37bc to 73769a2 (65 revisions) flutter/flutter@01d37bc...73769a2 2026-01-10 engine-flutter-autoroll@skia.org Roll Dart SDK from 5e855c2bb3ef to 87fbfd5381b6 (1 revision) (flutter/flutter#180800) 2026-01-10 engine-flutter-autoroll@skia.org Roll Skia from b2b109f0e980 to f39cc645b1dd (2 revisions) (flutter/flutter#180796) 2026-01-10 engine-flutter-autoroll@skia.org Roll Dart SDK from b7963905e6a2 to 5e855c2bb3ef (2 revisions) (flutter/flutter#180794) 2026-01-10 ahmedsameha1@gmail.com Make sure that a CupertinoTabScaffold doesn't crash in 0x0 environment (flutter/flutter#179824) 2026-01-10 engine-flutter-autoroll@skia.org Roll Dart SDK from d25ad331b7ea to b7963905e6a2 (2 revisions) (flutter/flutter#180783) 2026-01-10 ahmedsameha1@gmail.com Make sure that a Container doesn't crash in 0x0 environment (flutter/flutter#180350) 2026-01-10 ahmedsameha1@gmail.com Make sure that an Expansible doesn't crash in 0x0 environment (flutter/flutter#180478) 2026-01-10 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Enabled some disabled impeller fragment shader dart tests (#180759)" (flutter/flutter#180785) 2026-01-09 dkwingsmt@users.noreply.github.com Merge `widget_tester_leaks_free_test.dart` into `widget_tester_test.dart` (flutter/flutter#180600) 2026-01-09 codefu@google.com fix: there are no riscv fuchsia artifacts (flutter/flutter#180779) 2026-01-09 engine-flutter-autoroll@skia.org Roll Skia from 7386219151e6 to b2b109f0e980 (1 revision) (flutter/flutter#180771) 2026-01-09 chinmaygarde@google.com Re-prioritize pipeline compile jobs and perform them eagerly instead of waiting. (flutter/flutter#180022) 2026-01-09 30870216+gaaclarke@users.noreply.github.com Enabled some disabled impeller fragment shader dart tests (flutter/flutter#180759) 2026-01-09 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from rxeg-6UB678HKJ4UQ... to 83Favz_zzMzdVuOHg... (flutter/flutter#180765) 2026-01-09 jhy03261997@gmail.com [A11y ] Add `clearSemantics`in table (flutter/flutter#180665) 2026-01-09 chinmaygarde@google.com Update CODEOWNERS to remove chinmaygarde. (flutter/flutter#180703) 2026-01-09 vhaudiquet343@hotmail.fr [ Tool ] Add support for linux riscv64 architecture (flutter/flutter#178711) 2026-01-09 engine-flutter-autoroll@skia.org Roll Skia from e9b3264ade0c to 7386219151e6 (12 revisions) (flutter/flutter#180754) 2026-01-09 engine-flutter-autoroll@skia.org Roll Dart SDK from fe2ba2c5dd50 to d25ad331b7ea (10 revisions) (flutter/flutter#180741) 2026-01-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Unpin google_mobile_ads (#180573)" (flutter/flutter#180761) 2026-01-09 koichi20021217@gmail.com Fix typo in dropdown_menu.dart (flutter/flutter#180172) 2026-01-09 engine-flutter-autoroll@skia.org Roll Packages from 039a026 to 51fe1d9 (1 revision) (flutter/flutter#180742) 2026-01-09 goderbauer@google.com Unpin google_mobile_ads (flutter/flutter#180573) 2026-01-09 evanwall@buffalo.edu Update flutter changelog for 3.38.6 (flutter/flutter#180708) 2026-01-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix iOS xattr removal to clear all extended attributes (#180355)" (flutter/flutter#180709) 2026-01-08 ahmedsameha1@gmail.com Make sure that an EditableText doesn't crash in 0x0 environment (flutter/flutter#180457) 2026-01-08 matt.kosarek@canonical.com Implementation of tooltip windows for win32 (flutter/flutter#179147) 2026-01-08 mdebbar@google.com [web] Don't serve files outside of project (flutter/flutter#180699) 2026-01-08 engine-flutter-autoroll@skia.org Roll Skia from 837be28dd218 to e9b3264ade0c (1 revision) (flutter/flutter#180702) 2026-01-08 sokolovskyi.konstantin@gmail.com Add new motion accessibility features to iOS. (flutter/flutter#178102) 2026-01-08 104009581+Saqib198@users.noreply.github.com Fix iOS xattr removal to clear all extended attributes (flutter/flutter#180355) 2026-01-08 bkonyi@google.com [ Widget Preview ] Move widget_preview_scaffold tests to `dev/integration_tests/widget_preview_scaffold` (flutter/flutter#180658) 2026-01-08 30870216+gaaclarke@users.noreply.github.com De-interleaves engine dart test output (flutter/flutter#180651) 2026-01-08 zhongliu88889@gmail.com [web] Fix SemanticsService.announce not working inside dialogs (flutter/flutter#179958) 2026-01-08 engine-flutter-autoroll@skia.org Roll Skia from 42233226ac56 to 837be28dd218 (2 revisions) (flutter/flutter#180693) 2026-01-08 iinozemtsev@google.com Roll Dart SDK to 3.11.0-296.2.beta (flutter/flutter#180685) 2026-01-08 116356835+AbdeMohlbi@users.noreply.github.com Improve code quality in `AndroidTouchProcessorTest.java` (flutter/flutter#180583) 2026-01-08 1961493+harryterkelsen@users.noreply.github.com Revert "Reverts "[reland] Unify canvas creation and Surface code in S…kwasm and CanvasKit (#179473)" (#180152)" (flutter/flutter#180610) 2026-01-08 engine-flutter-autoroll@skia.org Roll Skia from a0c407bce408 to 42233226ac56 (4 revisions) (flutter/flutter#180688) 2026-01-08 bkonyi@google.com [ Tool ] Fix flake in overall_experience_test.dart (flutter/flutter#180655) 2026-01-08 engine-flutter-autoroll@skia.org Roll Packages from 9705815 to 039a026 (6 revisions) (flutter/flutter#180684) 2026-01-08 104147021+MohammedTarigg@users.noreply.github.com flutter_tools: Auto-generate ExportOptions.plist for manual iOS code signing (flutter/flutter#177888) 2026-01-08 engine-flutter-autoroll@skia.org Roll Skia from 58837e160874 to a0c407bce408 (2 revisions) (flutter/flutter#180679) 2026-01-08 engine-flutter-autoroll@skia.org Roll Skia from 1e3266fdba86 to 58837e160874 (1 revision) (flutter/flutter#180677) 2026-01-08 engine-flutter-autoroll@skia.org Roll Skia from 3c47ea10638f to 1e3266fdba86 (4 revisions) (flutter/flutter#180675) 2026-01-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from dTvN_JVSCfGFRasvH... to rxeg-6UB678HKJ4UQ... (flutter/flutter#180673) ...
This PR introduces a significant refactoring of the web engine's rendering layer by unifying the Surface and Rasterizer implementations. These components have been moved from being renderer-specific to a generic compositing directory, making the architecture more modular and easier to maintain. The rasterizers are now renderer-agnostic and are provided with renderer-specific surface factories via dependency injection. A new CanvasProvider abstraction has also been introduced to manage the lifecycle of the underlying canvas elements.
A key outcome of this work is that the Skwasm backend now correctly handles WebGL context loss events. This was achieved by refactoring SkwasmSurface to allow the Dart side to manage the OffscreenCanvas lifecycle. A communication channel between the main thread and the web worker is now used to gracefully handle context loss and recovery. This effort also included fixing several related bugs around surface sizing, resource cleanup, and callback handling in multi-surface scenarios.
To validate these changes, new testing APIs have been added to allow for the creation of renderer-agnostic surface tests. A new test file, surface_context_lost_test.dart, has been added to verify the context loss and recovery behavior across all supported renderers, ensuring the new architecture is robust and reliable.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.