Repository navigation
[CP-beta][web] Make resize and render atomic on the shared offscreen surface - #195839
Conversation
…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
|
@mdebbar please fill out the PR description above, afterwards the release team will review this request. |
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
![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 refactors offscreen surface rasterization to ensure that resizing, rendering, and capturing are atomic operations, preventing race conditions when sharing a single offscreen surface across multiple views. It updates both CanvasKit and Skwasm implementations to accept dimensions directly during rasterization and adds a comprehensive multi-view sizing test suite. The review feedback identifies critical issues in surface.cc where ResizeSurface is invoked before establishing the active GL context via Skwasm::makeCurrent, which could lead to crashes or undefined behavior. Additionally, a potential out-of-bounds RangeError is noted in offscreen_canvas_rasterizer.dart if the number of pictures exceeds the available display canvases.
| ResizeSurface(width, height); | ||
| Skwasm::makeCurrent(gl_context_); |
There was a problem hiding this comment.
The surface resizing operation ResizeSurface(width, height) is called before making the GL context current with Skwasm::makeCurrent(gl_context_). Since resizing the surface involves OpenGL/WebGL state changes and resource allocation, it must be executed while the correct GL context is active on the thread. Calling it before makeCurrent can lead to undefined behavior, GL errors, or crashes if the context is not current or if a different context is active.
Skwasm::makeCurrent(gl_context_);
ResizeSurface(width, height);| ResizeSurface(image->width(), image->height()); | ||
| Skwasm::makeCurrent(gl_context_); |
There was a problem hiding this comment.
Similarly, ResizeSurface(image->width(), image->height()) is called before Skwasm::makeCurrent(gl_context_). It should be called after making the GL context current to ensure all OpenGL operations are executed on the correct context.
Skwasm::makeCurrent(gl_context_);
ResizeSurface(image->width(), image->height());| (displayCanvases[index] as RenderCanvas).renderWithNoBitmapSupport( | ||
| source, | ||
| currentFrameSize.height, | ||
| currentFrameSize, | ||
| ); |
There was a problem hiding this comment.
If pictures.length is greater than displayCanvases.length, accessing displayCanvases[index] will throw a RangeError and crash the rendering pipeline. To prevent this, add a defensive bounds check to ensure index is within the bounds of displayCanvases.
if (index < displayCanvases.length) {
(displayCanvases[index] as RenderCanvas).renderWithNoBitmapSupport(
source,
currentFrameSize.height,
currentFrameSize,
);
}
github-actions
Bot
added
engine
reidbaker
added
the
autosubmit
1b9ee1a
into
flutter:flutter-3.50-candidate.0
This pull request is created by automatic cherry pick workflow
Cherry-pick of #194401.
Issue Link:
#185034
Impact Description:
In multi-view Flutter Web apps, views rendered at the same time race on a shared offscreen surface and end up with the wrong canvas size, clipping or overflowing their host HTML containers. Skwasm's
Image.toByteDatauses the same surface and has the same problem.This is a regression since Flutter 3.41 (#179473) that affects deployed multi-view web apps on both CanvasKit and Skwasm and has been blocking production apps from upgrading past 3.38.
Changelog Description:
flutter/185034: When rendering multiple views on the web, canvases may fail to match their host container dimensions.
Workaround:
None.
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
Follow the repro steps in #185034, or run the new tests in
engine/src/flutter/lib/web_ui/test/ui/multi_view_sizing_test.dart.