Bug 80622 – [chromium] Refactor CCLayerTreeHostCommon: clean up clipRect and drawableContentRect design

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

The clipRect is being computed in all cases anyway, and therefore usesLayerClipping is probably unnecessary.

This is desirable because it reduces the risks in clipRect logic, since there would only be one variable (the clipRect) instead of two variables (usesLayerClipping and clipRect).

Additionally, removing usesLayerClipping has the following implications:
  - we have to be careful of the meaning of an empty clipRect.  it should always imply that the layer is not visible;  it should not imply that the layer is not clipped.
  - clipRect becomes very similar to visibleLayerRect, and its worth investigating if they can be merged in a follow-up bug.

[Dana Jansens]

(In reply to comment #0)
>   - we have to be careful of the meaning of an empty clipRect.  it should always imply that the layer is not visible;  it should not imply that the layer is not clipped.

Yeh that definitely needs to be changed in CCOcclusionTracker then. (After/during this CL?)

[Shawn Singh]

I think during this CL is probably fine.  =)

[Shawn Singh]

I'm having second thoughts about this.  As I tried to refactor to remove the root special cases before calcDrawTransforms, i ran into this issue again.   It seems like some test code behavior changed when clipRects were initialized differently than they are now.

The problem is inconsistent behavior across 3 locations where cilpRect is owned, and the confusion of possible states that the clipRect may have.

state 1: clipRect should not be applied
state 2: clipRect is empty, and should be applied, meaning that everything is clipped away.
state 3: clipRect is normal and should be applied.

(a) For layers, all 3 possible states need to be represented.

(b) For surfaces and quads, ideally state 2 is unnecessary, so we have encoded state 1 as an empty clipRect.

(c) For surfaces, another condition must be true - which is where I encountered some minor bugs and got stuck when refactoring the root - the owningLayer's parent must have usesLayerClipping == true.  The surface's clipRect could potentially be initialized when it should in theory be un-used because of this condition.

proposed solution #1:
I think the right way to simplify this is to always have usesLayerClipping + clipRect, and NOT encode an emptyRect to mean "don't apply this rect".  This way this precarious code remains readable, more robust, and consistent across all 3 uses of clipRect.

What do you guys think?   If you agree, I'll re-name this bug and submit a patch that does this.  It will require adding bool usesLayerClipping to some additional data structures, however.

proposed solution #2:
As a compromise, if you want to avoid messing with quads for now, we could still change the semantics on renderSurfaces to be consistent with layers.  Fixing that should probably be enough for me to get un-stuck on the other refactoring patch, at least.

[Dana Jansens]

It sounds like state 1 is causing the mess? But if the clip rect is large (equal to contentrect of target surface?) then that's the same as not applying anything right?

[Shawn Singh]

(In reply to comment #4)
> It sounds like state 1 is causing the mess? But if the clip rect is large (equal to contentrect of target surface?) then that's the same as not applying anything right?

If we want to remove state 1, we would have to support "infinite rects", so that we don't have to make assumptions about where/how the rect is positioned.  this is an approach that webcore has taken in a few places, and I've personally already encountered a few bugs related to that for things like overflow.

usesLayerClipping seems to be a better option for robustness.   I once toyed with the idea of proposing to add "infinite" size concept to rect data types, but now I'm not convinced that's a good idea - it would cause a domino effect of necessary refactors, so that size, point, quad, etc can take infinite concept into account, and tat would spill over into users of these data types making it more error prone than it is with tolerating overflow or usesLayerClipping boolean.

[Shawn Singh]

summary of offline discussion with Dana - if we start by assuming clipRect would never be infinite (limited to the viewport, at its largest), then we can probably remove state 1.   a boolean might be necessary, but probably it could be placed on the stack, as part of recursion in calcDrawTransforms, and not stored in layers.  we could probably then ensure that clipRect is a valid finite rect at all times for all layers and surfaces and quads.

[Adrienne Walker]

(In reply to comment #6)
> summary of offline discussion with Dana - if we start by assuming clipRect would never be infinite (limited to the viewport, at its largest), then we can probably remove state 1.   a boolean might be necessary, but probably it could be placed on the stack, as part of recursion in calcDrawTransforms, and not stored in layers.  we could probably then ensure that clipRect is a valid finite rect at all times for all layers and surfaces and quads.

That's my thought too.  There's always going to be a clipRect, whether it's the viewport or the size of a surface.  However, I realize that there's some point during CCLayerTreeHostCommon traversal where the size of the surface is unknown, and thus the clip rect is essentially infinite temporarily.

If it makes it easier to add an explicit boolean function, then let's do that.  If it's easier to do without, let's do that.  I don't feel too strongly, but I agree that we should be consistent.

[Shawn Singh]

Created attachment 152333 [details]
Refactor complete, but not thoroughly tested yet

[Shawn Singh]

This refactor has a wonderful refreshing feeling =)  I hope you guys feel the same.

(1) Removed clipRect() and usesLayerClipping() from layer types.  Users of LayerChromium and CCLayerImpl should use drawableContentRect().

(2) Removed a lot of confusing and error-prone code about drawableContentRect(), and instead refactored that computation into the recursion itself, where it makes much more sense.  Before this patch, drawableContentRect was a half-meaningless value that represented the aggregate drawable rect of the layer and its subtree.  Now, after the patch, drawableContentRect is the properly clipped rect of what the layer can draw (not counting scissoring/occlusion/visibility).

(3) Based on (2) above, I would like to propose re-naming drawableContentRect() to "clippedContentRectInTargetSpace()" because that's really what it is.  "drawable" could be potentially misleading, for example a layer hidden by an overflow clip could arguably be "drawable" but not "visible", that contradicts the meaning we've been using for "drawable".

(4) for now, clipRect still exists on surfaces, with the same isEmpty() encoding for when it should/shouldnot be used.  I'd prefer to remove that in a separate patch, since it requires toying with drawableContentRect() and making 2 accessors out of it (one with replica, one with clip + without replica)

(5) The list of args in calcDrawTransforms will reduce a little in the follow-up refactoring in 88953.

(6) I'll be testing with layout tests and unit tests and removing the FIXMEs before submitting the real patch.

(7) If you guys like the approach, then these bugs are swallowed by this one, and I'll make them dupes.
  https://bugs.webkit.org/show_bug.cgi?id=82485
  https://bugs.webkit.org/show_bug.cgi?id=80331

[Dana Jansens]

Comment on attachment 152333 [details]
Refactor complete, but not thoroughly tested yet

View in context: https://bugs.webkit.org/attachment.cgi?id=152333&action=review

OMG go Shawn! This is really cool :) I like clippedContentRectInTargetSpace also.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:154
> +static inline bool layerClipsSubtree(LayerType* layer)

yay

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,
>                                              const WebTransformationMatrix& fullHierarchyMatrix, const WebTransformationMatrix& currentScrollCompensationMatrix,
> +                                            const IntRect& clipRectFromAncestor, const bool ancestorClipsSubtree,
>                                              RenderSurfaceType* nearestAncestorThatMovesPixels, LayerList& renderSurfaceLayerList, LayerList& layerList,
>                                              LayerSorter* layerSorter, int maxTextureSize)

i guess indenting has to be fixed here :(

aren't the non-first lines supposed to be indented only 4 chars to avoid that?

[Adrienne Walker]

Comment on attachment 152333 [details]
Refactor complete, but not thoroughly tested yet

View in context: https://bugs.webkit.org/attachment.cgi?id=152333&action=review

I really like clippedContentRectInTargetSpace.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:390
> +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,

Nooooo, don't return one thing via a return value but modify other things via reference parameters.  This should also be reference parameter.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
>>                                              LayerSorter* layerSorter, int maxTextureSize)
> 
> i guess indenting has to be fixed here :(
> 
> aren't the non-first lines supposed to be indented only 4 chars to avoid that?

We should really make this a struct.  The number of parameters here makes me cry every time I touch it.

[Shawn Singh]

OK, thanks to both of you for the initial reviews =)   I'll come back probably ETA monday with fixed unit tests.

A few responses to Enne's comments...


> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:390
> > +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,
> 
> Nooooo, don't return one thing via a return value but modify other things via reference parameters.  This should also be reference parameter.

OK - so just to avoid redundancy between the existing bool return value, I'll make it void instead?

> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> >>                                              LayerSorter* layerSorter, int maxTextureSize)
> > 
> > i guess indenting has to be fixed here :(
> > 
> > aren't the non-first lines supposed to be indented only 4 chars to avoid that?
> 
> We should really make this a struct.  The number of parameters here makes me cry every time I touch it.

Sounds good - i'll try to do that in bug 88953.  I already was able to simplify the args in the public calcDrawTransforms invocations in that patch.

[Adrienne Walker]

(In reply to comment #12)

> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:390
> > > +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,
> > 
> > Nooooo, don't return one thing via a return value but modify other things via reference parameters.  This should also be reference parameter.
> 
> OK - so just to avoid redundancy between the existing bool return value, I'll make it void instead?

Yes please.  :)

[Shawn Singh]

*** Bug 80331 has been marked as a duplicate of this bug. ***

[Shawn Singh]

*** Bug 82485 has been marked as a duplicate of this bug. ***

[Shawn Singh]

Created attachment 153194 [details]
Work in progress, well tested, only few issues remain

(1) Did not yet address reviewers comments (2) debugged and fixed a large number of unit tests; most failing tests either had bad initialization, and some tests really did need to be updated because after this patch, the rootLayer does clip the layer tree. (3) need to rebase, a lot of related code has changed since 550 CLs ago.  Will submit patch for review soon.

[Shawn Singh]

(In reply to comment #10)
> (From update of attachment 152333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152333&action=review
> 
> OMG go Shawn! This is really cool :) I like clippedContentRectInTargetSpace also.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:154
> > +static inline bool layerClipsSubtree(LayerType* layer)
> 
> yay
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> > +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,
> >                                              const WebTransformationMatrix& fullHierarchyMatrix, const WebTransformationMatrix& currentScrollCompensationMatrix,
> > +                                            const IntRect& clipRectFromAncestor, const bool ancestorClipsSubtree,
> >                                              RenderSurfaceType* nearestAncestorThatMovesPixels, LayerList& renderSurfaceLayerList, LayerList& layerList,
> >                                              LayerSorter* layerSorter, int maxTextureSize)
> 
> i guess indenting has to be fixed here :(
> 
> aren't the non-first lines supposed to be indented only 4 chars to avoid that?

looking again at the style guide, I don't see anything that requires this.

I tried indenting them only 4 spaces, and it actually looks very cluttered to me personally.  I like the indentation aligned with the parentheses (which it wasn't when you wrote this comment, but I fixed it), because that makes it much visually easier to realize its the beginning of a function definition.

ok with you?

[Shawn Singh]

(In reply to comment #17)
> (In reply to comment #10)
> > (From update of attachment 152333 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152333&action=review
> > 
> > OMG go Shawn! This is really cool :) I like clippedContentRectInTargetSpace also.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:154
> > > +static inline bool layerClipsSubtree(LayerType* layer)
> > 
> > yay
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> > > +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,
> > >                                              const WebTransformationMatrix& fullHierarchyMatrix, const WebTransformationMatrix& currentScrollCompensationMatrix,
> > > +                                            const IntRect& clipRectFromAncestor, const bool ancestorClipsSubtree,
> > >                                              RenderSurfaceType* nearestAncestorThatMovesPixels, LayerList& renderSurfaceLayerList, LayerList& layerList,
> > >                                              LayerSorter* layerSorter, int maxTextureSize)
> > 
> > i guess indenting has to be fixed here :(
> > 
> > aren't the non-first lines supposed to be indented only 4 chars to avoid that?
> 
> looking again at the style guide, I don't see anything that requires this.
> 
> I tried indenting them only 4 spaces, and it actually looks very cluttered to me personally.  I like the indentation aligned with the parentheses (which it wasn't when you wrote this comment, but I fixed it), because that makes it much visually easier to realize its the beginning of a function definition.
> 
> ok with you?


I think it's also worth noting that I'm going to try and make structs out of this as many people have suggested/agreed =)  We should discuss that soon.

[Adrienne Walker]

(In reply to comment #17)
> (In reply to comment #10)
> > (From update of attachment 152333 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152333&action=review
> > 
> > OMG go Shawn! This is really cool :) I like clippedContentRectInTargetSpace also.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:154
> > > +static inline bool layerClipsSubtree(LayerType* layer)
> > 
> > yay
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> > > +static IntRect calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLayer, const WebTransformationMatrix& parentMatrix,
> > >                                              const WebTransformationMatrix& fullHierarchyMatrix, const WebTransformationMatrix& currentScrollCompensationMatrix,
> > > +                                            const IntRect& clipRectFromAncestor, const bool ancestorClipsSubtree,
> > >                                              RenderSurfaceType* nearestAncestorThatMovesPixels, LayerList& renderSurfaceLayerList, LayerList& layerList,
> > >                                              LayerSorter* layerSorter, int maxTextureSize)
> > 
> > i guess indenting has to be fixed here :(
> > 
> > aren't the non-first lines supposed to be indented only 4 chars to avoid that?
> 
> looking again at the style guide, I don't see anything that requires this.
> 
> I tried indenting them only 4 spaces, and it actually looks very cluttered to me personally.  I like the indentation aligned with the parentheses (which it wasn't when you wrote this comment, but I fixed it), because that makes it much visually easier to realize its the beginning of a function definition.
> 
> ok with you?

Personally, I hate indenting to match parenthesis in the same way that I hate all variable width vertical alignment in code: it creates extra work for anybody making changes and bloats diffs with whitespace modifications.

If a function takes a million parameters, it's going to be equally unclear no matter how it's indented.  :)

The style guide does not say one way or the other on this issue, although http://www.webkit.org/coding/coding-style.html#braces-one-line has an example of a function call that only gets indented by four spaces.  That's a weak argument, though.

[Shawn Singh]

Created attachment 153568 [details]
Well tested, cleaned up, ready for review

tested on linux manually, unit tests, and layout tests, no obvious regressions.  finally ready for review!

[Shawn Singh]

Based on comment 19 I opted for the 4-spacing.   I guess it doesn't really matter in the end, feel free to nitpick about it and I'll do it whichever way you want =)

Please note, some upcoming refactors that will follow-up:

  - removing a large amount of root-layer special case part 1 (from the outside of calcDrawTransforms)
  - reducing as much as possible the special-ness of root layer part 2 (from inside calcDrawTransforms)
  - grouping values into structs as follows: (1) given properties from the user of the compositor (2) computed properties (3) something that clarifies and enforces when values are valid related to the confusion data flow in the recursion of calcDrawTransforms
   - onward!

[Adrienne Walker]

Comment on attachment 153568 [details]
Well tested, cleaned up, ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=153568&action=review

I like this patch quite a bit.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:468
>          rootLayer->createRenderSurface();
>      rootLayer->renderSurface()->setContentRect(IntRect(IntPoint(0, 0), deviceViewportSize()));

In the future, I'd love if deviceViewportSize could just become a parameter on calcDraw and this render surface creation and content rect setting could move there too.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:407
> +    /* NOT const, valid only after recursion returns */ IntRect& drawableContentRectOfSubtree,

Could you move the 'out' parameter to the end so that it's not mixed in with all of the 'in' parameters?

I also don't think this needs such a warning on it; reference out parameters are a pretty regular occurrence. If you want to clarify, maybe just prepend the variable name with 'out' or 'resulting'? We do something similar for LayerTextureUpdater::prepareToUpdate.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:408
> +    const IntRect& clipRectFromAncestor, const bool ancestorClipsSubtree,

No need for const on bool params.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-548
> -    // FIXME: This seems like the wrong place to set this
> -    layer->setUsesLayerClipping(false);

<3

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:615
> +        if (ancestorClipsSubtree)
> +            renderSurface->setClipRect(clipRectFromAncestor);

If the ancestor doesn't clip the subtree, do you need to initialize this clip rect? Alternatively, since the surface gets post-recursion clip rect setting, maybe this clip rect code should get moved post-recursion?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:621
> +        // The new renderSurface here will correctly clip the entire subtree. So, we do
> +        // not need to continue propagating the clipping state further down the tree. This
> +        // way, we can avoid transforming clipRects from ancestor target surface space to
> +        // current target surface space that could cause more w < 0 headaches.
> +        subtreeShouldBeClipped = false;

Even if you don't want to transform the clip rect of the target of the contributing surface into that surface's spaces, wouldn't you want to pass down subtreeShouldBeClipped in the case of layerClipsSubtree(layer)?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:722
> +        if (!drawableContentRectOfChildSubtree.isEmpty()) {
> +            accumulatedDrawableContentRectOfChildren.unite(drawableContentRectOfChildSubtree);
> +            if (child->renderSurface())

Ooh, this loop is much better.

[Shawn Singh]

Thanks for review as always.  Inline replies below.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:468
> >          rootLayer->createRenderSurface();
> >      rootLayer->renderSurface()->setContentRect(IntRect(IntPoint(0, 0), deviceViewportSize()));
> 
> In the future, I'd love if deviceViewportSize could just become a parameter on calcDraw and this render surface creation and content rect setting could move there too.

Sounds good - I've actually already done that as part of 88593


> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:615
> > +        if (ancestorClipsSubtree)
> > +            renderSurface->setClipRect(clipRectFromAncestor);
> 
> If the ancestor doesn't clip the subtree, do you need to initialize this clip rect? Alternatively, since the surface gets post-recursion clip rect setting, maybe this clip rect code should get moved post-recursion?

Yes I think it should work to move it to the post-recursion chunk of code.  In the existing patch, the idea was that clipRect would remain empty if ancestor does not clip the subtree, but now I realize that is wrong, and it needs to be explicitly initialized.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:621
> > +        // The new renderSurface here will correctly clip the entire subtree. So, we do
> > +        // not need to continue propagating the clipping state further down the tree. This
> > +        // way, we can avoid transforming clipRects from ancestor target surface space to
> > +        // current target surface space that could cause more w < 0 headaches.
> > +        subtreeShouldBeClipped = false;
> 
> Even if you don't want to transform the clip rect of the target of the contributing surface into that surface's spaces, wouldn't you want to pass down subtreeShouldBeClipped in the case of layerClipsSubtree(layer)?

That situation is already accounted for in the layerClipsSubtree if-statement below this.  I'm happy to adjust it if you think its more readable - but maybe I think it just simply needs a comment.   I specifically chose the code the way it is to be a bit more robust to changes we might make; the layerClipsSubtree if-statement would indicate the correct semantics without any nuanced logic from previous if statements.

[Shawn Singh]

Created attachment 153842 [details]
diff between previous patch and upcoming next one

[Shawn Singh]

Created attachment 153843 [details]
addressed reviewer comments

[Adrienne Walker]

Comment on attachment 153843 [details]
addressed reviewer comments

R=me.

[WebKit Review Bot]

Comment on attachment 153843 [details]
addressed reviewer comments

Clearing flags on attachment: 153843

Committed r123375: <http://trac.webkit.org/changeset/123375>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.