[Impeller] new blur: clamp downsample scalar to 1/16 #50262
Conversation
gaaclarke
changed the title
| @@ -16,7 +16,7 @@ struct KernelSample { | |||
|
|
|||
| uniform KernelSamples { | |||
| int sample_count; | |||
| KernelSample samples[32]; | |||
| KernelSample samples[48]; | |||
There was a problem hiding this comment.
Can we drop this down to 24 by only storing the samples from 0 to ?
I'm slightly concerned that we don't really have any tracking of the uniform size limit across our various backends.
There was a problem hiding this comment.
For the Pixel 5 the max UBO size is 64KB ( https://vulkan.gpuinfo.org/displayreport.php?id=27816#properties )
The vulkan 1.0 spec requires a minimuimum of 16384B ( https://registry.khronos.org/vulkan/specs/1.0-wsi_extensions/pdf/vkspec.pdf )
Assuming 32bit floats:
sizeof(KernelSample) = 12B
sizeof(KernelSample) * 48 = 576 bytes
I don't know if there is a definitive rule about this for opengles, but given the big gap between this an the 1.0 minimum I think we should be good.
edit: the ubo range limit is maxUniformBufferRange
There was a problem hiding this comment.
sizeof(KernelSample) is 16 bytes due to padding requirements. According to https://registry.khronos.org/OpenGL/specs/es/2.0/GLSL_ES_Specification_1.00.pdf
gl_MaxVertexUniformVectors = 128;
Which assume they mean 4 component float vectors, we're still below the limit. SGTM
Added test for reproducing blur shimmering when translating content under a blur
gaaclarke
force-pushed
the
animated-blur-test
branch
from
4cdced6
to
ff3f40e
Compare
|
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. |
gaaclarke
added
the
autosubmit
…142848) flutter/engine@fee0214...23763db 2024-02-03 chinmaygarde@google.com [Impeller] Fix blown stack due to always out of date window dimensions on high-dpi devices. (flutter/engine#50307) 2024-02-02 jason-simmons@users.noreply.github.com [Impeller] In advanced blends, unpremultiply and apply src_input_alpha only if there is no foreground color (flutter/engine#50260) 2024-02-02 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: clamp downsample scalar to 1/16 (flutter/engine#50262) 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 matanl@google.com,rmistry@google.com,zra@google.com 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
…#50363) issue: flutter/flutter#142753 After #50262 there were still some sigmas that could show shimmering around the sigma [50, 100] range which had a downsample amount of 1/16. This makes those ranges hang on to 1/8 for as long as possible. I'm unable to see any shimmering with `AiksTest.GaussianBlurAnimatedBackdrop` after this PR. I've also expanded that test so the clip region could be scaled to make sure that there aren't sizes which cause it to reappear. Testing: Expanded on manual testing. Since the error only manifests when evaluating multiple frames of rendering we don't have infrastructure to test that. Here is the graph of `GaussianBlurFilterContents::CalculateScale` after this change: <img width="903" alt="Screenshot 2024-02-05 at 2 10 41 PM" src="https://github.com/flutter/engine/assets/30870216/ac3b735a-95f0-4d7b-af12-58e1ae75278e"> ## 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
fixes flutter/flutter#142753
The theory here is that once you start throwing away more than 15/16th of the signal it's pretty noticable. By looking at the blur under different circumstances that seems like a reasonable limit.
There is no automated test for this. Doing so would be quite involved and would involve evaluating multiple rendered frames. There is a manual test.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.