Bug 56793 – [chromium] Move draw implementations to CCLayerImpl for everything except content layers

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Move draw implementations to CCLayerImpl for everything except content layers

[James Robinson]

Created attachment 86402 [details]
Patch

[James Robinson]

Should be pretty straightforward ;-D

[Nat Duca]

Comment on attachment 86402 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        and moving the code from each XXXLayerChromium to XXXCCLayerImpl.  In order to render from the CCLayerImpl side all state

XXXCCLayerImpl --> CCXXXLayerImpl? CC feels like a prefix... what does CA do? Is it OpenGLCALayer?

> Source/WebCore/ChangeLog:29
> +        5.) for each LayerChromium in tree order, update the layer's textures

Is this the step that requires a access to the compositor context?

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:292
> +void LayerChromium::updateToLayer(CCLayerImpl* layer)

should we be calling this commitToLayer?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:405
> +        m_rootLayer->createCCLayerImplIfNeeded();

Why do we create the impl right up front on the root layer? Sorry if I'm being daft...

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:471
>  {

Wonders how we can clean up this renderSurfacLayerList/layerList/worldTransform/renderLayer mess... not for this CL, but in the big picture. Maybe we should whiteboard this tomorrow? I'm wondering if we should refactor [in another cl] this monster function into a few functions with smaller and better-contained side effects, even at the penalty of walking the tree a few times.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:485
> +    // RenderSurfaces and we rely on RenderSurfaces being up to date in order to paint contents we have

Is the state dependency here the world transforms? Or is there more?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:761
> +void LayerRendererChromium::updateContentsTextureRecursive(LayerChromium* layer)

Can we call this updateCompositorResourcesRecursive?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:781
> +    layer->updateToLayer(drawLayer);

The first thing I'm going to do in the threading cl is make this yet-another tree walk, e.g. updateToLayerRecursive(...) (commitToLayerRecursive)... can we just make it that way from the start?

Also, when do the replica layers get updatedToLayer? I don't know much about how masks and replicas work in our system so...

[James Robinson]

Comment on attachment 86402 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        and moving the code from each XXXLayerChromium to XXXCCLayerImpl.  In order to render from the CCLayerImpl side all state
> 
> XXXCCLayerImpl --> CCXXXLayerImpl? CC feels like a prefix... what does CA do? Is it OpenGLCALayer?

They do CAOpenGLLayer (see WebGLLayer.h).  I'm not super in love with either naming scheme but CCXXXLayerImpl would be a little more consistent.

>> Source/WebCore/ChangeLog:29
>> +        5.) for each LayerChromium in tree order, update the layer's textures
> 
> Is this the step that requires a access to the compositor context?

yes

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:292
>> +void LayerChromium::updateToLayer(CCLayerImpl* layer)
> 
> should we be calling this commitToLayer?

I think this will be part of the commit step, but I'm not sure about commit as a verb in this context. updateProperties..() feels more natural than commitProperties..() even though we've been referring to this step as the commit phase in design discussions.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:405
>> +        m_rootLayer->createCCLayerImplIfNeeded();
> 
> Why do we create the impl right up front on the root layer? Sorry if I'm being daft...

this might not be strictly necessary now that you point it out.  setLayerRenderer() which is called on the next line currently defers to the ccLayerImpl in order to clean resources up, so if the ccLayerImpl has to exist - or maybe i can just null check in setLayerRenderer()

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:471

> 
> Wonders how we can clean up this renderSurfacLayerList/layerList/worldTransform/renderLayer mess... not for this CL, but in the big picture. Maybe we should whiteboard this tomorrow? I'm wondering if we should refactor [in another cl] this monster function into a few functions with smaller and better-contained side effects, even at the penalty of walking the tree a few times.

yeah let's try to figure this out.  the content layer update code is pretty convoluted right now to try to determine a visible region and it has issue anyway so we need to clean it up regardless of this patch.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:485
>> +    // RenderSurfaces and we rely on RenderSurfaces being up to date in order to paint contents we have
> 
> Is the state dependency here the world transforms? Or is there more?

world transforms and the render surface bounds (in order to test visibility).

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:761
>> +void LayerRendererChromium::updateContentsTextureRecursive(LayerChromium* layer)
> 
> Can we call this updateCompositorResourcesRecursive?

whoops! i renamed updateContentsTexture()->updateCompositorResources() but forgot to rename this one. will do!

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:781
>> +    layer->updateToLayer(drawLayer);
> 
> The first thing I'm going to do in the threading cl is make this yet-another tree walk, e.g. updateToLayerRecursive(...) (commitToLayerRecursive)... can we just make it that way from the start?
> 
> Also, when do the replica layers get updatedToLayer? I don't know much about how masks and replicas work in our system so...

as i referred to in the changelog i want to fold all tree walks together.  i think we can get away with folding what you want into this walk for now - let's chat about it tomorrow.

LayerChromium::updateToLayer() calls updateToLayer() on its replica, mask, and replica mask layers.  i'm not sure how we will represent these relationships exactly in the mirrored CCLayerImpl tree once that exists but once we figure that out it will hopefully be obvious where to put the update call for it

[Vangelis Kokkevis]

Comment on attachment 86402 [details]
Patch

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

Looks good on a first look.  Will take another look tomorrow.

>>> Source/WebCore/ChangeLog:12
>>> +        and moving the code from each XXXLayerChromium to XXXCCLayerImpl.  In order to render from the CCLayerImpl side all state
>> 
>> XXXCCLayerImpl --> CCXXXLayerImpl? CC feels like a prefix... what does CA do? Is it OpenGLCALayer?
> 
> They do CAOpenGLLayer (see WebGLLayer.h).  I'm not super in love with either naming scheme but CCXXXLayerImpl would be a little more consistent.

CCXXXLayerImp sounds better to me too.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:391
> +{

Why not always create a ccLayerImpl every in the constructor of LayerChromium?

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:471

>> 
>> Wonders how we can clean up this renderSurfacLayerList/layerList/worldTransform/renderLayer mess... not for this CL, but in the big picture. Maybe we should whiteboard this tomorrow? I'm wondering if we should refactor [in another cl] this monster function into a few functions with smaller and better-contained side effects, even at the penalty of walking the tree a few times.
> 
> 

I agree this function in getting out of control.  I'll help refactoring it. Different CL though.

> Source/WebCore/platform/graphics/chromium/cc/PluginCCLayerImpl.cpp:74
> +static void writeIndent(TextStream& ts, int indent)

The writeIndent method is repeated in all layer type files.  It should either be folded in o the base class or inlined in some common header file.

[James Robinson]

(In reply to comment #5)
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:391
> > +{
> 
> Why not always create a ccLayerImpl every in the constructor of LayerChromium?

I don't know the correct runtime (dynamic) type in the constructor of a LayerChromium and I can't find a good way to get it since virtuals don't work in constructors.  When we have code to explicitly sync the trees we'll be constructing the ccLayerImpls lazily so it seemed OK to figure out where I need the null checks now :)

[Adrienne Walker]

(In reply to comment #6)
> (In reply to comment #5)
> > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:391
> > > +{
> > 
> > Why not always create a ccLayerImpl every in the constructor of LayerChromium?
> 
> I don't know the correct runtime (dynamic) type in the constructor of a LayerChromium and I can't find a good way to get it since virtuals don't work in constructors.  When we have code to explicitly sync the trees we'll be constructing the ccLayerImpls lazily so it seemed OK to figure out where I need the null checks now :)

Are we really planning on constructing the ccLayerImpls lazily and implicitly? I was under the impression that we were going to sync the tree structure explicitly with "add this child layer" and "remove this layer subtree" changes.

(In reply to comment #4)
> (From update of attachment 86402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86402&action=review
>
> >> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:292
> >> +void LayerChromium::updateToLayer(CCLayerImpl* layer)
> > 
> > should we be calling this commitToLayer?
> 
> I think this will be part of the commit step, but I'm not sure about commit as a verb in this context. updateProperties..() feels more natural than commitProperties..() even though we've been referring to this step as the commit phase in design discussions.

I'm not sure about the "to" as a preposition in this context.  It just sounds awkward.  Maybe just updateLayer? Or alternatively pushToLayerImpl? </bikeshedding>

> > Wonders how we can clean up this renderSurfacLayerList/layerList/worldTransform/renderLayer mess... not for this CL, but in the big picture. Maybe we should whiteboard this tomorrow? I'm wondering if we should refactor [in another cl] this monster function into a few functions with smaller and better-contained side effects, even at the penalty of walking the tree a few times.
> 
> yeah let's try to figure this out.  the content layer update code is pretty convoluted right now to try to determine a visible region and it has issue anyway so we need to clean it up regardless of this patch.

I'm pretty sure we can reasonably abstract this in a way that doesn't require multiple tree walks.  That might be premature optimization, but it seems silly to be slower if we don't have to.

> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:485
> >> +    // RenderSurfaces and we rely on RenderSurfaces being up to date in order to paint contents we have
> > 
> > Is the state dependency here the world transforms? Or is there more?
> 
> world transforms and the render surface bounds (in order to test visibility).

Alpha value, for animated transparency on render surfaces?

[James Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:391
> > > > +{
> > > 
> > > Why not always create a ccLayerImpl every in the constructor of LayerChromium?
> > 
> > I don't know the correct runtime (dynamic) type in the constructor of a LayerChromium and I can't find a good way to get it since virtuals don't work in constructors.  When we have code to explicitly sync the trees we'll be constructing the ccLayerImpls lazily so it seemed OK to figure out where I need the null checks now :)
> 
> Are we really planning on constructing the ccLayerImpls lazily and implicitly? I was under the impression that we were going to sync the tree structure explicitly with "add this child layer" and "remove this layer subtree" changes.

It'll be explicit but as part of the commit phase and won't necessarily happen at a specific point relative to main thread operations (commit scheduling is up to the compositor side).

> 
> (In reply to comment #4)
> > (From update of attachment 86402 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=86402&action=review
> >
> > >> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:292
> > >> +void LayerChromium::updateToLayer(CCLayerImpl* layer)
> > > 
> > > should we be calling this commitToLayer?
> > 
> > I think this will be part of the commit step, but I'm not sure about commit as a verb in this context. updateProperties..() feels more natural than commitProperties..() even though we've been referring to this step as the commit phase in design discussions.
> 
> I'm not sure about the "to" as a preposition in this context.  It just sounds awkward.  Maybe just updateLayer? Or alternatively pushToLayerImpl? </bikeshedding>
>

the callsites look like:
<instance of LayerChromium> -> updateToLayer ( <instance of CCLayerImpl> )

updateLayer doesn't indicate which direction the operation is moving data to me.  pushToLayer would also work but I think the 'Impl' is a bit redundant with the type of the parameter.  the only thing i know about prepositions is that they are fun to end sentences with.


> > >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:485
> > >> +    // RenderSurfaces and we rely on RenderSurfaces being up to date in order to paint contents we have
> > > 
> > > Is the state dependency here the world transforms? Or is there more?
> > 
> > world transforms and the render surface bounds (in order to test visibility).
> 
> Alpha value, for animated transparency on render surfaces?

We don't use the alpha value when painting for content layers or the root layer currently.  The bad state dependency is because of uses of render surface stuff in the actual paintContents() call.  We'll always calculate render surfaces after doing the commit.

[James Robinson]

Created attachment 86519 [details]
Patch

[James Robinson]

Changes from patch 1:

*) s/XxxCCLayerImpl/CCXxxLayerImpl/g
*) avoids creating the root layer's CCLayerImpl eagerly. doing this meant cleaning up the setLayerRenderer path a bit
*) in LayerRendererChromium s/updateContentsTextureRecursive/updateCompositorResourcesRecursive/ to match the LayerChromium call


I didn't rename updateToLayer because I'm not sure what to rename it to.

I didn't refactor writeIndent because I forgot...I'll do that now and upload a new patch.

[James Robinson]

Created attachment 86521 [details]
common writeIndent

[James Robinson]

Sadly at this point this patch conflicts with a few other recent patches to other layer types.  I'll try to merge it up and repost and can hopefully land it before too many other invasive patches to *LayerChromiums land.

[James Robinson]

Created attachment 86652 [details]
Patch

[James Robinson]

Merged up.  The conflicts were mainly with http://trac.webkit.org/changeset/81740 and http://trac.webkit.org/changeset/81737.

[James Robinson]

ping?

[Kenneth Russell]

If Vangelis can review this I'll be happy to r+ it.

[Vangelis Kokkevis]

Comment on attachment 86652 [details]
Patch

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

Generally looks good to me other than a couple of small comments.

> Source/WebCore/ChangeLog:12
> +        and moving the code from each XXXLayerChromium to XXXCCLayerImpl.  In order to render from the CCLayerImpl side all state

XXXCCLayerImpl -> CCXXXLayerImpl

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:60
> +void CanvasLayerChromium::updateToLayer(CCLayerImpl* layer)

Not a huge fan of the updateToLayer name.  syncCompositorLayer() or something like that maybe?

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:99
> +        m_ccLayerImpl->cleanupResources();

Should we also call cleanupResources for the associated mask and replica layers?

[James Robinson]

Comment on attachment 86652 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        and moving the code from each XXXLayerChromium to XXXCCLayerImpl.  In order to render from the CCLayerImpl side all state
> 
> XXXCCLayerImpl -> CCXXXLayerImpl

Good catch. I'll fix before landing.

>> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:60
>> +void CanvasLayerChromium::updateToLayer(CCLayerImpl* layer)
> 
> Not a huge fan of the updateToLayer name.  syncCompositorLayer() or something like that maybe?

I really don't want to go another round of renames on this unless something is clearly better. syncCompositorLayer() doesn't give much information about what is going on since the word "sync" does not indicate which direction data is moving in and "compositor" and "layer" are implied (this is compositor code, and the function exists on and takes a layer parameter.

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:99
>> +        m_ccLayerImpl->cleanupResources();
> 
> Should we also call cleanupResources for the associated mask and replica layers?

No need - we call setLayerRenderer() on the mask and replica layers explicitly which triggers a cleanupResources() call.

[Kenneth Russell]

Comment on attachment 86652 [details]
Patch

It sounds to me like this is ready to go modulo small fixes to be done upon landing. r=me

[James Robinson]

Committed r82006: <http://trac.webkit.org/changeset/82006>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82006 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
plugins/embed-prefers-plugins-for-images.html