[wimp] Adds wimp-heavy variant for non-chrome browsers - #189953
[wimp] Adds wimp-heavy variant for non-chrome browsers#189953gaaclarke wants to merge 2 commits into
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
gaaclarke
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a heavy variant of the Wimp renderer, 'wimp_heavy', by updating build targets, artifact copying steps, and the Web SDK configuration. Additionally, the loader logic in 'skwasm_loader.js' is updated to resolve 'wimp_heavy' when a heavy build is required and Wimp is enabled. Feedback suggests using optional chaining on the 'config' object in the loader to prevent potential runtime errors if 'config' is undefined.
| const needsHeavy = (!browserEnvironment.hasImageCodecs || !browserEnvironment.hasChromiumBreakIterators) | ||
| const fileStem = needsHeavy | ||
| ? 'skwasm_heavy' | ||
| ? (config.enableWimp ? 'wimp_heavy' : 'skwasm_heavy') |
There was a problem hiding this comment.
To prevent potential runtime errors if the config object is null or undefined (for example, in certain test environments or custom embeddings), consider using optional chaining (config?.enableWimp) when checking the configuration.
| ? (config.enableWimp ? 'wimp_heavy' : 'skwasm_heavy') | |
| ? (config?.enableWimp ? 'wimp_heavy' : 'skwasm_heavy') |
github-actions
Bot
added
engine
eyebrowsoffire
left a comment
eyebrowsoffire
left a comment
There was a problem hiding this comment.
We will actually need some kind of solution for how to deal with image decoding. As of right now, just flipping the heavy flag just compiles in SkAnimatedImage, which will have skia do image decoding for us on non-Chrome browsers. But that just produces an SkImage, which is incompatible with wimp, and will basically cause things to crash if the web engine attempts to render an image this way.
The reason the "light" version of skwasm/wimp gets away with this is because on Chrome, all code paths on the dart side use the browser-based ImageDecoder API to produce an ImageBitmap and so the animated image path is never used. I think the newest versions of Firefox and Safari support this API now, so theoretically we could do the same there, but in practice we haven't turned that on on the dart side for browsers other than Chrome. In the past we used to feature gate things by the existence of the browser API rather than explicitly turning it on per-browser, but we got burned by that when browsers would release a buggy version of their API that we hadn't vetted yet and we were left scrambling to put a patch in to disable it.
If it turns out that both Firefox's and Safari's ImageDecoder APIs are actually ready for prime time and available in all supported versions of our browser, we could consider completely removing the SkAnimatedImage path and then it's one less vector to worry about for the heavy vs. light conversation. But we'll need to talk to the other web folks to move forward on that and make sure we change things on the dart side to always use the browser-based ImageDecoder API.
|
@eyebrowsoffire okay, that probably explains the weird errors I've been trying to get to the bottom of.
I don't actually think that is the case for Safari: https://developer.mozilla.org/en-US/docs/Web/API/ImageDecoder It is an explicit omission on safari. |
|
@eyebrowsoffire I'll look into the ImageDecoder in a separate PR. This actually renders something in firefox, whereas before this it rendered nothing. |
Actually the problem was the one fixed in #189958. Firefox webgl2 is stricter in this case. |
|
Chatted with @eyebrowsoffire offline. He would like a fallback path for image decoding and enabling some felt integration tests as part of this pr. His notes:
|
|
rebasing to get our opengles fix (#189958) |
2 participants
issue: #187212
This gets wimp_heavy loading on firefox. There are still some webgl errors though.
video
Before this change nothing rendered for counter, now it works as long as you don't hover over the floating button.
firefox.mov
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.