Bug 72162 – [chromium] Move setVisibleRect() calls into calculateDrawTransformAndVisibility()

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Move setVisibleRect() calls into calculateDrawTransformAndVisibility()

[W. James MacLean]

Created attachment 114748 [details]
Patch

[W. James MacLean]

This patch isn't quite right - seems too complicated and fails one layout test (compositing/backface-visibility-hierarchical-transform.html).

Suggestions? I imagine there must be a more elegant way, but it has eluded me thus far.

I tried just computing/setting the visible rect at the end, but this seemed to be missing reflection cases.

[Shawn Singh]

I grappled with this, too, when shuffling things in LayerRendererChromium.  The problem is that the visibleLayerRect requires knowing the targetSurface contentRect, and that contentRect is not known until after the recursion of that entire subtree for the surface is done.  By that time, the layer of interest is no longer in scope.

Solution 1 (simple, but will probably change in the long run):

rename calculateDrawTransformsAndVisibility --> calculateDrawTransformsAndContentRects

Then, create a new wrapper, calculateDrawTransformsAndVisibility
{
    calculateDrawTransformsAndContentRects

    for every surface in renderSurfaceList
        for every layer in renderSurfaceList->layerList {
            handle special case of this layer actually drawing its RenderSurface rather than the layer itself
            compute visibleLayerRect.
            if visibleLayerRect is empty, remove the layer from the list.
        }
}


Solution 2:  (much more correct and elegant, but will be hard and maybe even not feasible)

I think we should revisit the entire visible rect logic and try to distill it, if possible.  I suspect that it can be cleaned up a LOT.  I'm not 100% aware of why all the rect logic is needed, but I think most of it has to do with clipping, and now that the code exists and works, it might be possible to apply incremental cleanups to reduce the rect complexity there.

Would people be OK with Solution 1, and then wjmaclean and I can work together to get solution two after our culling/scissoring stuff gets done?

[Adrienne Walker]

Comment on attachment 114748 [details]
Patch

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

I suspect the problem here is that calculateVisibleLayerRect uses the content rect of the target render surface, which hasn't been set yet until the surface (possibly a grandparent) has been processed, because its bounds grow to encompass all of its children.

I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface.  The visible rect for the top layer surface is just the viewport, which gets set elsewhere.  You would also need to set the visible rect before you processed all the children.  I'm not 100% sure, but I think approach seems plausible.

You could also then skip layers that were not visible or all the children of surfaces that were not visible, in the same way that we skip layers and surfaces for opacity.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:360
> +        paintContentsIfDirty(replicaLayer, replicaLayer->visibleLayerRect());

You could just make this paintContentsIfDirty(Layer*)

[Shawn Singh]

(In reply to comment #4)
> (From update of attachment 114748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114748&action=review
> 
> I suspect the problem here is that calculateVisibleLayerRect uses the content rect of the target render surface, which hasn't been set yet until the surface (possibly a grandparent) has been processed, because its bounds grow to encompass all of its children.
> 
> I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface.  The visible rect for the top layer surface is just the viewport, which gets set elsewhere.  You would also need to set the visible rect before you processed all the children.  I'm not 100% sure, but I think approach seems plausible.
> 
> You could also then skip layers that were not visible or all the children of surfaces that were not visible, in the same way that we skip layers and surfaces for opacity.

This solution sounds good to me, as well =)  And then after that we can discuss if its possible to distill the rect logic further.

[W. James MacLean]

(In reply to comment #4)
> (From update of attachment 114748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114748&action=review
> 
> I suspect the problem here is that calculateVisibleLayerRect uses the content rect of the target render surface, which hasn't been set yet until the surface (possibly a grandparent) has been processed, because its bounds grow to encompass all of its children.
> 
> I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface.  The visible rect for the top layer surface is just the viewport, which gets set elsewhere.  You would also need to set the visible rect before you processed all the children.  I'm not 100% sure, but I think approach seems plausible.

OK. Let me take a crack at that, and I'll see if I can get it to work.

> You could also then skip layers that were not visible or all the children of surfaces that were not visible, in the same way that we skip layers and surfaces for opacity.

Sounds good.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:360
> > +        paintContentsIfDirty(replicaLayer, replicaLayer->visibleLayerRect());
> 
> You could just make this paintContentsIfDirty(Layer*)

Yeah, I figured I'd do that once the rest worked properly :-)

[W. James MacLean]

(In reply to comment #4)
> (From update of attachment 114748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114748&action=review
> 
> I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface. 

> The visible rect for the top layer surface is just the viewport, which gets set elsewhere.

Question: I can understand this for the root layer, but if there is more than one RenderSurface, do the visible rects for surfaces that own these extra RenderSurfaces all use the ViewPort too? Or should they do something with their parent layer's visible rect?

[W. James MacLean]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 114748 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114748&action=review
> > 
> > I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface. 
> 
> > The visible rect for the top layer surface is just the viewport, which gets set elsewhere.
> 
> Question: I can understand this for the root layer, but if there is more than one RenderSurface, do the visible rects for surfaces that own these extra RenderSurfaces all use the ViewPort too? Or should they do something with their parent layer's visible rect?

OK, found where the sub-surface's contentRect gets set ... can I use the same technique to set the owning layer's visible rect?

[Adrienne Walker]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 114748 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114748&action=review
> > 
> > I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface. 
> 
> > The visible rect for the top layer surface is just the viewport, which gets set elsewhere.
> 
> Question: I can understand this for the root layer, but if there is more than one RenderSurface, do the visible rects for surfaces that own these extra RenderSurfaces all use the ViewPort too? Or should they do something with their parent layer's visible rect?

I was thinking this process could be iterative.  It's not their parent's visible rect that you're looking for here (because this isn't a bounding box hierarchy and a parent visible rect is entirely independent of a chlid's visible rect), but rather it's the visible rect for the target render surface that the layer is being drawn into.

Render surfaces themselves don't have any internal notion of a visible rect, so the visible rect for a CCRenderSurface is really CCRenderSurface::m_owningLayer->visibleRect().

[W. James MacLean]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #4)
> > > (From update of attachment 114748 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=114748&action=review
> > > 
> > > I'm thinking you could fix this by calculating it on the first pass down the tree by changing calculateVisibleLayerRect to use take the visible rect for the current surface's layer instead of implicitly using the content bounds for the surface. 
> > 
> > > The visible rect for the top layer surface is just the viewport, which gets set elsewhere.
> > 
> > Question: I can understand this for the root layer, but if there is more than one RenderSurface, do the visible rects for surfaces that own these extra RenderSurfaces all use the ViewPort too? Or should they do something with their parent layer's visible rect?
> 
> I was thinking this process could be iterative.  It's not their parent's visible rect that you're looking for here (because this isn't a bounding box hierarchy and a parent visible rect is entirely independent of a chlid's visible rect), but rather it's the visible rect for the target render surface that the layer is being drawn into.
> 
> Render surfaces themselves don't have any internal notion of a visible rect, so the visible rect for a CCRenderSurface is really CCRenderSurface::m_owningLayer->visibleRect().

OK, that helps, but it still leaves open the question of what is the correct way to compute the visible rect for the surface's owning layer when that layer is not the root layer. At present this seems to be based on the union of the child drawable content rects, as they determine the layer's drawable content rect which in turn defines the surface's content rect which in turn is used to define the visible rect. I'm not sure I understand how this leads to an iterative process, since we need to traverse the children first (unless you're proposing a separate reverse iteration through the layer list?).

[W. James MacLean]

(In reply to comment #10)
> OK, that helps, but it still leaves open the question of what is the correct way to compute the visible rect for the surface's owning layer when that layer is not the root layer. At present this seems to be based on the union of the child drawable content rects, as they determine the layer's drawable content rect which in turn defines the surface's content rect which in turn is used to define the visible rect. I'm not sure I understand how this leads to an iterative process, since we need to traverse the children first (unless you're proposing a separate reverse iteration through the layer list?).

OK, so setting visible rect on a surface-owning layer to the viewport *almost* does it ... we're zeroing in ...

[Adrienne Walker]

(In reply to comment #10)

> > Render surfaces themselves don't have any internal notion of a visible rect, so the visible rect for a CCRenderSurface is really CCRenderSurface::m_owningLayer->visibleRect().
> 
> OK, that helps, but it still leaves open the question of what is the correct way to compute the visible rect for the surface's owning layer when that layer is not the root layer. At present this seems to be based on the union of the child drawable content rects.

I see what you're getting at, I think.  I think you're going to have to touch up calculateVisibleLayerRect for layers that have explicit bounds or clip rects and maybe write a separate function for non-clipped surfaces.  Right now we're taking a fixed size layer content rect and transforming the target surface space into it and seeing what bounding box that creates.  However, for layers that don't have a content rect yet (and don't have a clip rect), we really need to transform the target surface's visible rect into "infinite" layer space and see what rect that bounds.  I think you could reuse the projectQuad bit of calculateVisibleRect on the target surface visible rect to do that math.


(In case going down this path has gotten too complicated, a second option here is to just add a second pass to walk the tree after calculateDrawTransformAndVisibility() and add a FIXME to combine the two passes in a later patch.  There's certainly enough goodness in not having setVisibleLayerRect calls while walking the tree, in my opinion.  I was hoping this was going to have less wrinkles to sort out, but that was probably too optimistic.)

[W. James MacLean]

Created attachment 115157 [details]
Patch

[W. James MacLean]

Comment on attachment 115157 [details]
Patch

Just posting for discussion, I don't expect this patch to be seriously reviewed (and certainly not landed).

[W. James MacLean]

(In reply to comment #12)
> (In reply to comment #10)
> 
> > > Render surfaces themselves don't have any internal notion of a visible rect, so the visible rect for a CCRenderSurface is really CCRenderSurface::m_owningLayer->visibleRect().
> > 
> > OK, that helps, but it still leaves open the question of what is the correct way to compute the visible rect for the surface's owning layer when that layer is not the root layer. At present this seems to be based on the union of the child drawable content rects.
> 
> I see what you're getting at, I think.  I think you're going to have to touch up calculateVisibleLayerRect for layers that have explicit bounds or clip rects and maybe write a separate function for non-clipped surfaces.  Right now we're taking a fixed size layer content rect and transforming the target surface space into it and seeing what bounding box that creates.  However, for layers that don't have a content rect yet (and don't have a clip rect), we really need to transform the target surface's visible rect into "infinite" layer space and see what rect that bounds.  I think you could reuse the projectQuad bit of calculateVisibleRect on the target surface visible rect to do that math.


Sounds good.

Just for interest's sake, I posted a patch that passes all but four layout tests using the naive approach of starting each surface at the full viewport size. I don't expect we'll proceed with it until calculateVisibleRect is fully cleaned up, but post it only for discussion purposes.
 
> (In case going down this path has gotten too complicated, a second option here is to just add a second pass to walk the tree after calculateDrawTransformAndVisibility() and add a FIXME to combine the two passes in a later patch.  There's certainly enough goodness in not having setVisibleLayerRect calls while walking the tree, in my opinion.  I was hoping this was going to have less wrinkles to sort out, but that was probably too optimistic.)

I'd like to suggest this:

1) In the short term I'd like to do the second-walk option, as it can be reliably done quickly to allow other work to progress.

2) I will, after it lands, and in parallel with other work, land the better version. I may need to request a little "white-board time" with Enne and/or Shawn and/or Nat before I'll be fully able to complete it, but that's a worthwhile investment at any rate.

I'll start working on this plan just in case everyone agrees.

[W. James MacLean]

Created attachment 115171 [details]
Patch

[W. James MacLean]

Here is the second-pass (short-term) approach.

[Adrienne Walker]

Comment on attachment 115171 [details]
Patch

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

Looks good to me, other than one tiny nit.  Thanks for doing this.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:452
> +        if (!renderSurface->drawOpacity())
> +            continue;
> +

nit: You shouldn't need this.  The early out about render surface opacity in calculateDrawEtcEtc should take care of that.

[W. James MacLean]

Created attachment 115183 [details]
Patch

[W. James MacLean]

(In reply to comment #18)
> (From update of attachment 115171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115171&action=review
> 
> Looks good to me, other than one tiny nit.  Thanks for doing this.  :)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:452
> > +        if (!renderSurface->drawOpacity())
> > +            continue;
> > +
> 
> nit: You shouldn't need this.  The early out about render surface opacity in calculateDrawEtcEtc should take care of that.

Fixed!

[Kenneth Russell]

Comment on attachment 115183 [details]
Patch

Looks good to me mainly based on enne's review.

[WebKit Review Bot]

Comment on attachment 115183 [details]
Patch

Clearing flags on attachment: 115183

Committed r100388: <http://trac.webkit.org/changeset/100388>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xianzhu Wang]

Comment on attachment 115183 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:354
> +        paintContentsIfDirty(renderSurfaceLayer->maskLayer());

2 questions:
1. Why setVisibleLayerRect to the whole content bounds of the renderSurfaceLayer instead of the actual visible rect?
2. Why isn't this setVisibleLayerRect moved into calculateDrawTransformAndVisibility()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:361
> +        paintContentsIfDirty(replicaLayer);

Is the visibleLayerRect set in calculateDrawTransformAndVisibility() for the replicaLayer?

[Shawn Singh]

(In reply to comment #24)
> (From update of attachment 115183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115183&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:354
> > +        paintContentsIfDirty(renderSurfaceLayer->maskLayer());
> 
> 2 questions:
> 1. Why setVisibleLayerRect to the whole content bounds of the renderSurfaceLayer instead of the actual visible rect?
> 2. Why isn't this setVisibleLayerRect moved into calculateDrawTransformAndVisibility()?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:361
> > +        paintContentsIfDirty(replicaLayer);
> 
> Is the visibleLayerRect set in calculateDrawTransformAndVisibility() for the replicaLayer?

I'll be looking at these rects in detail when making unit testing for CCLayerTreeHostCommon (but not very soon) ...  If you have some time pressure to get these questions answered, let me know and we can discuss offline. =)

[Xianzhu Wang]

(In reply to comment #25)
> I'll be looking at these rects in detail when making unit testing for CCLayerTreeHostCommon (but not very soon) ...  If you have some time pressure to get these questions answered, let me know and we can discuss offline. =)

Thanks. Just filed bug 75717 for tracking that.