Repository navigation
Conversation
github-actions
Bot
added
engine
github-actions
Bot
added
team-web
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a synchronization mechanism in OffscreenCanvasRasterizer to prevent concurrent access to the shared offscreenSurface across multiple views, and adds a test to verify canvas sizing. The review feedback points out a race condition where synchronizing only the draw method is insufficient because the asynchronous prepareToDraw method, which resizes the shared surface, remains unsynchronized. To resolve this, the reviewer suggests synchronizing the entire rasterize method instead to ensure atomic execution of both preparation and drawing.
|
This looks like a reasonable quick solution. However, I wonder if we have an unnecessary levels of concurrency happening. Animation frame scheduling happens on the main page. At the end of the frame we know all views that need to be redrawn. Can we draw by sending a batch of draw commands to be rendered as a single unit. The renderer (current thread if in ST mode, or worker) would render the whole batch serially, sizing the OffscreenCanvas accordingly. |
| /// that we only render one view at a time. | ||
| Future<void> _lock = Future<void>.value(); | ||
|
|
||
| Future<T> _synchronized<T>(Future<T> Function() computation) async { |
There was a problem hiding this comment.
what happens if we have 3 concurrent draw calls? It seems like the first one will go ahead, but the second two will wait on the same oldLock... Maybe we need a queue here?
| group('MultiView Sizing', () { | ||
| setUpUnitTests(withImplicitView: true); | ||
|
|
||
| test('canvases for multiple views have correct sizes', () async { |
There was a problem hiding this comment.
Please add a test with 3 views
|
Superseded by #194401 |
…lutter#194401) Alternative to flutter#185622. flutter#179473 split rendering on the shared `OffscreenSurface` into two separately awaited steps: `setSize()` and then `rasterizeToImageBitmaps()`. When multiple views render at the same time, one view can resize the surface between another view's resize and render, so that view ends up with a bitmap of the wrong size. Skwasm's `Image.toByteData` uses the same surface and has the same problem. Instead of using a lock (like I did [here](flutter#185622)), the size now travels with each render, so the surface resizes and renders as one indivisible operation: - Skwasm: the size is part of the same message to the raster thread (as it was before flutter#179473). Messages are processed in order, so no locking is needed, and there is one less worker round trip per view per frame. - CanvasKit: resize, draw, and `createImageBitmap()` run synchronously. Fixes flutter#185034
…lutter#194401) Alternative to flutter#185622. flutter#179473 split rendering on the shared `OffscreenSurface` into two separately awaited steps: `setSize()` and then `rasterizeToImageBitmaps()`. When multiple views render at the same time, one view can resize the surface between another view's resize and render, so that view ends up with a bitmap of the wrong size. Skwasm's `Image.toByteData` uses the same surface and has the same problem. Instead of using a lock (like I did [here](flutter#185622)), the size now travels with each render, so the surface resizes and renders as one indivisible operation: - Skwasm: the size is part of the same message to the raster thread (as it was before flutter#179473). Messages are processed in order, so no locking is needed, and there is one less worker round trip per view per frame. - CanvasKit: resize, draw, and `createImageBitmap()` run synchronously. Fixes flutter#185034
…lutter#194401) Alternative to flutter#185622. flutter#179473 split rendering on the shared `OffscreenSurface` into two separately awaited steps: `setSize()` and then `rasterizeToImageBitmaps()`. When multiple views render at the same time, one view can resize the surface between another view's resize and render, so that view ends up with a bitmap of the wrong size. Skwasm's `Image.toByteData` uses the same surface and has the same problem. Instead of using a lock (like I did [here](flutter#185622)), the size now travels with each render, so the surface resizes and renders as one indivisible operation: - Skwasm: the size is part of the same message to the raster thread (as it was before flutter#179473). Messages are processed in order, so no locking is needed, and there is one less worker round trip per view per frame. - CanvasKit: resize, draw, and `createImageBitmap()` run synchronously. Fixes flutter#185034
Summary
This PR fixes a regression in multi-view sizing (#185034) where canvases in multiple views would fail to match their host dimensions. The root cause was the shared
OffscreenSurfaceinOffscreenCanvasRasterizer. Because rendering is asynchronous, View B could resize the shared surface while View A was still in the middle of a draw call, leading to View A inheriting View B's dimensions.Design
I introduced a synchronization mechanism (
synchronizedblock) inOffscreenCanvasRasterizerto ensure that only oneViewRasterizercan use the shared surface at a time. This guarantees rendering correctness while maintaining the efficiency of a single WebGL context.Pros:
Testing
engine/src/flutter/lib/web_ui/test/ui/multi_view_sizing_test.dartwhich renders two differently sized views in the same event loop turn and verifies their canvas dimensions.Fixes #185034