Bug 58840 – [chromium] Decouple layer visibility calculations from render surfaces / computed draw transforms

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Currently in ContentLayerChromium we determine the visible portion of the layer using the render surface scissor rect and the computed draw transform for the layer.  In the multithreaded compositor world, however, we'll be generating render surfaces and the final draw transform in the compositor thread since they might change from frame to frame (due to animation or whatnot).

We need to figure out some way to calculate visibility on the main thread, preferably without having to duplicate all of the draw transform + render surface bound calculations.

[James Robinson]

I'm taking a look into this now.  My rough plan is as follows:

- land https://bugs.webkit.org/show_bug.cgi?id=58830 so the property pushing aspect of LRC::updatePropertiesAndRenderSurfaces() is separate from the transform portion

- make LayerRendererChromium::updatePropertiesAndRenderSurfaces() a generic function to calculate the draw transforms, visible regions, and z-order for a set of layers templated for LayerChromium+??? and CCLayerImpl+RenderSurfaceChromium so we can use the same codepath for LayerChromiums and CCLayerImpl.  This will also involve abstracting the "holds on to a z-order list of layers, transform and bounds" portion of RenderSurfaceChromium out from the "is usable as a render target and can draw itself" of RenderSurfaceChromium.

We'll run the algorithm to calculate draw transforms, visible regions, and z-order in the main thread portion using the LayerChromium tree and aggregate results into some class name TBD.  We'll then run the same code on the CCLayerImpl tree in the compositor thread and aggregate into real RenderSurfaceChromiums.

[Adrienne Walker]

(In reply to comment #1)

> - make LayerRendererChromium::updatePropertiesAndRenderSurfaces() a generic function to calculate the draw transforms, visible regions, and z-order for a set of layers templated for LayerChromium+??? and CCLayerImpl+RenderSurfaceChromium so we can use the same codepath for LayerChromiums and CCLayerImpl.  This will also involve abstracting the "holds on to a z-order list of layers, transform and bounds" portion of RenderSurfaceChromium out from the "is usable as a render target and can draw itself" of RenderSurfaceChromium.

I was looking at some of this same code yesterday to try to better fix https://bugs.webkit.org/show_bug.cgi?id=59020.  The scissor rect / visible region calculation is wrong in one particular case, but I think there also needs to be some refactoring about where we calculate and store that scissor rect.  However, this sounds like exactly the same patch of code that you're about to uproot.

Would it make sense for you to try to address both at the same time? Otherwise, maybe I should postpone that fix until you're done reorganizing this?

[James Robinson]

Created attachment 102323 [details]
Patch

[James Robinson]

This patch might be a touch large to review (hah!).  I can think of at least one preliminary patch that I can land ahead of time to make the diff easier to read, which I'll file as a dependent bug on this one, but for the most part these changes have to land atomically, I'm afraid.

[WebKit Review Bot]

Comment on attachment 102323 [details]
Patch

Attachment 102323 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9269278

[WebKit Review Bot]

Comment on attachment 102323 [details]
Patch

Attachment 102323 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9266366

[James Robinson]

Created attachment 102372 [details]
compile fixes

[James Robinson]

Created attachment 102417 [details]
Patch

[James Robinson]

With http://trac.webkit.org/changeset/92037 landed this patch is still large, but a bit more manageable.

[Vangelis Kokkevis]

Comment on attachment 102417 [details]
Patch

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

Looks good!  I suppose additional work will need to be done to deal with the texture manager used by CCRenderSurface?

> Source/WebCore/ChangeLog:408
> [phone]  James Robinson  <[e-mail]>

Needs to be removed

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:410
> +bool LayerChromium::descendantsDrawsContent()

nit: descendantDrawsContent() ?

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:-42
> -#include "RenderSurfaceChromium.h"

Why change the order here?  Headers were alphabetically sorted.  If the need to be rearranged then there are hidden dependencies that should be eliminated.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:30
> +#include "CCRenderSurface.h"

cc/CCRenderSurface.h  ?

[James Robinson]

(In reply to comment #10)
> (From update of attachment 102417 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102417&action=review
> 
> Looks good!  I suppose additional work will need to be done to deal with the texture manager used by CCRenderSurface?

Yes, there's some more work needed here - https://bugs.webkit.org/show_bug.cgi?id=64772 is the next piece of that puzzle.

> 
> > Source/WebCore/ChangeLog:408
> > [phone]  James Robinson  <[e-mail]>
> 
> Needs to be removed

fixed

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:410
> > +bool LayerChromium::descendantsDrawsContent()
> 
> nit: descendantDrawsContent() ?

good idea, fixed

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:-42
> > -#include "RenderSurfaceChromium.h"
> 
> Why change the order here?  Headers were alphabetically sorted.  If the need to be rearranged then there are hidden dependencies that should be eliminated.

resorted

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> 2011

done
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:30
> > +#include "CCRenderSurface.h"
> 
> cc/CCRenderSurface.h  ?

done

[James Robinson]

Created attachment 102579 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 102579 [details]
Patch

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

Couple more small things but otherwise and unofficial LGTM from me.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:66
> +    , m_debugBorderWidth(0)

I don't see m_debugBorderWidth used here.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

[James Robinson]

Created attachment 102592 [details]
fix copyright year

[James Robinson]

(In reply to comment #13)
> (From update of attachment 102579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102579&action=review
> 
> Couple more small things but otherwise and unofficial LGTM from me.
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:66
> > +    , m_debugBorderWidth(0)
> 
> I don't see m_debugBorderWidth used here.
> 

It's used in pushPropertiesTo() and later used on the CC side of life when drawing layers.

[Kenneth Russell]

Comment on attachment 102592 [details]
fix copyright year

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

Overall this looks like good progress. A couple of minor comments. r=me

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:245
>      OwnPtr<CCCanvasLayerImpl::Program> m_canvasLayerProgram;
> +    OwnPtr<CCPluginLayerImpl::Program> m_pluginLayerProgram;
> +    OwnPtr<CCRenderSurface::MaskProgram> m_renderSurfaceMaskProgram;
> +    OwnPtr<CCRenderSurface::Program> m_renderSurfaceProgram;
>      OwnPtr<CCVideoLayerImpl::RGBAProgram> m_videoLayerRGBAProgram;
>      OwnPtr<CCVideoLayerImpl::YUVProgram> m_videoLayerYUVProgram;

It seems to me that these references are abstraction violations. The LayerRendererChromium shouldn't reference these programs directly. Is there a plan to move these entirely into the cc/ package? If so, is it worth a FIXME?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:107
> +    OwnPtr<LayerTexture> m_contentsTexture;

Is this reference an abstraction violation? It seems to me that the compositor's classes should use those in cc/ but not the other way around.

[James Robinson]

You are correct on both points.  Nat is working on splitting up LayerRendererChromium into (at least) two halves, part of that will involve moving the program management into a cc/ class.  For now, though, there isn't a good cc/ class to hold the programs on.

The LayerTexture dependency is also a bit wonky because we are using the TextureManager concept for both content textures (which aren't currently a cc/ concept) and for render surfaces (which are).  It's going to take a bit more work over the next few weeks to fully clean this up.

[WebKit Review Bot]

Comment on attachment 102592 [details]
fix copyright year

Clearing flags on attachment: 102592

Committed r92245: <http://trac.webkit.org/changeset/92245>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.