Bug 71038 – [chromium] Implicitly skip render surfaces that won't be drawn

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Implicitly skip render surfaces that won't be drawn

[Adrienne Walker]

Created attachment 112714 [details]
Patch

[Adrienne Walker]

Just a quick low-priority cleanup patch.

[Shawn Singh]

Comment on attachment 112714 [details]
Patch


Looks good to me, with one minor nit...


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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:318
>          renderSurface->setSkipsDraw(true);

Looking at CCRenderSurface::prepareContentsTexture, seems like we should remove this setSkipsDraw here (and the one inside the if-statement, too).  The logic inside the CCRenderSurface is already doing it.

[Adrienne Walker]

Created attachment 112719 [details]
Remove unnecessary skipsDraw

[James Robinson]

Comment on attachment 112719 [details]
Remove unnecessary skipsDraw

R=me

[Vangelis Kokkevis]

Comment on attachment 112719 [details]
Remove unnecessary skipsDraw

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

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

I think we could further short-circuit this by checking the opacity before even creating the render surface.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:385
> +        renderSurfaceLayerList.removeLast();

What if we clear the layer's RS so we don't have layer's pointing to empty RS's ?  That way also we won't have to check for empty RS's on line 366.

[Shawn Singh]

Enne and I discussed offline, it also seems that the skipsDraw flag inside of the CCRenderSurface is unnecessary too, and that change could be part of this patch, too.

[Adrienne Walker]

Created attachment 112793 [details]
Patch

[Adrienne Walker]

(In reply to comment #8)
> Created an attachment (id=112793) [details]
> Patch

Sorry to toss away the R+.  I implemented both of Vangelis's suggestions, because they sounded great.

Shawn: removing the skipsDraw flag from CCRenderSurface sounded good, but end up being possible.  It looks like it gets used in CCRenderSurface to specify that a render surface couldn't get reserved just like it's used for TiledLayerChromium.  If you can think of a better way to clean that up, you're welcome to have at it in a different patch.  :)

[WebKit Review Bot]

Comment on attachment 112793 [details]
Patch

Clearing flags on attachment: 112793

Committed r98757: <http://trac.webkit.org/changeset/98757>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Singh]

(In reply to comment #11)
> All reviewed patches have been landed.  Closing bug.

Did we notice https://bugs.webkit.org/show_bug.cgi?id=71150 ...  should we re-open this bug?

[Adrienne Walker]

I saw it.  Just hadn't reopened this yet.

[Adrienne Walker]

Created attachment 115521 [details]
Patch

[Adrienne Walker]

(In reply to comment #14)
> Created an attachment (id=115521) [details]
> Patch

Now with more passing tests.  The only changes from before are that the render surface is not cleared for the root layer (we make this check elsewhere) and I updated a test that was depending on a render surface being created to do some other tests, but that render surface was getting removed because its only child had no bounds.

[Vangelis Kokkevis]

Comment on attachment 115521 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:245
> +        if (!drawOpacity)

Wouldn't it be simpler to have a check for layer->opacity() == 0 at the very top of this function and early out before we do any further calculations?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383
> +    if (layer->renderSurface() && !layer->renderSurface()->layerList().size()) {

Won't the RS by definition contain in its layer list at least the layer that triggered its creation?

[Adrienne Walker]

(In reply to comment #16)
> (From update of attachment 115521 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115521&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:245
> > +        if (!drawOpacity)
> 
> Wouldn't it be simpler to have a check for layer->opacity() == 0 at the very top of this function and early out before we do any further calculations?

I'm not sure it ends up being simpler, but I'm open to suggestions.

You can only early out if the layer creates a render surface; if not, you still have to process all of its descendants.  And, the transform math that you want to skip sometimes determines whether or not a render surface is created: masking to bounds only does if the transform is not a scale or translation.

(Although, I wonder if that means that layers that draw content and mask to bounds will apply their opacity differently to their children.  That sounds like a bug, but maybe in WebKit no clipping layers have opacity != 1 and so we haven't ever triggered that?)

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383
> > +    if (layer->renderSurface() && !layer->renderSurface()->layerList().size()) {
> 
> Won't the RS by definition contain in its layer list at least the layer that triggered its creation?

That's not the case anymore if that layer doesn't get drawn.  There's a layerShouldBeSkipped() check before inserting any layer into a render surface list.

[Vangelis Kokkevis]

Comment on attachment 115521 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:245
>>> +        if (!drawOpacity)
>> 
>> Wouldn't it be simpler to have a check for layer->opacity() == 0 at the very top of this function and early out before we do any further calculations?
> 
> I'm not sure it ends up being simpler, but I'm open to suggestions.
> 
> You can only early out if the layer creates a render surface; if not, you still have to process all of its descendants.  And, the transform math that you want to skip sometimes determines whether or not a render surface is created: masking to bounds only does if the transform is not a scale or translation.
> 
> (Although, I wonder if that means that layers that draw content and mask to bounds will apply their opacity differently to their children.  That sounds like a bug, but maybe in WebKit no clipping layers have opacity != 1 and so we haven't ever triggered that?)

A layer's opacity is applied to the layer and all its descendants.  So if a layer has opacity 0 then there's no need to process the descendants.  At least that's the way I understand it.

A layer with opacity < 1 always creates a render surface unless it has a preserves3D property, in which case we don't want to flatten out its descendants into a render surface.  Regardless though, its opacity will affect them.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383
>>> +    if (layer->renderSurface() && !layer->renderSurface()->layerList().size()) {
>> 
>> Won't the RS by definition contain in its layer list at least the layer that triggered its creation?
> 
> That's not the case anymore if that layer doesn't get drawn.  There's a layerShouldBeSkipped() check before inserting any layer into a render surface list.

You're right!

[Adrienne Walker]

Created attachment 115614 [details]
Earlier opacity checks

[Shawn Singh]

Comment on attachment 115614 [details]
Earlier opacity checks

Unofficially looks good to me, except for one minor nit.


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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:121
>      typedef Vector<RefPtr<LayerType> > LayerList;
> +
> +    float drawOpacity = layer->opacity();
> +    if (layer->parent() && layer->parent()->preserves3D())
> +        drawOpacity *= layer->parent()->drawOpacity();
> +    // The opacity of a layer always applies to its children (either implicitly
> +    // via a render surface or explicitly if the parent preserves 3D), so the
> +    // entire subtree can be skipped if this layer is fully transparent.
> +    if (!drawOpacity)
> +        return;
> +

Would it be OK to move this to below the big comment?

[Adrienne Walker]

Created attachment 115618 [details]
Move code below comment

[James Robinson]

Comment on attachment 115618 [details]
Move code below comment

R=me

[Adrienne Walker]

Committed r100660: <http://trac.webkit.org/changeset/100660>

[Vangelis Kokkevis]

Comment on attachment 115618 [details]
Move code below comment

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:197
> +    if (!drawOpacity)

Sorry, somewhat of a delayed comment... 
You don't need to calculate the drawOpacity.  It's sufficient to check and early out if layer->opacity() == 0 . 
In this case drawOpacity == 0 only if layer->opacity() == 0 . parent->drawOpacity() will never be zero because it it were, we would have pruned the recursion at the parent and not descended to this layer.