Skip to content

[canvaskit] Resize to exactly the requested dimensions #162708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

harryterkelsen
Copy link
Contributor

@harryterkelsen harryterkelsen commented Feb 5, 2025

Adds a SurfaceResizeStrategy to Surface. The default option, onlyGrow, implements the current behavior of only re-allocating the Surface when the requested size is larger than the currently allocated Surface. This PR adds the option of SurfaceResizeStrategy.exact which always reallocates the Surface and resizes the underlying OffscreenCanvas. This options performs better in practice since having the OffScreenCanvas larger than the actual rendered size causes calls to createImageBitmap to be slower than if the OffscreenCanvas is exactly the size of the bitmap being created.

Fixes #162700

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added engine flutter/engine repository. See also e: labels. platform-web Web applications specifically labels Feb 5, 2025
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to unblock the fix, but I sense there's a bigger underlying issue here.

According to the PR description:

having the OffScreenCanvas larger than the actual rendered size causes calls to createImageBitmap to be slower than if the OffscreenCanvas is exactly the size of the bitmap being created.

The extra cost in createImageBitmap must be due to memory copying. The returned ImageBitmap is not the same as the one owned by the OffscreenCanvas, but a new copy. So, at the end of every frame createImageBitmap leaves two copies, one owned by the OffscreenCanvas, and one owned by the copy created by createImageBitmap. If the window is being resized, all textures need to be discarded. So we'll have to discard two textures.

In contrast, transferImageBitmap does not create a copy, so at the end of the frame there's only one texture. When resizing, only that one texture needs to be discarded.

This is all assuming one target surface. I'm not sure how the math works out with platform views (or multi-view).


/// Resizes the surface only if the current size is less than the requested
/// size. The surface will only ever grow.
onlyGrow,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we use onlyGrow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just got rid of the option and updated the tests to reflect the new resize behavior.

@harryterkelsen
Copy link
Contributor Author

Re: #162708 (review)

I agree. I am investigating if we can switch to transferToImageBitmap here. This PR is a pre-requisite for enabling transferToImageBitmap since the underlying canvas needs to be exactly-sized for transferToImageBitmap

@harryterkelsen harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2025
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@auto-submit auto-submit bot added this pull request to the merge queue Feb 6, 2025
Merged via the queue into flutter:master with commit 1b095a0 Feb 6, 2025
174 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
harryterkelsen added a commit to harryterkelsen/flutter that referenced this pull request Feb 28, 2025
Adds a `SurfaceResizeStrategy` to `Surface`. The default option,
`onlyGrow`, implements the current behavior of only re-allocating the
Surface when the requested size is larger than the currently allocated
Surface. This PR adds the option of `SurfaceResizeStrategy.exact` which
always reallocates the Surface and resizes the underlying
OffscreenCanvas. This options performs better in practice since having
the OffScreenCanvas larger than the actual rendered size causes calls to
`createImageBitmap` to be slower than if the OffscreenCanvas is exactly
the size of the bitmap being created.

Fixes flutter#162700

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine flutter/engine repository. See also e: labels. platform-web Web applications specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CanvasKit performance degrades a lot after resizing
2 participants