Null check in BackdropFilterLayer::Diff

[jason-simmons]

Fixes flutter/flutter#95211

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[flar]

We need to do something with the readback region if the filter transform fails.

I think a better solution would be to not use makeWithLocalMatrix. Instead, compute the filter bounds of the untransformed paint_bounds and then transform that expanded bounds by the context transform? @knopp ?

[knopp]

I'm afraid these are not strictly identical. If you compute filter bounds first and then transform it, you may end up with area that's either slightly larger or slightly smaller (probably not smaller because we would roundOut as last step). Because computing filter bounds rounds to integer boundaries, we want to do it as last step (in screen space), so that we get exact affected pixels.

That said, I definitely think we should do this as fallback if filter transform fails.

[knopp]

Just to demonstrate why computing filter bounds first is a problem. Imagine filter that inflates original dimensions by 5%, rect 10x10 pixels and subsequent 3x transform.

Filter bounds in local coordinates:

• 10x10 original rect
• 10.5x10.5 rect with filter transform,
• 11x11 rect rounded by filterBounds
• 33x33 after applying transform

Filter bounds in screen coordinates:

• 10x10 original rect
• 30x30 transformed
• 31.5x31.5 with filter transform
• 32x32 rounded by filterBounds

[flar]

There is an accuracy difference here, but it should not cause a functional error. If we are worried about the accuracy then we could use the local bounds technique as a backup plan as you say.

Consider, though, that many image filters are already conservative. A gaussian blur with sigma=5 ends up adding almost 4 pixels of padding, not because it deliberately adds extra pixels, but because the theoretical math for determining which pixels are affected differs from the real-life results by a fairly wide margin.

But, the primary issue here is that we can't exit this code without setting some sort of readback region, whether by switching to a more reliable generalized calculation, or by making sure we implement a backup.

[knopp]

The larger value is wrong. The error originates from filterBounds rounding out to integer (and then we exacerbate the error with subsequent transform). Let's say that the transform is 10x. Then it would be 110x110 (wrong), vs 105x105 (right). The larger value result in unnecessary readback. Of course that is still better than no readback at all, so it's a good fallback if we can't transform the filter.

I'll have PR for this later today.

[flar]

For reference, this is how inaccurate SkPicture bounds are for a simple drawRect: https://fiddle.skia.org/c/c04a0df3ab37994a58fa46b881000b27

And for text: https://fiddle.skia.org/c/e1b16a1e5dfa3ed13fbe61d084f5ddab

I've tried to do better with DL.bounds(), but there is a big tradeoff between code complexity, calculation time, and accuracy of bounds that leads to tradeoffs all throughout the bounds system.

[flar]

(Having said all of that - mainly to point out that the bounds being tracked in the system are already not as accurate as one might predict from the 2-stage calculation above, I did approve the other PR.)

[flar]

I think "overkill" is a strong word. It's more that if we are going to try this hard to avoid some pixel slop in the bounds here, we should probably take it as a task to really address the pixel bounds inaccuracies that are endemic to the bounds calculations.

In terms of impact, I don't think this one case of computing the bounds more accurately has a big impact compared to all of the other "roundouts" and other conservative approaches and estimates. It's better, but how much practical difference it makes is an open question.

Also, I wonder if the transform parameter that we seem to provide as an Identity matrix could already do what we are doing with extra code here. Have we investigated what happens if we just handed the context matrix to the filterBounds method?

[knopp]

The ctm argument in filterBounds seems undocumented, but if it does what we need then the code could be simplified. I tried replacing filter_->makeWithLocalMatrix with simply passing the matrix in filterBounds and the original test passed. The new test (with perspective matrix where makeWithLocalMatrix returns null) fails, but that does not necessarily means it's wrong. This is something we should look into.

[flar]

Maybe file an issue to follow up on that parameter.

[knopp]

I added new PR with unit test and fallback readback calculation.