Fixes RuntimeEffect when used with ImageFilter.compose and gaussian blur #177687
Conversation
github-actions
bot
added
engine
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 addresses an issue with shader compose coordinates. The main change is in RuntimeEffectFilterContents, where input snapshots with non-identity transforms are now re-rasterized to bake the transform into the texture before being used by a runtime effect shader. This ensures the shader receives an input it can sample correctly. A new test, ComposeBackdropRuntimeOuterBlurInner, has been added to validate this scenario. Additionally, a 1-pixel coverage expansion in Contents::RenderToSnapshot has been removed.
engine/src/flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc
Show resolved
Hide resolved
|
Goldens that look wrong when removing the offset by 1:
All of the problems that look like the original reported issue the offset by 1 fixed were on opengles. |
… animating sigma" This reverts commit 32934f3.
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The current state of the code causes a one pixel shift when moving from sigma==0 to sigma>0. I can eliminate it by commenting out the expansion that happens in RenderToSnapshot ( ). We can't get rid of that because it's loadbearing. We need to find some way to counterbalance that extra padding that is being added.I've play around with shifting the source and dest of the TextureContents. I think that may be a way to do it. |
|
In the future we can potentially send the transform into the fragment shader, then we could avoid removing the one extra pixel of padding. It looks like removing that pixel of padding is making us get slightly different results on different platforms when sampling outside of the texture. This behavior is much better than what we had previously though. There is a previous comment where jonah said we could pass in this transform as part of an |
|
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. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| } | ||
| Rect input_coverage = maybe_input_coverage.value(); | ||
|
|
||
| // If the input snapshot does not have an identity transform the |
There was a problem hiding this comment.
I don't think we document this performance consideration anywhere. Perhaps a section in ImageFilter.shader?
There was a problem hiding this comment.
Another alternative that I am sure was suggested elsewhere could be to pass the transform into the shader as a uniform and push the responsibility of taking it into account during sampling. But I think this is a good compromise when it comes to ease-of-use and correctness.
There was a problem hiding this comment.
There is a previous comment where jonah said we could pass in this transform as part of an ImageFilter.shader2 api. But actually now that we have uniform-by-name we don't need a new API at the dart level, we could just look for a magic uniform name and set it.
Ah just read your followup. Yeah, that makes sense and should be easier to implement now with the uniform lookup by name stuff. But this patch is good from a correctness standpoint.
There was a problem hiding this comment.
Yea, we're definitely in the "make it work" phase, then we can "make it fast" later. I don't think we can document it as being slow to the user because there isn't actionable information. There is no good alternative. If we do something like passing in the transform to avoid the RenderToSnapshot conditionally, that would be the time to make sure to document it for users.
| texture_contents.SetSourceRect(bounds.value()); | ||
| texture_contents.SetDestinationRect(coverage); | ||
| texture_contents.SetStencilEnabled(false); | ||
| texture_contents.SetSamplerDescriptor(input_snapshot->sampler_descriptor); |
There was a problem hiding this comment.
Shouldn't the sampler be nearest-neighbor? The input snapshots sampler will then be applied when the texture is composited up?
There was a problem hiding this comment.
I don't think so. For the blur case this texture we are rendering to a snapshot is going to potentially be 1/8th the resolution of what we are drawing it as. So I think it's important we have linear sampling to make the blur look smooth.
There was a problem hiding this comment.
yea, looks much better with linear
nearest
linear
engine/src/flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc
Show resolved
Hide resolved
|
I'm going to file a follow up bug for the different results for the different backends when sampling outside of the texture. |
flutter-dashboard
bot
removed
the
autosubmit
flutter/flutter@6f8abdd...027f2e4 2025-10-31 engine-flutter-autoroll@skia.org Roll Dart SDK from bb45c4186fb2 to db168d9e7471 (1 revision) (flutter/flutter#177839) 2025-10-31 737941+loic-sharma@users.noreply.github.com Hide "waiting for customer" issues from text input triage (flutter/flutter#177524) 2025-10-31 matt.kosarek@canonical.com Making the multiple_windows example app demonstrate dialogs of dialogs (flutter/flutter#177786) 2025-10-31 bruno.leroux@gmail.com Fix ElevatedButton.icon breaks focus traversal and VoiceOver when toggling icon (flutter/flutter#177579) 2025-10-31 bruno.leroux@gmail.com Fix FilledButton.icon and FilledButton.tonalIcon break focus traversal and VoiceOver (flutter/flutter#177593) 2025-10-31 coldpalelight@gmail.com Fix cubic subdivision estimation using correct Wang’s formula (flutter/flutter#177758) 2025-10-31 engine-flutter-autoroll@skia.org Roll Skia from ccbd7697791f to 2cf9a1923078 (1 revision) (flutter/flutter#177832) 2025-10-31 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ksXeDDo2yYBXJ4uEu... to O-OoG6j4wHXd1ThNM... (flutter/flutter#177831) 2025-10-31 engine-flutter-autoroll@skia.org Roll Skia from 1532fabb4b7d to ccbd7697791f (4 revisions) (flutter/flutter#177828) 2025-10-31 engine-flutter-autoroll@skia.org Roll Dart SDK from cf24b43cb643 to bb45c4186fb2 (2 revisions) (flutter/flutter#177824) 2025-10-31 engine-flutter-autoroll@skia.org Roll Skia from 825d5c854302 to 1532fabb4b7d (3 revisions) (flutter/flutter#177808) 2025-10-30 engine-flutter-autoroll@skia.org Roll Dart SDK from da663596bf6d to cf24b43cb643 (2 revisions) (flutter/flutter#177798) 2025-10-30 jason-simmons@users.noreply.github.com Update the path used by the download_fuchsia_sdk.py script to //third_party/fuchsia-sdk (flutter/flutter#177794) 2025-10-30 engine-flutter-autoroll@skia.org Roll Skia from 6af53143b120 to 825d5c854302 (2 revisions) (flutter/flutter#177788) 2025-10-30 30870216+gaaclarke@users.noreply.github.com Fixes RuntimeEffect when used with ImageFilter.compose and gaussian blur (flutter/flutter#177687) 2025-10-30 47866232+chunhtai@users.noreply.github.com Adds cache extent type to two_dimentional_viewport (flutter/flutter#177411) 2025-10-30 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Update .ci.yaml in flutter/flutter to use 15.5 (#177669)" (flutter/flutter#177793) 2025-10-30 fluttergithubbot@gmail.com Marks Windows windowing_test to be flaky (flutter/flutter#177716) 2025-10-30 43054281+camsim99@users.noreply.github.com Fixes `SettingsChannelTest` flake (flutter/flutter#177061) 2025-10-30 30870216+gaaclarke@users.noreply.github.com Implements uniform-by-name for web (flutter/flutter#176980) 2025-10-30 okorohelijah@google.com Update .ci.yaml in flutter/flutter to use 15.5 (flutter/flutter#177669) 2025-10-30 jason-simmons@users.noreply.github.com [Impeller] Fall back to OpenGL ES on older Adreno GPUs (flutter/flutter#177747) 2025-10-30 jhy03261997@gmail.com [VPAT] Update a11y assessment app and guideline tests (flutter/flutter#177690) 2025-10-30 engine-flutter-autoroll@skia.org Roll Skia from 18457971c30f to 6af53143b120 (4 revisions) (flutter/flutter#177778) 2025-10-30 engine-flutter-autoroll@skia.org Roll Packages from 41c6b3d to 1a7075b (2 revisions) (flutter/flutter#177777) 2025-10-30 engine-flutter-autoroll@skia.org Roll Dart SDK from 4785d5971d64 to da663596bf6d (1 revision) (flutter/flutter#177772) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
2 participants
fixes #170820
fixes #177611
This PR snapshots the input of a runtimeeffect if its transform is not identity. This fixes the cases where we use compose with the runtime effect and the inner effect evaluates to a snapshot that doesn't match the size of x, such as
runtime_effect(blur(x)). Blur will perform at lower resolutions when the sigma is high as an optimization. Blur also adds a halo around what it is blurring. Previously we could optimize away this extra render pass when we assumed that the coverage of inner(x) was the same as x (but potentially just translated), but we can't do that in practice because of things like gaussian blur.In order to make this PR work we had to come up with a way to disable the extra padding that is added to every
RenderToSnapshot. This is because users want to to reference specific fragments. The extra border of padding would increase the texture size of inner(x) and x which created a jump in the locations calcuated in the fragment shaders when the inner snapshot's transform would jump from identity to non-identity. The only other alternative would be to pass in the offset to the fragment shader but that would be a breaking change.In order to get the shaders rendering in the right orientation on opengles I had to switch the shaders to do a
uv.y = 1.0 - uv.y. This is in the documentation for ImageFilter.shader but it is an annoying gotcha. Hopefully this doesn't break someone if the logic was different previously for ImageFilter.compose but this is the correct thing to do and since it wasn't tested, there is no breaking change.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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.