Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Use 32 bit Gaussian function in the 2-pass blur #42069

Merged
merged 1 commit into from
May 16, 2023

Conversation

bdero
Copy link
Member

@bdero bdero commented May 16, 2023

Resolves flutter/flutter#126487.

Increases the 2-pass blur quality and distribution limit.

It turns out sigma was breaking down beyond ~255 (moderately high, but not unreasonable for users to rely on). The Gaussian function computes sigma^2, and half precision floats only have 5 bit exponents and overflow for numbers above 65k.

Coincidentally, this also returns us to a state where we look a lot more like Skia's blurs for larger blur sigmas. Medium blurs have much less visual banding (although it's still there if you look closely). I suspect half precision isn't really enough for tracking the integral.

Unfortunately, this means our SIMD pipelining isn't going to be as good. I'll be interested in watching the blur-driven benchmarks for the perf hit.

Before Impeller:

Screen Shot 2023-05-15 at 11 14 20 PM

After Impeller:

Screen Shot 2023-05-15 at 11 12 18 PM

Skia:

Screen Shot 2023-05-15 at 11 16 58 PM

@bdero bdero self-assigned this May 16, 2023
@bdero bdero changed the title [Impeller] Increase 2-pass blur quality and distribution limit [Impeller] Use 32 bit Gaussian function in the 2-pass blur May 16, 2023
@bdero bdero force-pushed the bdero/blur-fidelity branch from 37e0f32 to d717881 Compare May 16, 2023 07:32
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

We could also compute the integral on the CPU and expose via SSBO if performance is a problem. I suspect it will be fine though, texture read/write is the bottleneck

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this turns out to be a regression for all sigmas, could we instead have two shaders, one for small sigmas, and one for large sigmas?

Or maybe only support up to some max sigma?

@jonahwilliams
Copy link
Member

jonahwilliams commented May 16, 2023

I think at that point we'd be better off computing the integral on the CPU.

@bdero bdero force-pushed the bdero/blur-fidelity branch from d717881 to 2c7c60b Compare May 16, 2023 19:14
@bdero
Copy link
Member Author

bdero commented May 16, 2023

We could also compute the integral on the CPU and expose via SSBO if performance is a problem.

The rub is that we still need the Gaussian function to weight the color, and so if we used a LUT for the integral we'd be exchanging only a few additions per cache miss (32 additions, since I think modern iPhones have 128 byte cache lines), which would likely work out to a net loss.

Precomputing the Gaussian function itself would probably work out to be a benefit if we're careful about cache management:

  • The distribution width is continuous, and so we'd need to choose a max density and linear sample.
  • To make the quality good enough for arbitrarily high radius values, we'd probably need to place few hundred samples in the LUT to avoid banding artifacts.
  • To avoid the extreme overhead of cache misses when sampling the distribution with a small radius, we could mipmap the texture.
    • The only downside here is that, unless we trilinear filter (and ~double up the cache misses), then we're going to get visual color popping between mip levels as the blur sigma is animated.

@bdero bdero force-pushed the bdero/blur-fidelity branch from 2c7c60b to 7c64188 Compare May 16, 2023 21:30
@bdero bdero force-pushed the bdero/blur-fidelity branch from 7c64188 to f642511 Compare May 16, 2023 21:34
@bdero bdero merged commit 87a03e1 into flutter:main May 16, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 17, 2023
…126968)

flutter/engine@1c775e3...87a03e1

2023-05-16 [email protected] [Impeller] Use 32 bit Gaussian function in the 2-pass blur (flutter/engine#42069)
2023-05-16 [email protected] Roll Skia from 9b0e912a1cb9 to 88d7a68694d9 (13 revisions) (flutter/engine#42081)
2023-05-16 [email protected] Use client ICU data with skwasm. (flutter/engine#42018)

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@chinmaygarde
Copy link
Member

From Triage: This is patched and we are waiting on framework benchmarks to determine viability for cherry-picking.

@bdero
Copy link
Member Author

bdero commented May 20, 2023

No relevant benchmark regressions showed up during the roll containing this change. Also, no malioc record adjustments. Texture fetches get reordered to start as soon as possible and happen in parallel, and so this is probably free because we have plenty of cycles to spare waiting for the texture sample within the scope of the loop.

@zanderso
Copy link
Member

@bdero Since there were no perf regressions and the patch seems low-risk, could you file a CP request for this? cc @chinmaygarde

bdero added a commit to bdero/flutter-engine that referenced this pull request May 22, 2023
…2069)

Resolves flutter/flutter#126487.

Increases the 2-pass blur quality and distribution limit.

It turns out sigma was breaking down beyond ~255 (moderately high, but
not unreasonable for users to rely on). The Gaussian function computes
sigma^2, and half precision floats only have 5 bit exponents and
overflow for numbers above 65k.

Coincidentally, this also returns us to a state where we look a lot more
like Skia's blurs for larger blur sigmas. Medium blurs have much less
visual banding (although it's still there if you look closely). I
suspect half precision isn't really enough for tracking the integral.

Unfortunately, this means our SIMD pipelining isn't going to be as good.
I'll be interested in watching the blur-driven benchmarks for the perf
hit.

(cherry picked from commit 87a03e1)
bdero added a commit to bdero/flutter-engine that referenced this pull request May 22, 2023
…2069)

Resolves flutter/flutter#126487.

Increases the 2-pass blur quality and distribution limit.

It turns out sigma was breaking down beyond ~255 (moderately high, but
not unreasonable for users to rely on). The Gaussian function computes
sigma^2, and half precision floats only have 5 bit exponents and
overflow for numbers above 65k.

Coincidentally, this also returns us to a state where we look a lot more
like Skia's blurs for larger blur sigmas. Medium blurs have much less
visual banding (although it's still there if you look closely). I
suspect half precision isn't really enough for tracking the integral.

Unfortunately, this means our SIMD pipelining isn't going to be as good.
I'll be interested in watching the blur-driven benchmarks for the perf
hit.

(cherry picked from commit 87a03e1)
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…lutter#126968)

flutter/engine@1c775e3...87a03e1

2023-05-16 [email protected] [Impeller] Use 32 bit Gaussian function in the 2-pass blur (flutter/engine#42069)
2023-05-16 [email protected] Roll Skia from 9b0e912a1cb9 to 88d7a68694d9 (13 revisions) (flutter/engine#42081)
2023-05-16 [email protected] Use client ICU data with skwasm. (flutter/engine#42018)

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@kuemme01
Copy link

It seems like after the fix the values you set to sigmaX/Y are doubled inside now. To match the result optically with my template in Figma I need to halve the desired value. Is there a new bug now?

@zanderso
Copy link
Member

@kuemme01 Could you file a new issue? It's a bit difficult for the team to triage comments on closed PRs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Impeller] ImageFilter.blur is not shown when sigma is greater than 85
5 participants