Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Null check in BackdropFilterLayer::Diff #30448

Closed
jason-simmons wants to merge 1 commit into from
Closed

Null check in BackdropFilterLayer::Diff #30448

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions flow/layers/backdrop_filter_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ void BackdropFilterLayer::Diff(DiffContext* context, const Layer* old_layer) {
auto input_filter_bounds = paint_bounds.roundOut();
auto filter = filter_->makeWithLocalMatrix(context->GetTransform());

auto filter_bounds = // in screen coordinates
filter->filterBounds(input_filter_bounds, SkMatrix::I(),
SkImageFilter::kReverse_MapDirection);
if (filter) {
auto filter_bounds = // in screen coordinates
filter->filterBounds(input_filter_bounds, SkMatrix::I(),
SkImageFilter::kReverse_MapDirection);

context->AddReadbackRegion(filter_bounds);
context->AddReadbackRegion(filter_bounds);
Copy link
Contributor

@flar flar Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@knopp knopp Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@knopp knopp Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@flar flar Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@knopp knopp Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@flar flar Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@flar flar Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Contributor

@flar flar Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@knopp knopp Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@flar flar Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe file an issue to follow up on that parameter.

}
}

DiffChildren(context, prev);
Expand Down