Add DiffContext

[knopp]

Description

Adds DiffContext class to facilitate diffing of layer trees.

Related Issues

flutter/flutter#33939

Tests

I added the following tests:

DiffContextTest.*

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the contributor guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the C++, Objective-C, Java style guides for the engine.
• I read the tree hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation.
• All existing and new tests are passing.
• I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

• I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.

[flar]

Should this use SkImageFilter->filterBounds(..., ReverseDirection, ...)?

[knopp]

Possibly. At least in ImageFilterLayer computeFastBounds seems wrong.

One thing I don't quite understand, the documentation for filterBounds says it maps device-space rect, but ImageFilterLayer seems to apply it on local coordinate rect?

[flar]

It looks like computeFastBounds would be a NOP in this case, just returning the cull_rect. It apparently just does a union of the input bounds without considering how the filter will modify them. The bounds you pass in is the bounds to assume for any null inputs (null inputs represent source data that will be supplied later when you perform the filter).

[knopp]

computerFastBounds works for blur (I tested it). But I can use filterBounds instead. However this is one thing I'm unclear about:

void ImageFilterLayer::Preroll(PrerollContext* context,  SkMatrix& matrix) 
...
 if (filter_) {
    const SkIRect filter_input_bounds = child_bounds.roundOut();
    SkIRect filter_output_bounds =
        filter_->filterBounds(filter_input_bounds, SkMatrix::I(), SkImageFilter::kForward_MapDirection);
    child_bounds = SkRect::Make(filter_output_bounds);
}

filterBounds is supposed to map device space rects, so why is it used here on layer space rect?

[flar]

filterBounds maps device space rects wrt the transform you give it, which is Identity in this case.

[flar]

We may be losing some accuracy here. There may be a subtle difference between tx(filterBounds(rect)) and filterBounds(tx(rect)).

Part of me is now wondering why paint bounds is in layer space. I suppose it could be so that retained layers don't change their paint bounds info if an ancestor changes its transform.

[knopp]

Yeah. In most cases, (2x 3x pixel scaling) the paint bounds will be larger than they should be. In case where the layer is scaled down it may actually paint outside of paint_bounds. Not directly related to diffing but something that we might want to fix for partial redraw because it could cause subtle issue for layers that are scaled down.

[knopp]

Should be fixed here.

[flar]

The link no longer seems to go anywhere, but in looking through the latest version of this code, I think the filter needs to be used with "makeWithLocalMatrix" as we do here in image_filter_layer when we are applying the filter to a cached version.

[knopp]

Fixed here.

[flar]

Looking at the test failures there seems to be a problem with resolving dependencies based on differing NNBD settings. A rebase to ToT might help with that.

Other than that, is there anything else holding this up? Can the WIP labels be removed so we can review this and get it in?

[knopp]

I plan to rebase this over the weekend and put the tests to respective layer test classes.

[flar]

Sorry for the delays, I'm going to start a final pass over this today.

[iskakaushik]

maybe file a github issue for this.

[flar]

@chinmaygarde @zanderso the specific files that I would like to see some review from your perspective would be:

main focus:

• diff_context.[cc,h]

support:

• scene_builder.[cc,h]
• layer.[cc,h]
• container_layer.[cc,h]

the rest of the files fall mainly into the category of just implementing some supporting methods on the various layers, but the new code is focused in the first 2 files above.

[chinmaygarde]

@flar Is expecting to land this. @knopp This is good to go right?