WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(84.56 KB, patch)
2011-03-22 15:21 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
common writeIndent
(84.53 KB, patch)
2011-03-22 15:30 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(83.80 KB, patch)
2011-03-23 11:36 PDT
,
James Robinson
kbr
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2011-03-21 18:40:11 PDT
Created
attachment 86402
[details]
Patch
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
Created
attachment 86519
[details]
Patch
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
Created
attachment 86652
[details]
Patch
James Robinson
Comment 14
2011-03-23 11:37:40 PDT
Merged up. The conflicts were mainly with
http://trac.webkit.org/changeset/81740
and
http://trac.webkit.org/changeset/81737
.
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
Committed
r82006
: <
http://trac.webkit.org/changeset/82006
>
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.
Top of Page
Format For Printing
XML
Clone This Bug