[Impeller] rrect_blur: scale max radius clamp by transform

[gaaclarke]

fixes https://github.com/flutter/flutter/issues/152778

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.

[flutter-dashboard[bot]]

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.

Changes reported for pull request #54350 at sha e1b349c89bcebd0d5fb1045b7d50ef7c29d1c45e

[gaaclarke]

This breaks an old golden test that is relying on a fixed 250 limit for the blur radius. But it is rendered at a content scalar of 1x so the limit is twice as big as with 2x. I'm not sure if we can reconcile this without knowing the content scalar.

[flar]

This breaks an old golden test that is relying on a fixed 250 limit for the blur radius. But it is rendered at a content scalar of 1x so the limit is twice as big as with 2x. I'm not sure if we can reconcile this without knowing the content scalar.

What's the purpose of the max? Legacy compatibility? Avoiding unbounded memory consumption or performance drains?

[gaaclarke]

What's the purpose of the max? Legacy compatibility? Avoiding unbounded memory consumption or performance drains?

Legacy compatibility I believe.

[gaaclarke]

Here's where it was introduced: https://github.com/flutter/engine/pull/42269. It was introduced to match the limit of the regular gaussian blur.

[flutter-dashboard[bot]]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54350 at sha 729fb4cda38d9022f69c65b525d42dfa7abc2f9b

[gaaclarke]

Here's rrect vs gaussian with larger sigmas. Before we even get to the clamp max the rrect blur is already dissipating faster than the gaussian blur.

[gaaclarke]

Adding @bdero to the review since he originally added this clamp. I think this is good now because:

1. The old golden test will pass
2. The area in question, rrect blur radius > 100 is already dissipating faster than the gaussian blur so clamping it at say 125 instead of 250 is inconsequential.

[flutter-dashboard[bot]]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54350 at sha 211f0dcd7cc4287f9e744625f10b040db9490e31

[gaaclarke]

I slept on this and I'm not so sure now what is the best max sigma. As written the golden tests don't break, but behavior of at 2x will be different. Maybe the 2x behavior is a better benchmark. I'm interested to hear what @bdero thinks.

[flutter-dashboard[bot]]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54350 at sha 8dc9a72cf473971f869eab653bd3b585458d406b

[gaaclarke]

I chatted with @bdero and he said the clamp was chosen solely as a mechanism to avoid hitting NaN's in the shader. That is the behavior we should maintain. He also said that the shader has changed since that was introduced so we may just be able to get rid of it.

[gaaclarke]

@bdero okay I went back to what I originally had. I believe this is correct. But after talking to brandon it's been decided the golden image will be changed but the results of the golden image aren't so important, it's that it renders something and doesn't crash. PTAL when you get a chance.

[flutter-dashboard[bot]]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54350 at sha ecb74096e9931f1076ecfaae7d4c158b9db79ebe

[flutter-dashboard[bot]]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

[chunhtai]

(triage) Hi @gaaclarke , can this be merged?

[gaaclarke]

No, it has conflicts now but it should be merged.

[jmagman]

@gaaclarke post-monorepo can you recreate this in the framework and close this engine PR?

[gaaclarke]

@gaaclarke post-monorepo can you recreate this in the framework and close this engine PR?

done: https://github.com/flutter/flutter/pull/161238