RESOLVED FIXED 56793
[chromium] Move draw implementations to CCLayerImpl for everything except content layers
https://bugs.webkit.org/show_bug.cgi?id=56793
Summary [chromium] Move draw implementations to CCLayerImpl for everything except con...
James Robinson
Reported 2011-03-21 18:08:40 PDT
[chromium] Move draw implementations to CCLayerImpl for everything except content layers
Attachments
Patch (82.89 KB, patch)
2011-03-21 18:40 PDT, James Robinson
no flags
Patch (84.56 KB, patch)
2011-03-22 15:21 PDT, James Robinson
no flags
common writeIndent (84.53 KB, patch)
2011-03-22 15:30 PDT, James Robinson
no flags
Patch (83.80 KB, patch)
2011-03-23 11:36 PDT, James Robinson
kbr: review+
James Robinson
Comment 1 2011-03-21 18:40:11 PDT
James Robinson
Comment 2 2011-03-21 18:41:07 PDT
Should be pretty straightforward ;-D
Nat Duca
Comment 3 2011-03-21 20:15:03 PDT
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 4 2011-03-21 21:11:00 PDT
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 5 2011-03-22 00:39:22 PDT
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
Comment 6 2011-03-22 00:48:59 PDT
(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
Comment 7 2011-03-22 12:49:17 PDT
(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
Comment 8 2011-03-22 13:52:18 PDT
(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
Comment 9 2011-03-22 15:21:07 PDT
James Robinson
Comment 10 2011-03-22 15:24:45 PDT
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
Comment 11 2011-03-22 15:30:05 PDT
Created attachment 86521 [details] common writeIndent
James Robinson
Comment 12 2011-03-22 20:08:03 PDT
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
Comment 13 2011-03-23 11:36:37 PDT
James Robinson
Comment 14 2011-03-23 11:37:40 PDT
James Robinson
Comment 15 2011-03-24 20:02:16 PDT
ping?
Kenneth Russell
Comment 16 2011-03-24 21:24:12 PDT
If Vangelis can review this I'll be happy to r+ it.
Vangelis Kokkevis
Comment 17 2011-03-24 23:10:03 PDT
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 18 2011-03-24 23:24:14 PDT
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 19 2011-03-25 10:51:49 PDT
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
Comment 20 2011-03-25 17:10:25 PDT
WebKit Review Bot
Comment 21 2011-03-25 18:13:15 PDT
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
Note You need to log in before you can comment on or make changes to this bug.