Layer::Diff should use new LayerStateStack or DisplayListMatrixClipTracker objects

[flar]

The Layer::Diff method maintains tree state to perform its work. The other 2 recursion methods Layer::Preroll and Layer::Paint were recently modified to use a new LayerStateStack mechanism to manage their state as they recurse through a layer tree. The Diff method tracks state similarly, but doesn't necessarily use all of the mechanisms that the LayerStateStack object provides. Minimally, though, it tracks transform and clip which can now be maintained via the DisplayListMatrixClipTracker object which provides better support for 4x4 matrices and difference clipping.

Diff should minimally use at least DisplayListMatrixClipTracker for better (and future-proofed) clip/matrix tracking, and look at the full LayerStateStack to see if it provides needed functionality with minimal overhead so that all 3 recursion methods have essentially consistent, if not identical, state management code in them.

[flar]

@knopp thoughts?

[knopp]

I agree that all 3 should be consistent. I need to take a good look at the new classes to see how they're meant to be used.

Couple of things:

• Preroll and Paint already aren't consistent, because paint applies integralTransform and preroll doesn't. This complicates diff context (because it has to account for the difference). Can we fix this in layers?
• There is more state tracked in stack in the diff context, we still need to put it somewhere. But perhaps it can be separate from clip and transform.

[flar]

I noticed that too. We should fix the difference in the integralTransform in a separate PR for bisecting purposes if anything goes wrong.

With regard to the extra state. Preroll and Paint don't use it for all state that they track, just clip, transform, and mutators (opacity, filters)

[flar]

I think Diff probably wants to apply all of the bounds mutators in the "state stack" to a given layer's bounds? That feature is not present in the current LSS.

Moving to DisplayListMatrixClipTracker might be a simple short term change that would enable us to embrace full SkM44's in TransformLayer, and then moving over to LSS for everything else might be a lower priority longer term goal. But if it's not too bad, then having LSS manage it all for consistency would be the best end state.

[flar]

It would also be interesting to merge Diff/Preroll since both of them compute bounds, but in just slightly different ways. (Preroll computes local bounds, Diff computes destination bounds).

[knopp]

DiffContext result is used in preroll to cull layers. Also diff doesn't need to walk the entire tree (it skips retained layers).

[knopp]

I'll try applying integralTransform to preroll tomorrow to see how it works and whether i can remove the workaround from diff context. That should make it easier to move it to DisplayListMatrixClipTracker.

[flar]

DiffContext result is used in preroll to cull layers. Also diff doesn't need to walk the entire tree (it skips retained layers).

I don't think this is true. Preroll doesn't look at diff context info at all AFAIK. It produces local bounds for culling layers during Paint, but it doesn't take info from Diff.

[knopp]

https://github.com/flutter/engine/blob/d85707af098739a002489dce858d436d3246e69b/flow/compositor_context.cc#L128-L134

  std::optional<SkRect> clip_rect =
      frame_damage
          ? frame_damage->ComputeClipRect(layer_tree, !ignore_raster_cache)
          : std::nullopt;


  bool root_needs_readback = layer_tree.Preroll(
      *this, ignore_raster_cache, clip_rect ? *clip_rect : kGiantRect);

First clip_rect is being computed (using the DiffContext), then it is passed to LayerTree::Preroll as cull_rect.

[flar]

First clip_rect is being computed, then it is passed to LayerTree::Preroll as cull_rect.

It does maintain an item called "cull_rect", but it only uses it for determining whether it matters to generate a cache entry, not for culling painting or sub-trees in Preroll.

[flar]

If you look for calls to the LSS::content_culled method, it only happens from Paint methods or from RasterCacheItem methods. So, we do "cull" RasterCacheItems, in the sense that we mark them as not useful, but I wasn't considering that culling because it doesn't abort a sub-tree. So, ... terminology ...

[knopp]

I think originally it used to affect needs_painting(), that was later changed to query actual clip from canvas. So maybe the cull rect in preroll it's less useful now.

[flar]

Also, bdfLayer sets its bounds to the (local_)cull_rect.

[knopp]

Right. So it was that before flutter/engine@ecfe5ae#diff-b1762f426be15321282c8ff8ecf76dcd351c0f2b401f1012d0002a34be310e33 we we're calculating layer paint bounds after intersection with cull rect. That's the main reason why Diff was/is a separate step.

But that's not the case anymore so maybe it's time to reconsider.

I think bdf setting bounds to cull_rect would be fine even if cull rect is not clipped.

[flar]

I looked for all references to it in the LSS PR (flutter/engine#37394) and only found culling of RCItems and BDFLayer joining it into its paint bounds. All of the rest of the code (in the Preroll phase) was modifying it and resetting it when updating the transform.

Paint does do culling, but that is based on the SkCanvas/DLBuilderRecorder

[flar]

I think bdf setting bounds to cull_rect would be fine even if cull rect is not clipped.

The cull_rect is the clip - well the bounds of it.

[flar]

We might also want to consider computing layer paint bounds in device space? Wait, um, no - that would end up making a mistake when calling Paint on a layer from the raster cache code in a private layer with a new origin. Hmmm... The bounds would still be the same scale/skew - but a different origin, so it might be faster than having to transform local bounds... If we store a "surface origin" in the PaintContext then we could maybe get away with this...

[flar]

Oh, uh, then we have the issue where we have to apply a filter and that works better with local bounds. Ugh. So many ways to need the data in different coordinate frames.

[flar]

One idea I have for the long run is to have a bounds object that stores a quad so we don't end up with messy "bounding box expansion" errors every time we have to switch coordinate reference frames. It can compute a bounds if needed, but it stores and computes on the corners.

[knopp]

For diff context the final result is in device coordinates. So it only makes sense to store painted area for each layer in device coordinates. It would make it quite more difficult to process things in local space and in the end we need to convert to device space anyway.

As far as diff context is concerned it should only ever be one transform per rect (local to device). Once in device space we don't do any other transformations, with the exception of imagefilterlayer, where each affected rect is inflated by filter->map_device_bounds (by filter that has been converted to device space). If this part turns to be problematic we can try doing this in local space, but we'll need to transform the filter either way.

Edit. We also get the BDF paint rect by inverse transforming cull rect from device to local coordinates.

[flar]

I was thinking more in terms of convincing Preroll to work in device space rather than converting the Diff stuff the other way. I think always using device space involves the fewest transformations in general as you indicate.

[flar]

I filed the "Preroll/Paint/pixel snapping" issue as #116232

[knopp]

Also, I misremembered :-/ It's not that Preroll() and Paint() are inconsistent. That was the case before. Now the problem is just in Paint:

void ContainerLayer::PaintChildren(PaintContext& context) const {
  for (auto& layer : layers_) {
    // <<< Here, the `needs_painting` check works with non-integer CTM >>>
   if (layer->needs_painting(context)) {
      layer->Paint(context);
    }
  }
}

void DisplayListLayer::Paint(PaintContext& context) const {
 
  // <<< We got here by passing check with non integer CTM >>>
  FML_DCHECK(needs_painting(context));

  if (context.raster_cache) {
    // <<< But now we paint children on integer CTM >>>
    mutator.integralTransform();
    ...
    }
  }

I'll have to think about this a bit.

[flar]

As I mentioned in the other issue, if Preroll computes the paint bounds to reflect the pixel snapping then that should all work just fine.

The paint bounds are relative to the parent, so if the content moves because this layer does pixel snapping, then it needs to report the snapped bounds to the parent...

[flar]

A piece of information that would help is if the snapping methods would return the amount by which they snapped the translation components.

Unfortunately, since they snapped the bounds by a fraction of a device pixel, and since Preroll is dealing with local coordinates, it will need to bias the paint bounds by the inverse transform of the device snapping distance.

[knopp]

@flar, you can take a look at flutter/engine#38010. It bit rought around the edges and I haven't tested it on device yet but all tests pass.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.