[Impeller] Use downsample shader for blur instead of mip levels. #53760
Conversation
|
Marking ready for review so I can see goldens. |
jonahwilliams
changed the title
|
okay I think this should be good. There appear to be minor golden diffs but nothing that holds up to visual inspection. |
|
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. |
| renderer.MakeSubpass("Gaussian Blur Filter", pass_args.subpass_size, | ||
| command_buffer, subpass_callback); | ||
| return render_target; | ||
| if (pass_args.effective_scalar.x >= 0.5f) { |
There was a problem hiding this comment.
Since ui.Images will already have full mip chains generated and TiledTextureContents::RenderToSnapshot will passthrough in some cases, I think we can still go down this path to let mip filtering take care of the downsample when the input texture has a mip chain? Otherwise I think that case will regress.
There was a problem hiding this comment.
Hmm good point! Ill make it conditional based on mip count
|
Golden file changes are available for triage from new commit, Click here to view. |
jonahwilliams
added
the
autosubmit
…151495) flutter/engine@d3269d5...9d943eb 2024-07-09 [email protected] Avoid using a private GTest macro to skip tests. (flutter/engine#53782) 2024-07-09 [email protected] [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#53760) 2024-07-09 [email protected] [engine] support combined UI/Platform thread for iOS/Android. (flutter/engine#53656) 2024-07-09 [email protected] [Impeller] Enable framebuffer fetch tests disabled on OpenGL ES. (flutter/engine#53766) 2024-07-09 [email protected] [Impeller] implement experimental canvas in snapshot controller. (flutter/engine#53750) 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
|
This change has created shimmer in the blur at around 70 sigma. Sorry I don't have an automated test for this yet: beforebeforeblurchange.movafterafterblurchange.mov@jonahwilliams we probably want to revert this and try to reland it when we've addressed the shimmering. I'm going to try to come up with an automated test for this. |
|
I printed out the downscale factor to pinpoint it's relation to when the shimmering happens. At 1/16th downsample it is definitely shimmering. At 1/8th it mostly looks good until you get around sigma=40, then you can start to notice some artifacts. |
|
Here is just a comparison of the downsample pass output before and after this change. There seems to be some sort of repeated fractional scaling factor happening in both cases. We should be able to get rid of that. beforeoldjumper.movafterjumper.mov |
|
I've finally reproduced the problem in the shimmer test in flutter/flutter#152195. Without this PR the average rmse is 0.6, with this change it's 28. The only thing I changed was using the boston image. It didn't reproduce with the kalimba image for some reason. before the changeout.mp4after the changew-downsample-shader.mp4 |
|
Here's the comparison of the just the downsample output. We can see that the jittering isn't unique to this change. It's just that the 1/16 down-scaler is just lower quality. We should now focus our effort on just making that better instead of trying to remove the jitters (as I have been). beforeout.mp4afterjitters.mp4 |
…ls. (flutter#53760)" This reverts commit 04e4989.
…els. (flutter#53760)" This reverts commit dd48304.
…tter#53760) Fixes flutter/flutter#150973 On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.
…ls. (#53760)" (#54148) This is reverted since it introduces shimmering when the downsample scalar goes to 1/16th. It was reverted instead of fixed to allow the test that should have blocked this to land in the meantime. reverts #53760 fixed and relanded in #54149 Part of a series of gaussian blur changes: 1) #54148 1) #54116 1) #54150 1) #54149 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
…tter#53760) Fixes flutter/flutter#150973 On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.
…tter#53760) Fixes flutter/flutter#150973 On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.
…tter#53760) Fixes flutter/flutter#150973 On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.
…tter#53760) Fixes flutter/flutter#150973 On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.
…ls. (#54149) relands #53760 reverted in #54148 The fix for this was found with the help of Jonah. Part of a series of gaussian blur changes: 1) #54148 1) #54116 1) #54150 1) #54149 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Fixes flutter/flutter#150973
On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.