Clip accelerated descendants of an accelerated layer having border radius and clip overflow

Clip accelerated descendants of an accelerated layer having border radius and clip overflow

BUG=157218
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162463

[rosca, 2013-07-22 19:32:49]

There are 2 cases for the issue 157218:
1) the clipping layer having border radius is an accelerated layer and it clips
accelerated descendants
2) an accelerated layer should be clipped by one or more non-accelerated
ancestors having border radius.

This patch fixes the first case. It sets up a mask on the childContainmentLayer.
Some of the code is shared with https://codereview.chromium.org/16688004/,
because I use the same mask to clip child layers as for clipping the accelerated
contents.

I intend to implement the second case with another patch by setting up a mask on
the ancestorClippingLayer.

[jamesr, 2013-07-25 22:21:35]

the compositor interactions lgtm. I'd prefer someone familiar with
RenderObject+painting to double check the offset math

[eseidel, 2013-07-29 19:17:18]

I'm confused by what this new paint-phase means.  It doesn't look like it
interacts with SVG at all (its RenderBlock specific?) is that OK?

Historically the PaintPhases were all defined by CSS paint order, but I guess
that's no longer true:
http://www.w3.org/TR/CSS21/zindex.html#painting-order

[rosca, 2013-07-29 21:48:10]

On 2013/07/29 19:17:18, eseidel wrote:
> I'm confused by what this new paint-phase means.  It doesn't look like it
> interacts with SVG at all (its RenderBlock specific?) is that OK?
> 
> Historically the PaintPhases were all defined by CSS paint order, but I guess 
> that's no longer true:
> http://www.w3.org/TR/CSS21/zindex.html#painting-order

This paint phase is valid only for hardware composited elements and it's called
on an auxiliary layer, so this phase is never mixed up with the main layer's
phases. The auxiliary layer is set up as a mask only for the composited elements
which should clip overflow using an irregular clip path (border-radius and,
later, clip-path), and only if there is any composited child to be clipped. I
think this is RenderBlock specific, isn't it? I should probably check that
renderer() is RenderBlock before painting this phase.

[Justin Novosad, 2013-11-20 00:18:46]

Thanks for adding me on this review.

Sad to see we were working on this problem concurrently.
I think the approach of https://codereview.chromium.org/59613010/ is more
general because it addresses the issues of nested RenderBlocks with cumulative
clipping, render layer position offset accumulation through the layer hierarchy,
and cumulative CSS transforms between the clip being applied and the layer being
rendered.

[rosca, 2013-11-20 01:36:25]

On 2013/11/20 00:18:46, junov wrote:
> Thanks for adding me on this review.
> 
> Sad to see we were working on this problem concurrently.
> I think the approach of https://codereview.chromium.org/59613010/ is more
> general because it addresses the issues of nested RenderBlocks with cumulative
> clipping, render layer position offset accumulation through the layer
hierarchy,
> and cumulative CSS transforms between the clip being applied and the layer
being
> rendered.

On 2013/11/20 00:18:46, junov wrote:
> Thanks for adding me on this review.
> 
> Sad to see we were working on this problem concurrently.
> I think the approach of https://codereview.chromium.org/59613010/ is more
> general because it addresses the issues of nested RenderBlocks with cumulative
> clipping, render layer position offset accumulation through the layer
hierarchy,
> and cumulative CSS transforms between the clip being applied and the layer
being
> rendered.

No problem, this was a quick and not incomplete solution, clipping only
accelerated descendants.

I was thinking initially of 2 steps for clipping any accelerated layer with
parent's border radius (and it should be adaptable for clip-path). Both steps
implied using masks for clipping, set on auxiliary clipping layers
(childContainmentLayer and ancestorClippingLayer). Assume the accelerated
element E should be clipped by ascendants P1, P2 and P3 (Tree is like Root -> P3
-> P2 -> P1 -> E). So, P1, P2 and P3 have border-radius + overflow hidden, P3
and E are accelerated, P2 and P1 are not accelerated.
1) P3 creates a child containment/clipping layer and uses a mask on it to clip
all its children, including E. This should be more efficient than propagating
clip path to all its descendants.
2) to clip E with P3 and P2 border radius, we can compute the cumulated clip
path for P1 and P2 (what your patch actually does) and use it with a mask on the
ancestor clipping layer of E. This way we clip E using P1 and P2, without
recursing/clipping through all the descendants.

So, this patch was intended to implement 1). I think your patch does the clip
accumulation needed in 2), but applies clipping on each element in the subtree
instead of using a clipping mask (please correct me if I'm wrong).

What do you think of this approach? Should it be more efficient in term of
performance?

[eseidel, 2013-11-20 04:26:51]

RenderBlock.cpp changes lgtm.

[eseidel, 2013-11-20 04:27:16]

CCing Levi, current effective owner of RenderBlock.

[Justin Novosad, 2013-11-20 14:48:37]

On 2013/11/20 01:36:25, Rosca wrote:
> 
> So, this patch was intended to implement 1). I think your patch does the clip
> accumulation needed in 2), but applies clipping on each element in the subtree
> instead of using a clipping mask (please correct me if I'm wrong).
> 
> What do you think of this approach? Should it be more efficient in term of
> performance?

The approach I took in my CL was really based getting correct results in as many
cases as possible (correctness first) and I took an approach that I think is
very straightforward, but there are several inefficiencies with
https://codereview.chromium.org/59613010/
1) Each accelerated layer that is a child of a "complex clip" gets its own child
clipping mask, which can be redundant in many cases,
2) The paths used to construct the clipping mask are re-rasterized for each
individual accelerated layer that is affected by it.

I think you have in mind a strategy to get to correct results without these
inefficiencies.  If we go ahead with this CL, I will file bugs corresponding to
the layout tests created for the other CL, so that we can improve upon this
incrementally.

[rosca, 2013-11-20 15:54:36]

On 2013/11/20 14:48:37, junov wrote:
> The approach I took in my CL was really based getting correct results in as
many
> cases as possible (correctness first) and I took an approach that I think is
> very straightforward, but there are several inefficiencies with
> https://codereview.chromium.org/59613010/
> 1) Each accelerated layer that is a child of a "complex clip" gets its own
child
> clipping mask, which can be redundant in many cases,
> 2) The paths used to construct the clipping mask are re-rasterized for each
> individual accelerated layer that is affected by it.
You are applying clipping mask on descendants at paint time (excepting for
accelerated content - canvases, videos, etc). This means that if an accelerated
child of a "complex clip" gets transformed/animated you'll need to recompute the
clip path and repaint layer content? Instead, if you apply the clipping mask on
graphics layer it should not require any repaint.

> I think you have in mind a strategy to get to correct results without these
> inefficiencies.  If we go ahead with this CL, I will file bugs corresponding
to
> the layout tests created for the other CL, so that we can improve upon this
> incrementally.

I was thinking of touching RenderLayerCompositor::ClippedByAncestor [1] to
decide if there is any ascendant software layer that clips the composited layer.
The complex clip path will clip only the mask which will be set on the ancestor
clipping layer. This way we do not clip the actual content.

So, are you agree with this approach, should I land this patch?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Justin Novosad, 2013-11-20 16:10:40]

On 2013/11/20 15:54:36, Rosca wrote:
> On 2013/11/20 14:48:37, junov wrote:
> > The approach I took in my CL was really based getting correct results in as
> many
> > cases as possible (correctness first) and I took an approach that I think is
> > very straightforward, but there are several inefficiencies with
> > https://codereview.chromium.org/59613010/
> > 1) Each accelerated layer that is a child of a "complex clip" gets its own
> child
> > clipping mask, which can be redundant in many cases,
> > 2) The paths used to construct the clipping mask are re-rasterized for each
> > individual accelerated layer that is affected by it.
> You are applying clipping mask on descendants at paint time (excepting for
> accelerated content - canvases, videos, etc). This means that if an
accelerated
> child of a "complex clip" gets transformed/animated you'll need to recompute
the
> clip path and repaint layer content? Instead, if you apply the clipping mask
on
> graphics layer it should not require any repaint.
> 
> > I think you have in mind a strategy to get to correct results without these
> > inefficiencies.  If we go ahead with this CL, I will file bugs corresponding
> to
> > the layout tests created for the other CL, so that we can improve upon this
> > incrementally.
> 
> I was thinking of touching RenderLayerCompositor::ClippedByAncestor [1] to
> decide if there is any ascendant software layer that clips the composited
layer.
> The complex clip path will clip only the mask which will be set on the
ancestor
> clipping layer. This way we do not clip the actual content.
> 
> So, are you agree with this approach, should I land this patch?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

lgtm

[commit-bot: I haz the power, 2013-11-20 16:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/19954010/13001

[commit-bot: I haz the power, 2013-11-20 16:14:55]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 473.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
c63031529b842337984f0a909fc810f43daa65d3..d1ac8c919c14af89a4d91d44932d13b672d2d379
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -473,6 +473,7 @@ crbug.com/239225
fast/css3-text/css3-text-decoration/text-underline-position/tex
 crbug.com/239225
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-auto.html
[ NeedsRebaseline ]
 crbug.com/239225
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under-out-of-flow.html
[ NeedsRebaseline ]
 crbug.com/239225
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under.html
[ NeedsRebaseline ]
+crbug.com/157218 fast/clip/overflow-border-radius-composited-parent.html [
NeedsRebaseline ]
 
 # This can't be rebaselined because it's a reftest.
 crbug.com/311423 animations/opacity-transform-animation.html [ ImageOnlyFailure
]

[commit-bot: I haz the power, 2013-11-20 16:24:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/19954010/203001

[commit-bot: I haz the power, 2013-11-20 17:49:14]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-21 09:09:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/19954010/353001

[commit-bot: I haz the power, 2013-11-21 10:18:24]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-11-21 11:07:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/19954010/353001

[commit-bot: I haz the power, 2013-11-21 11:07:57]

Change committed as 162463