[stable] Cherrypick fix for stretched embedded app on mobile #164406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
flutter#163175) Use `transferToImageBitmap` instead of `createImageBitmap` if we have an `OffscreenCanvas`. Now that we are sizing the `OffscreenCanvas` exactly to the frame size, we can use `transferToImageBitmap`, which should be zero-copy and therefore faster than `createImageBitmap`. In testing, this speeds things up a little bit but we still drop frames compared to multi-Surface rasterization. Helps with flutter#162618 ## 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 `///`). - [ ] 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
yjbanov
approved these changes
Mar 3, 2025
camsim99
approved these changes
Mar 6, 2025
reidbaker
requested changes
Mar 10, 2025
|
autosubmit label was removed for flutter/flutter/164406, because This PR has not met approval requirements for merging. Changes were requested by {reidbaker}, please make the needed changes and resubmit this PR.
|
reidbaker
approved these changes
Mar 11, 2025
|
autosubmit label was removed for flutter/flutter/164406, because - The status or check suite Linux linux_web_engine_tests has failed. Please fix the issues identified (or deflake) before re-applying this label. |
reidbaker
reviewed
Mar 11, 2025
| test('uses transferToImageBitmap for bitmap creation', () async { | ||
| final Surface surface = Surface(); | ||
| surface.ensureSurface(const BitmapSize(10, 10)); | ||
| final DomOffscreenCanvas offscreenCanvas = surface.debugGetOffscreenCanvas()!; |
There was a problem hiding this comment.
It looks like debugGetOffscreenCanvas does not exist.
|
@yjbanov @harryterkelsen looks like a method used in a test does not exist at this commit. This pr is at risk of not making the next flutter stable. |
|
@reidbaker It was a trivial test-only change. I backported it into this cherry-pick: 0885ee6 |
|
Thank you I will try to get this landed tomorrow |
|
FYI: I pushed a commit to cleanup the changelog! |
reidbaker
approved these changes
Mar 12, 2025
fbf3485
into
flutter:flutter-3.29-candidate.0
150 checks passed
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 14, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 14, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Link:
What is the link to the issue this cherry-pick is addressing?
#159974
Changelog Description:
Explain this cherry pick in one line that is accessible to most Flutter developers. See best practices for examples
Improve performance of CanvasKit by avoiding copying bitmaps when rendering
Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)
Improves performance of all apps using CanvasKit. Fixes a bug relating to embedded Flutter apps on mobile.
Workaround:
Is there a workaround for this issue?
No
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:
What are the steps to validate that this fix works?
Run the example app in the fixed issue on Chrome for Android and verify it works as expected.