[Impeller] rrect_blur: scale max radius clamp by transform #54350
Conversation
|
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. |
|
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? |
Legacy compatibility I believe. |
|
Here's where it was introduced: #42269. It was introduced to match the limit of the regular gaussian blur. |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
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. 
|
|
Adding @bdero to the review since he originally added this clamp. I think this is good now because:
|
|
Golden file changes are available for triage from new commit, Click here to view. |
| // See also: https://github.com/flutter/flutter/issues/152778 | ||
| TEST_P(DlGoldenTest, LargeDownscaleRrect) { | ||
| impeller::Point content_scale = GetContentScale(); | ||
| auto draw = [&](DlCanvas* canvas, const std::vector<sk_sp<DlImage>>& images) { |
There was a problem hiding this comment.
This lambda method seems gratuitous as it's only called once.
| Matrix basis_invert = entity.GetTransform().Basis().Invert(); | ||
| Vector2 max_sigmas = | ||
| Vector2((basis_invert * Vector2(250.f, 0.f)).GetLength(), | ||
| (basis_invert * Vector2(0.f, 250.f)).GetLength()); |
There was a problem hiding this comment.
This seems like a lot of calculations. Basically don't we want to avoid the case of the X sigma being scaled to more than 250 and same with the Y? We should be able to directly calculate the expansion factor of the matrix in both dimensions and then divide by that expansion factor. I think GetMaxBasisLengthXY() might do the trick?
There was a problem hiding this comment.
I reverted this since it's not quite the same. I'm pretty sure I want the min of those values not the max.
There was a problem hiding this comment.
You didn't do what I asked, but did something else entirely. I'm suggesting that you divide the largest sigma by the MaxBasisLength() - there is never any matrix inversion in that concept. Something like:
// Pseudo code as I don't think we have scalar/vector division defined.
// Actually, a scalar is fine if we can only pass along a single value to the next step.
max_sigmas = 250 / entity.GetTransform().MaxBasisLengthXY();
sigmas = min(sigmas, max_sigmas);
There was a problem hiding this comment.
That's why it was inverted. An inverted multiply is the same thing as a divide. That wasn't the issue though, MaxBasisLengthXY() returns one value and it's not the one we want. I want the minimum.
There was a problem hiding this comment.
It's even more complicated because I'm looking for the BasisLengths as a vector, but the methods only return a scalar. So perhaps something like this:
transform = entity.GetTransform();
max_sigmas = {250 / transform.GetXBasisLength(), 250 / transform.GetYBasisLength()};
sigmas = min(sigmas, max_sigmas);
We'd need the individual basis lengths from the transform, either delivered in a pair of Scalar methods, or in a method that returns a vector of both rather than the min/max of both.
There was a problem hiding this comment.
That's why it was inverted. An inverted multiply is the same thing as a divide. That wasn't the issue though, MaxBasisLengthXY() returns one value and it's not the one we want. I want the minimum.
An inversion takes almost 200 multiplies and 80 adds. It's not the same.
There was a problem hiding this comment.
MaxBasis takes the elements representing how much X distances and Y distances are expanded and finds the max of them. In other words, what is the biggest expansion factor we have for how big a distance will be expanded in either X or Y.
250 divided by that value is the largest user space sigma we can have that will be expanded to no more than 250 in device space once it is transformed.
There was a problem hiding this comment.
Why do you want the min? Is it because you only want to clamp in the smaller direction and let the other direction scale as much as it wants - to infinity?
(I think I get that part now. If you are working on the Inverted matrix then you'd want min, but that's a slower way to go and is missing the methods that would make it work - see next comment).
There was a problem hiding this comment.
Ah, yes, if you are doing Invert.MaxBasis then you are getting the max of the inverted values, but after inversion you want the min. But Invert is a really expensive operation in the first place and you are missing the MinBasis function you would want.
But, 1/MaxBasis (or 250/MaxBasis) uses the "max" that already exists, and does it before the multiplicative inverse (which is performed much faster on a Scalar than a matrix), so the computations work out to what they should have been if you used Invert.MinBasis and are much faster.
|
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. |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
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. |
|
@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. |
|
Golden file changes are available for triage from new commit, Click here to view. |
gaaclarke
added
the
autosubmit
|
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. |
|
(triage) Hi @gaaclarke , can this be merged? |
|
No, it has conflicts now but it should be merged. |
|
@gaaclarke post-monorepo can you recreate this in the framework and close this engine PR? |
done: flutter/flutter#161238 |
migrated PR flutter/engine#54350 ## 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], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [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/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
5 participants
fixes flutter/flutter#152778
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.