fix: web canvaskit fragment shaders were not using updated uniform values #53246

Moncader · 2024-06-06T09:34:30Z

This fixes an issue where in CanvasKit builds, uniforms set in setFloat function in the Paint class don't get updated after the initial render.
For example, like in the issue linked below, having a shader that animates a value over time and you want to reuse the Paint object that the shader is set to.

I'm no expert with CanvasKit nor with the Flutter Engine, but from what I could tell it seemed that the uniforms were only being sent to Skia on creation of the shader via _makeEffect.

However, any uniforms set afterwords were just stored in a local dart-side List and forgotten about.

This PR changes the List to use a WASM backed SkFloat32List to keep the references to the uniforms linked to dart-side.

fixes flutter/flutter#149800

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

flutter-dashboard · 2024-06-06T09:34:34Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

Moncader · 2024-06-06T09:36:44Z

I'll get to writing tests (if possible for gl related issues on the web?) and add it here soon.
Do golden tests even work with shaders?

eyebrowsoffire · 2024-06-07T18:14:24Z

test/canvaskit/canvaskit_api_test.dart is broken with this change and needs to be updated.

flutter-dashboard · 2024-06-10T07:48:45Z

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 #53246 at sha bce9de7

Moncader · 2024-06-11T00:46:10Z

The golden is incorrect and so far I'm not sure why.
I'll slowly work it out over the next few days. (the golden seems to be affected by the previous test and is showing the 25px version on the 10px test even though it's a new paint object.

I assume it's a bug on my side but I'd also like to know if as a rule in this flutter engine golden test system you can't re-use a golden test file in a single test like I am for this one?

Moncader · 2024-06-14T07:01:20Z

I've confirmed on my local machine that this is not a bug on my side. But the golden tests are producing different results in CI than Chrome is showing per-frame on my local machine.

Does the golden system have some kind of restriction where you aren't allowed to use multiple of the same golden file?

IntensiCode · 2024-06-17T17:37:37Z

@Moncader just stumbled across your PR after running into this issue. currently using the new Paint() for every frame as a work-around. anyway, is there anything I can do to help out with this PR?

Moncader · 2024-06-18T00:56:59Z

@IntensiCode The support and fix is complete. I'm waiting on someone to give some insight on how the golden test system works due to the test saying there's a diff when there isn't actually a diff when confirmed locally.

I'm asking over on discord as well.

lib/web_ui/test/ui/fragment_shader_test.dart

…lues

Co-authored-by: Jackson Gardner <[email protected]>

flutter-dashboard · 2024-06-19T01:41:27Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53246 at sha 7a0dfcc

eyebrowsoffire · 2024-06-19T01:47:51Z

The goldens seem to indicate that this issue exists in the skwasm renderer as well and needs to be fixed. I can look into that in a followup though.

Moncader · 2024-06-19T02:57:24Z

I'll check it out first actually. I don't mind getting my feet wet there. (assuming the issue ends up being a simple fix like this one was)

eyebrowsoffire · 2024-06-20T16:45:56Z

I think the fix for the skwasm side should just be here: https://github.com/flutter/engine/blob/main/lib/web_ui/skwasm/shaders.cpp#L134

Instead of doing SkData::MakeWithCopy(uniforms->data(), uniforms->size()), we should just use the existing SkData object by doing sk_ref_sp(uniforms)

eyebrowsoffire · 2024-06-20T16:41:16Z

lib/web_ui/lib/src/engine/canvaskit/painting.dart

 class CkFragmentShader implements ui.FragmentShader, CkShader {
  CkFragmentShader(this.name, this.effect, int floatCount, int textureCount)
-      : floats = List<double>.filled(floatCount + textureCount * 2, 0),
+      : floats = mallocFloat32List(floatCount + textureCount * 2),


Since we malloc'd this list, we also need to free it in dispose

Moncader · 2024-06-21T00:33:09Z

I think the fix for the skwasm side should just be here: https://github.com/flutter/engine/blob/main/lib/web_ui/skwasm/shaders.cpp#L134

Instead of doing SkData::MakeWithCopy(uniforms->data(), uniforms->size()), we should just use the existing SkData object by doing sk_ref_sp(uniforms)

This is what I was thinking as well, but wasn't sure if it's safe to use the reference like that.
Is there a guarantee that those uniforms won't be free'd or swapped to a different object somewhere else?

…aders.

flutter-dashboard · 2024-06-21T01:45:42Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53246 at sha 2c8681f

eyebrowsoffire · 2024-06-21T19:05:16Z

I think the fix for the skwasm side should just be here: https://github.com/flutter/engine/blob/main/lib/web_ui/skwasm/shaders.cpp#L134
Instead of doing SkData::MakeWithCopy(uniforms->data(), uniforms->size()), we should just use the existing SkData object by doing sk_ref_sp(uniforms)

This is what I was thinking as well, but wasn't sure if it's safe to use the reference like that. Is there a guarantee that those uniforms won't be free'd or swapped to a different object somewhere else?

It's managed by a smart pointer sk_sp so it should keep it alive as long as it is needed.

flutter-dashboard · 2024-06-25T07:41:11Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53246 at sha 943832e

Moncader · 2024-06-25T08:41:48Z

The test looks good now. Seems I don't have permission to say OK to the golden though?

eyebrowsoffire

I went ahead and approved the goldens. Thanks for the fix!

yjbanov

…uniform values (flutter/engine#53246)

…150797) flutter/engine@62e0b5f...94023d7 2024-06-25 [email protected] [skwasm] Fixes for getting pixels from an image. (flutter/engine#53561) 2024-06-25 [email protected] fix: web canvaskit fragment shaders were not using updated uniform values (flutter/engine#53246) 2024-06-25 [email protected] Roll Dart SDK from b5fc85cfcf1b to 65ab2f2cf0d3 (1 revision) (flutter/engine#53565) 2024-06-25 [email protected] Roll Skia from e4e4feb97a54 to da1ea4eb0270 (4 revisions) (flutter/engine#53563) 2024-06-25 [email protected] [DisplayList] Switch to recording DrawVertices objects by reference (flutter/engine#53548) 2024-06-25 [email protected] Roll Skia from 5f21260470cf to e4e4feb97a54 (1 revision) (flutter/engine#53559) 2024-06-25 [email protected] [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 [email protected],[email protected],[email protected] 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

github-actions bot added the platform-web Code specifically for the web engine label Jun 6, 2024

jonahwilliams requested a review from eyebrowsoffire June 7, 2024 15:31

flutter-dashboard bot added the will affect goldens label Jun 10, 2024

eyebrowsoffire suggested changes Jun 18, 2024

View reviewed changes

lib/web_ui/test/ui/fragment_shader_test.dart Outdated Show resolved Hide resolved

Moncader and others added 3 commits June 19, 2024 10:00

fix: web canvaskit fragment shaders were not using updated uniform va…

331c2d7

…lues

Added/fixed tests for reusing Paint objects with fragment shaders

69994ae

Update lib/web_ui/test/ui/fragment_shader_test.dart

7a0dfcc

Co-authored-by: Jackson Gardner <[email protected]>

Moncader force-pushed the fix/web-shader-uniforms branch from e65456a to 7a0dfcc Compare June 19, 2024 01:00

eyebrowsoffire suggested changes Jun 20, 2024

View reviewed changes

Moncader added 2 commits June 21, 2024 10:02

free the pointer made for the float uniforms in canvaskit fragment sh…

2c8681f

…aders.

do not make a copy of uniform data for fragment shaders in skwasm

943832e

eyebrowsoffire approved these changes Jun 25, 2024

View reviewed changes

yjbanov approved these changes Jun 25, 2024

View reviewed changes

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 25, 2024

auto-submit bot merged commit ee1884c into flutter:main Jun 25, 2024

engine-flutter-autoroll mentioned this pull request Jun 25, 2024

Roll Flutter Engine from 62e0b5f9c340 to ee1884c88383 (6 revisions) flutter/flutter#150795

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024

ee1884c88 fix: web canvaskit fragment shaders were not using updated …

3a726c9

…uniform values (flutter/engine#53246)

engine-flutter-autoroll mentioned this pull request Jun 25, 2024

Roll Flutter Engine from 62e0b5f9c340 to 94023d711db3 (7 revisions) flutter/flutter#150797

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024

ee1884c88 fix: web canvaskit fragment shaders were not using updated …

315df9c

…uniform values (flutter/engine#53246)

Moncader deleted the fix/web-shader-uniforms branch June 26, 2024 02:06

fix: web canvaskit fragment shaders were not using updated uniform values #53246

fix: web canvaskit fragment shaders were not using updated uniform values #53246

Uh oh!

Conversation

Moncader commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jun 6, 2024

Uh oh!

Moncader commented Jun 6, 2024

Uh oh!

eyebrowsoffire commented Jun 7, 2024

Uh oh!

flutter-dashboard bot commented Jun 10, 2024

Uh oh!

Moncader commented Jun 11, 2024

Uh oh!

Moncader commented Jun 14, 2024

Uh oh!

IntensiCode commented Jun 17, 2024

Uh oh!

Moncader commented Jun 18, 2024

Uh oh!

Uh oh!

flutter-dashboard bot commented Jun 19, 2024

Uh oh!

eyebrowsoffire commented Jun 19, 2024

Uh oh!

Moncader commented Jun 19, 2024

Uh oh!

eyebrowsoffire commented Jun 20, 2024

Uh oh!

eyebrowsoffire Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

Moncader commented Jun 21, 2024

Uh oh!

flutter-dashboard bot commented Jun 21, 2024

Uh oh!

eyebrowsoffire commented Jun 21, 2024

Uh oh!

flutter-dashboard bot commented Jun 25, 2024

Uh oh!

Moncader commented Jun 25, 2024

Uh oh!

eyebrowsoffire left a comment

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Moncader commented Jun 6, 2024 •

edited

Loading