[Impeller] Do not clamp the local coverage limit to the root render target - #195860

Since the intersection with the root render target size is a no-op on the root pass (where pass_texture == root_RT) and incorrect on subpasses (where the subpass texture can extend beyond the root RT), we can completely remove the clamping logic and the clamp_to_root parameter.\n\nThis simplifies the API and avoids having to pass /*clamp_to_root=*/false at all the call sites (e.g., in SaveLayer and DrawDisplayList).\n\nIf we remove the clamping entirely, GetLocalCoverageLimit will naturally return the correct bounded limit for both root passes and subpasses.

Thanks, but I would rather not drop the clamp entirely. In SaveLayer, the coverage limit is not only an upper bound: ComputeSaveLayerCoverage returns it directly as the subpass size when the output floods or the bounds are unbounded. Unclamping it there would grow offscreen textures, and it is not needed for this bug.

I have narrowed the change so only DrawDisplayList (culling) uses the unclamped value, and SaveLayer is unchanged. I also replaced the tests with one that fails without the fix.

I think we do need to drop the clamp entirely.

Your fix covers the DrawDisplayList case, where DrawDisplayList is used inside a SaveLayer. It removes the clamp so when DrawDisplayList has contents drawn off screen, it doesn't get culled because the SaveLayer could have a filter that moves the off-screen contents on-screen.

The other case where GetLocalCoverageLimit is used is in SaveLayer to get the subpass size. Your fix leaves this unchanged. But I think this case has the same issue. If SaveLayer is called inside a parent SaveLayer, the child SaveLayer's off-screen contents could be moved on-screen by a filter on the parent SaveLayer. If the clamp is kept for this case, the child SaveLayer's off-screeen contents would get incorrectly culled and wouldn't get drawn when the parent SaveLayer's filter moves it on-screen.

This can be reproed with a modified version of your DrawDisplayListInTranslatedSaveLayerIsNotCulled test:

TEST_P(AiksTest, NestedSaveLayerInTranslatedSaveLayerIsNotCulled) {
  DlPaint green;
  green.setColor(DlColor::kGreen());

  DisplayListBuilder builder;
  builder.Scale(GetContentScale().x, GetContentScale().y);

  DlPaint paint;
  paint.setImageFilter(DlImageFilter::MakeMatrix(
      DlMatrix::MakeTranslation({300, 0}), DlImageSampling::kLinear));
  builder.SaveLayer(std::nullopt, &paint);
  // Control: drawn directly into the parent layer.
  builder.DrawCircle(DlPoint(-100, 100), 80, green);
  DlPaint child_paint;
  builder.SaveLayer(std::nullopt, &child_paint);
  // Drawn inside the child SaveLayer; not visible when SaveLayer clamps.
  builder.DrawCircle(DlPoint(-100, 300), 80, green);
  builder.Restore();
  builder.Restore();

  ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

As you mention, removing the clamping can result in growing offscreen textures. But this is correct and necessary. The offscreen texture must be sized to contain contents that ends up on-screen by the SaveLayer's translation.

You are right, thanks. The child SaveLayer case reproduces with plain dart:ui on 3.38.3 (missing on Impeller, present on Skia); I added it to the issue. Done in 4244c8d: the clamp is removed entirely and NestedSaveLayerInTranslatedSaveLayerIsNotCulled is added.

I think we do need to drop the clamp entirely.

Your fix covers the DrawDisplayList case, where DrawDisplayList is used inside a SaveLayer. It removes the clamp so when DrawDisplayList has contents drawn off screen, it doesn't get culled because the SaveLayer could have a filter that moves the off-screen contents on-screen.

The other case where GetLocalCoverageLimit is used is in SaveLayer to get the subpass size. Your fix leaves this unchanged. But I think this case has the same issue. If SaveLayer is called inside a parent SaveLayer, the child SaveLayer's off-screen contents could be moved on-screen by a filter on the parent SaveLayer. If the clamp is kept for this case, the child SaveLayer's off-screeen contents would get incorrectly culled and wouldn't get drawn when the parent SaveLayer's filter moves it on-screen.

This can be reproed with a modified version of your DrawDisplayListInTranslatedSaveLayerIsNotCulled test:

TEST_P(AiksTest, NestedSaveLayerInTranslatedSaveLayerIsNotCulled) {
  DlPaint green;
  green.setColor(DlColor::kGreen());

  DisplayListBuilder builder;
  builder.Scale(GetContentScale().x, GetContentScale().y);

  DlPaint paint;
  paint.setImageFilter(DlImageFilter::MakeMatrix(
      DlMatrix::MakeTranslation({300, 0}), DlImageSampling::kLinear));
  builder.SaveLayer(std::nullopt, &paint);
  // Control: drawn directly into the parent layer.
  builder.DrawCircle(DlPoint(-100, 100), 80, green);
  DlPaint child_paint;
  builder.SaveLayer(std::nullopt, &child_paint);
  // Drawn inside the child SaveLayer; not visible when SaveLayer clamps.
  builder.DrawCircle(DlPoint(-100, 300), 80, green);
  builder.Restore();
  builder.Restore();

  ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

As you mention, removing the clamping can result in growing offscreen textures. But this is correct and necessary. The offscreen texture must be sized to contain contents that ends up on-screen by the SaveLayer's translation.

The description is incorrect. The circles are drawn with one on top and one below, not side by side. "Left one" and "right one" should be "top one" and "bottom one".

Same for the description of the nested save layer test below.

Fixed in e508e3b, both test descriptions now say top/bottom.

This check is no longer needed. It was needed before, because we had to check that the optional had a value before making another Intersection call below. But that Intersection was removed. So we can just directly return maybe_coverage_limit (or return the result directly without a maybe_coverage_limit variable at all).

Done in e508e3b. Rect::Intersection already returns nullopt when the rects do not overlap and a non-empty rect otherwise, so it now returns the intersection directly.

With the updated code, I think this comment is a bit confusing. I'd remove the second half of it, just leaving:

  // Do not intersect with the root render target. Inside a subpass, content
  // outside the root RT can still end up on-screen (e.g. an ancestor image
  // filter translates it).

I think the rest just adds details that are confusing or not useful to someone trying to understand the code.

• "so both culling and nested subpass sizing must be bounded by the current pass texture and clip only." describes how the two current callers use the result. The function itself shouldn't need to know that, and the comment will go stale if callers are added or changed.
• "On the root pass the pass texture is the root RT, so the result is unchanged there" is saying that intersecting with the root render target would make no difference for the root pass case. But we're no longer doing this intersection in any case. So the comment only makes sense in the context of the PR and with knowledge of the old code. For someone just reading the new code, it's a confusing thing to note.

Done in 24e2988, thanks. I also merged master in dc568a4.