[Impeller] started scaling the gaussian blur sigma to match skia output #47405
Conversation
|
I'm not sure what sort of test we should add. This will probably be picked up in existing goldens. |
| Sigma ScaleSigma(Sigma sigma) { | ||
| // Limit the kernel size to 1000x1000 pixels, like Skia does. | ||
| Scalar clamped = std::min(sigma.sigma, 500.0f); | ||
| Scalar scalar = 1.02 - 3.89e-3 * clamped + 4.36e-06 * clamped * clamped; |
There was a problem hiding this comment.
TIL you can use scientific notation in C++
There was a problem hiding this comment.
IIRC from reading the Skia code, it goes something like
- If the Sigma is <= 4, leave it alone.
- Otherwise scale the Sigma down to 4, and use that same scaling factor to adjust the texture size.
So ScaleSigma is related to the curve we use below to scale the texture size, as in the both need to approximately agree.
If this looks like an improvement overall, then LGTM, but we might end up adjusting the whole thing anyway
|
Here is the difference between skia and impeller with sigma 164 before this change, just to show this is an obvious improvement. 
|
|
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. |
|
Yeah that looks way better |
There was a problem hiding this comment.
Yes! Fix the look of your blur with this one line trick. :)
The scaledown purely adds to the blurriness. Our blurs already look pretty good, so just remap it to match Skia as a holdover.
We could always adjust this later with a more closely fitted curve, but this parabola looks pretty close!
|
I looked through the goldens and they all seemed kosher. Since there is existing coverage, I think I'm going to avoid adding another golden test. |
…137461) flutter/engine@7e2aa68...f1e30b4 2023-10-27 [email protected] [Impeller] started scaling the gaussian blur sigma to match skia output (flutter/engine#47405) 2023-10-27 [email protected] Roll Fuchsia Linux SDK from gPQSfYJVLOgXjxQce... to ESkamdoXWIwkdWdP-... (flutter/engine#47404) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from gPQSfYJVLOgX to ESkamdoXWIwk 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
3 participants
fixes flutter/flutter#127770
This will help performance as well since our kernels will be smaller. The justification for scaling the sigma is that we need to account in the math the scaling down that we are doing of the input.
Here are the side-by-side comparisons with this math and skia, impeller is always on the right:
sigma 10
sigma 75
sigma 164 (sigma in bug)
sigma 400
sigma 1000
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.