Bug 71038 - [chromium] Implicitly skip render surfaces that won't be drawn
Summary: [chromium] Implicitly skip render surfaces that won't be drawn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on: 71150
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-27 11:20 PDT by Adrienne Walker
Modified: 2011-11-22 18:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.41 KB, patch)
2011-10-27 11:23 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Remove unnecessary skipsDraw (8.63 KB, patch)
2011-10-27 11:59 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (7.90 KB, patch)
2011-10-27 17:53 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2011-11-16 21:50 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Earlier opacity checks (10.94 KB, patch)
2011-11-17 10:15 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Move code below comment (11.28 KB, patch)
2011-11-17 10:36 PST, Adrienne Walker
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-10-27 11:20:42 PDT
[chromium] Implicitly skip render surfaces that won't be drawn
Comment 1 Adrienne Walker 2011-10-27 11:23:07 PDT
Created attachment 112714 [details]
Patch
Comment 2 Adrienne Walker 2011-10-27 11:24:12 PDT
Just a quick low-priority cleanup patch.
Comment 3 Shawn Singh 2011-10-27 11:50:13 PDT
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.
Comment 4 Adrienne Walker 2011-10-27 11:59:33 PDT
Created attachment 112719 [details]
Remove unnecessary skipsDraw
Comment 5 James Robinson 2011-10-27 12:37:21 PDT
Comment on attachment 112719 [details]
Remove unnecessary skipsDraw

R=me
Comment 6 Vangelis Kokkevis 2011-10-27 13:09:23 PDT
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.
Comment 7 Shawn Singh 2011-10-27 13:46:42 PDT
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.
Comment 8 Adrienne Walker 2011-10-27 17:53:04 PDT
Created attachment 112793 [details]
Patch
Comment 9 Adrienne Walker 2011-10-27 17:56:00 PDT
(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.  :)
Comment 10 WebKit Review Bot 2011-10-28 13:45:15 PDT
Comment on attachment 112793 [details]
Patch

Clearing flags on attachment: 112793

Committed r98757: <http://trac.webkit.org/changeset/98757>
Comment 11 WebKit Review Bot 2011-10-28 13:45:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Shawn Singh 2011-10-28 18:03:00 PDT
(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?
Comment 13 Adrienne Walker 2011-10-28 18:07:03 PDT
I saw it.  Just hadn't reopened this yet.
Comment 14 Adrienne Walker 2011-11-16 21:50:25 PST
Created attachment 115521 [details]
Patch
Comment 15 Adrienne Walker 2011-11-16 21:52:06 PST
(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.
Comment 16 Vangelis Kokkevis 2011-11-16 23:27:47 PST
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?
Comment 17 Adrienne Walker 2011-11-17 09:20:47 PST
(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.
Comment 18 Vangelis Kokkevis 2011-11-17 09:51:55 PST
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!
Comment 19 Adrienne Walker 2011-11-17 10:15:26 PST
Created attachment 115614 [details]
Earlier opacity checks
Comment 20 Shawn Singh 2011-11-17 10:33:09 PST
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?
Comment 21 Adrienne Walker 2011-11-17 10:36:21 PST
Created attachment 115618 [details]
Move code below comment
Comment 22 James Robinson 2011-11-17 12:51:35 PST
Comment on attachment 115618 [details]
Move code below comment

R=me
Comment 23 Adrienne Walker 2011-11-17 13:01:24 PST
Committed r100660: <http://trac.webkit.org/changeset/100660>
Comment 24 Vangelis Kokkevis 2011-11-22 18:29:07 PST
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.