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.

Sign up for GitHub

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

Jump to bottom

[skwasm] ColorScheme.fromImageProvider always produces the same result #141326

Closed
parlough opened this issue Jan 11, 2024 · 12 comments · Fixed by flutter/engine#53561
Closed

[skwasm] ColorScheme.fromImageProvider always produces the same result #141326

parlough opened this issue Jan 11, 2024 · 12 comments · Fixed by flutter/engine#53561
Assignees
@eyebrowsoffire
Labels
e: web_skwasm Skwasm rendering backend for web engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list platform-web Web applications specifically team-web Owned by Web platform team triaged-web Triaged by Web platform team

Comments

@parlough
Copy link
Member

parlough commented Jan 11, 2024 •
edited
Loading

No matter what image is provided, the same ColorScheme is being created by ColorScheme.fromImageProvider.

I haven't looked too deeply, but seems to be something going wrong in the _imageProviderToScaled method or how it interacts with ui_web/painting.
@parlough parlough added platform-web Web applications specifically e: web_skwasm Skwasm rendering backend for web team-web Owned by Web platform team labels Jan 11, 2024
@parlough parlough mentioned this issue Jan 11, 2024
Closed
25 tasks
@yjbanov yjbanov added P1 High-priority issues at the top of the work list engine flutter/engine repository. See also e: labels. triaged-web Triaged by Web platform team labels Jan 11, 2024
@yjbanov yjbanov added P2 Important issues not at the top of the work list and removed P1 High-priority issues at the top of the work list labels Feb 22, 2024
@eyebrowsoffire
Copy link
Contributor

@parlough Are you still seeing this issue on the master branch? When I was working on getting the framework unit test working with Skwasm, I originally saw some unit test failures related to this code path (ColorScheme.fromImageProvider), but they went away after I changed some of the renderer detection logic in the framework. I'm wondering if this issue got resolved by that as well.

@parlough
Copy link
Member Author

parlough commented May 5, 2024 •
edited
Loading

@eyebrowsoffire Sorry for the late response. I just tested on latest main (b8abc10689) and I'm still seeing the issue.

You can see this on material_3_demo's sidebar which allows you to choose an image to extract a seed color from.

Hosted: https://parlough-testing-skwasm.web.app/

@parlough
Copy link
Member Author

parlough commented Jun 4, 2024

I went to verify this again and it's still occurring, but I noticed that if I call .toByteData on the resolved image before rescaling it by painting in _imageProviderToScaled, it works?

@GioPan04
Copy link

I was doing some testing and it turns out that ColorScheme.fromImageProvider has the same problem on Flutter Desktop.

Here is the code I tested with: https://dartpad.dev/?id=3b81fc3ac996af3d44c329d731bfad61 . On Flutter web (stable), it works just fine. On Flutter desktop I get the default light color scheme.

Web (stable):
image

Desktop (linux, stable):
image

@GioPan04
Copy link

Edit: It seems to work on channel beta. So it's probably not relevant.

@eyebrowsoffire
Copy link
Contributor

It turns out there are two issues here. I am working on a fix on the flutter side, but I discovered a skia bug that is also contributing to this issue: https://g-issues.skia.org/issues/349201915

I think I have a workaround for the skia bug in the meantime, and I'm working on correctly implementing the workaround now in skwasm.

@eyebrowsoffire eyebrowsoffire added P1 High-priority issues at the top of the work list and removed P2 Important issues not at the top of the work list labels Jun 25, 2024
@eyebrowsoffire eyebrowsoffire self-assigned this Jun 25, 2024
eyebrowsoffire added a commit to eyebrowsoffire/engine that referenced this issue Jun 25, 2024
@eyebrowsoffire
[skwasm] Fixes for getting pixels from an image.
4d207fa 
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
@eyebrowsoffire eyebrowsoffire mentioned this issue Jun 25, 2024
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Jun 25, 2024
@eyebrowsoffire
[skwasm] Fixes for getting pixels from an image. (#53561)
94023d7 
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
@parlough
Copy link
Member Author

Thanks so much for tackling this @eyebrowsoffire! Sounds like it ended up being quite the investigation.

I'll try the fix out with the Material 3 demo, and see if we can start deploying it with JS/Wasm switching :D

@eyebrowsoffire
Copy link
Contributor

I personally tested it with the Material 3 Demo and it seems to fix the issue! But I'd love a second verification whenever you get a chance.

@kevmoo
Copy link
Contributor

kevmoo commented Jun 25, 2024

I tried this out w/ the material demo and saw NO change. We should sync up.

@parlough
Copy link
Member Author

I just tried it out now that the engine commit rolled into the framework and it's working as I expect! Thanks :D

@kevmoo
Copy link
Contributor

kevmoo commented Jun 25, 2024

Confirmed deploy here! https://flutterweb-wasm.web.app/

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 9, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@eyebrowsoffire eyebrowsoffire

Labels
e: web_skwasm Skwasm rendering backend for web engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list platform-web Web applications specifically team-web Owned by Web platform team triaged-web Triaged by Web platform team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[skwasm] Fixes for getting pixels from an image. eyebrowsoffire/engine
5 participants
@kevmoo @yjbanov @parlough @eyebrowsoffire @GioPan04