[Impeller] Skip mask blur with 0 sigma.#48457
Conversation
gaaclarke
left a comment
There was a problem hiding this comment.
Weird, this should be accounted for in the blur. I'm not sure what's going on here.
impeller/aiks/canvas.cc
Outdated
| return false; | ||
| } | ||
| // A blur sigma that is close to zero should not result in any shadow. | ||
| if (paint.mask_blur_descriptor->sigma.sigma <= kEhCloseEnough && |
There was a problem hiding this comment.
fabsf(paint.mask_blur_descriptor->sigma.sigma) <= kEhCloseEnough ?
| // A blur sigma that is close to zero should not result in any shadow. | ||
| if (std::fabs(paint.mask_blur_descriptor->sigma.sigma) <= kEhCloseEnough) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Could you add an early return to SolidRRectBlurContents too?
Because there's no guard against zero sigma, nothing is protecting us from div by zero in IPGaussian, so NaNs are probably propagating from the shader and getting mapped to a negative number on the float format we use for wide gamut.
There was a problem hiding this comment.
(Sorry for the late red light, I just realized what's probably happening here.)
There was a problem hiding this comment.
Good idea! Yes the nan issue, I remember it now. Thanks @bdero :)
|
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. |
flutter/engine@fd3a33f...97ede15 2023-11-28 jonahwilliams@google.com [Impeller] Skip mask blur with 0 sigma. (flutter/engine#48457) 2023-11-28 jonahwilliams@google.com [Impeller] match sigma scaling to Skia scaling. (flutter/engine#48434) 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
<0 sigma render incorrectly with wide gamut, but these shouldn't produce shadows at all as far as I can tell. flutter/flutter#138842
<0 sigma render incorrectly with wide gamut, but these shouldn't produce shadows at all as far as I can tell. flutter/flutter#138842
…9168) flutter/engine@fd3a33f...97ede15 2023-11-28 jonahwilliams@google.com [Impeller] Skip mask blur with 0 sigma. (flutter/engine#48457) 2023-11-28 jonahwilliams@google.com [Impeller] match sigma scaling to Skia scaling. (flutter/engine#48434) 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
<0 sigma render incorrectly with wide gamut, but these shouldn't produce shadows at all as far as I can tell.
flutter/flutter#138842