Skip to content
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

[skwasm] Fixes for getting pixels from an image. #53561

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

eyebrowsoffire
Copy link
Contributor

There were two issues here:

  1. We have to stop using the emscripten thread scheduling APIs, as they can be invoked out of order from the rest of the messages that are posted. In some cases, out of order message handling can cause the request for reading pixels in an image to be serviced before some of the texture sources have been transfered to the web worker.

  2. Skia's readPixels fails if there is is a lazy picture image made from a picture that contains a lazy texture image. The pertinent bug is here: https://g-issues.skia.org/issues/349201915

To work around the Skia bug, we just render the image itself onto our scratch canvas and pull the pixels out with glReadPixels.

This fixes flutter/flutter#141326

There were two issues here:

1) We have to stop using the emscripten thread scheduling APIs, as they
can be invoked out of order from the rest of the messages that are
posted. In some cases, out of order message handling can cause the
request for reading pixels in an image to be serviced before some of the
texture sources have been transfered to the web worker.

2) Skia's `readPixels` fails if there is is a lazy picture image made
from a picture that contains a lazy texture image. The pertinent bug is
here: https://g-issues.skia.org/issues/349201915

To work around the Skia bug, we just render the image itself onto our
scratch canvas and pull the pixels out with `glReadPixels`.

This fixes flutter/flutter#141326
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jun 25, 2024
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

@@ -34,12 +35,9 @@ Surface::Surface() {
skwasm_syncTimeOriginForThread(_thread);
}

// Main thread only
// Worker thread only
Copy link
Contributor

Choose a reason for hiding this comment

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

Important distinction! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The semantics here actually changed as part of this PR. dispose used to be called on the main thread, and then that dispatched to the web worker which called _dispose. Now there's just one method, dispose and that does what _dispose used to do.

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 25, 2024
@auto-submit auto-submit bot merged commit 94023d7 into flutter:main Jun 25, 2024
31 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 25, 2024
…150797)

flutter/engine@62e0b5f...94023d7

2024-06-25 jacksongardner@google.com [skwasm] Fixes for getting pixels from an image. (flutter/engine#53561)
2024-06-25 parrott.jason@gmail.com fix: web canvaskit fragment shaders were not using updated uniform values (flutter/engine#53246)
2024-06-25 skia-flutter-autoroll@skia.org Roll Dart SDK from b5fc85cfcf1b to 65ab2f2cf0d3 (1 revision) (flutter/engine#53565)
2024-06-25 skia-flutter-autoroll@skia.org Roll Skia from e4e4feb97a54 to da1ea4eb0270 (4 revisions) (flutter/engine#53563)
2024-06-25 flar@google.com [DisplayList] Switch to recording DrawVertices objects by reference (flutter/engine#53548)
2024-06-25 skia-flutter-autoroll@skia.org Roll Skia from 5f21260470cf to e4e4feb97a54 (1 revision) (flutter/engine#53559)
2024-06-25 30870216+gaaclarke@users.noreply.github.com [impeller] Cleanup blur (flutter/engine#53543)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[skwasm] ColorScheme.fromImageProvider always produces the same result
2 participants