Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[canvaskit] read pixels back in Picture.toImage #40004

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Mar 2, 2023

This undoes the optimization made in #38573, and adds a test-case that would have failed when that optimization was made. The issue is that CanvasKit loses track of which resources belong to which context that leads to WebGL errors followed by UI corruption.

Fixes flutter/flutter#121758

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 2, 2023
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Nit: this says its about toByteData, but the changes are in toImage.

Is there a way to defer the usage of MakeRasterDirectSurface until we're calling toByteData? ITs more or less expected that toByteData can be slow, but toImageSync is useful for composite effects using fragment shaders - and running those in software is not going to be feasible.

@yjbanov yjbanov force-pushed the toByteData-gl-context branch from 71fbadb to 509eaf9 Compare March 6, 2023 22:02
@yjbanov yjbanov changed the title [canvaskit] use MakeRasterDirectSurface for Image.toByteData [canvaskit] read pixels back in Picture.toImage Mar 6, 2023
@yjbanov yjbanov force-pushed the toByteData-gl-context branch from 509eaf9 to 205b923 Compare March 6, 2023 22:42
@yjbanov yjbanov force-pushed the toByteData-gl-context branch from 205b923 to d679b87 Compare March 7, 2023 00:13
@yjbanov
Copy link
Contributor Author

yjbanov commented Mar 7, 2023

Nit: this says its about toByteData, but the changes are in toImage.

Is there a way to defer the usage of MakeRasterDirectSurface until we're calling toByteData? ITs more or less expected that toByteData can be slow, but toImageSync is useful for composite effects using fragment shaders - and running those in software is not going to be feasible.

Couldn't find a way to defer it. I'm going to start a thread with the Skia team to figure this out. In the meantime, I'm reverting #38573 and adding a new unit-test that catches the issue. Hopefully, next time we attempt it, it won't regress.

@yjbanov
Copy link
Contributor Author

yjbanov commented Mar 7, 2023

For posterity, I tried the following solutions that didn't work:

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

@flutter-dashboard
Copy link

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.

Changes reported for pull request #40004 at sha d679b87

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2023
@auto-submit auto-submit bot merged commit be56e05 into flutter:main Mar 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web:canvaskit] Image.toByteData loses track of WebGL contexts corrupting rendering pipeline state
3 participants