Bug 54047 - [chromium] Separate the "update" and "draw" portions of LayerRendererChromium's drawLayers function
Summary: [chromium] Separate the "update" and "draw" portions of LayerRendererChromium...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 54315 55013
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-08 17:02 PST by James Robinson
Modified: 2011-03-07 12:22 PST (History)
7 users (show)

See Also:


Attachments
WIP (55.84 KB, patch)
2011-02-08 17:03 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (75.46 KB, patch)
2011-02-10 18:47 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (82.03 KB, patch)
2011-02-10 21:49 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (79.73 KB, patch)
2011-02-22 11:33 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2011-02-25 13:30 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (27.81 KB, patch)
2011-03-02 15:08 PST, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-02-08 17:02:19 PST
[chromium] Separate the "update" and "draw" portions of LayerRendererChromium's drawLayers function
Comment 1 James Robinson 2011-02-08 17:03:08 PST
Created attachment 81718 [details]
WIP
Comment 2 James Robinson 2011-02-08 17:05:32 PST
This is still very rough, but it shows where I'm heading.  The goal here is to move all of the state that is only used for drawing over to the CompCCLayer (aka CCLayerPres or whatever we call it), which is used for actually compositing and will eventually live in the compositor thread; and to split up LayerRendererChromium's drawLayers() call into two pieces one that updates each layer (potentially calling into the rest of WebKit to paint) and the second that actually issues GL calls to composite the scene.  At this point the state split is not very clean at all but the codepaths are starting to get there.
Comment 3 Adrienne Walker 2011-02-08 18:04:47 PST
Comment on attachment 81718 [details]
WIP

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

> Source/WebCore/ChangeLog:55
> +        * platform/graphics/chromium/cc/CompCCLayer.cpp: Added.

What does CompCCLayer stand for? I parse that as "Compositor Chromium Compositor Layer".

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:259
> +    // ------======== divide. below here is GL stuff, above here is WebKit stuff =======------

Well, except for that pesky scrollbar paint later. but that's going to have to happen on the compositor thread anyway.
Comment 4 Nat Duca 2011-02-08 18:23:07 PST
Comment on attachment 81718 [details]
WIP

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

> Source/WebCore/WebCore.gypi:2634
> +            'platform/graphics/chromium/cc/CompCCLayer.cpp',

Possible better names:
1.  CCLayerRenderer // eg ContentLayerRenderer, WebGLLayerRenderer
2.  CCLayerBackend  // eg ContentLayerBackend, WebGLLayerBackend
My vote is for 1.
Comment 5 James Robinson 2011-02-08 18:37:45 PST
(In reply to comment #3)
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:259
> > +    // ------======== divide. below here is GL stuff, above here is WebKit stuff =======------
> 
> Well, except for that pesky scrollbar paint later. but that's going to have to happen on the compositor thread anyway.

The comment is more aspirational than descriptive currently :)
Comment 6 Vangelis Kokkevis 2011-02-09 16:55:22 PST
Comment on attachment 81718 [details]
WIP

Looks good.  Thanks for cleaning up the accessors!  The RenderSurfaceChromium is also mostly applicable to the compositor so it should probably get a CC treatment as well (although it could be done in a separate step).
Comment 7 James Robinson 2011-02-10 18:47:27 PST
Created attachment 82086 [details]
Patch
Comment 8 James Robinson 2011-02-10 18:54:16 PST
The most recent patch feels a lot cleaner to me.  I've moved most of the drawing state over to exist only on the compositor's layers (which is called CCLayerImpl now) and made a cleaner split between the 'update' phase and the 'composite' phase.  LayerRendererChromium::updateLayersRecursive() now spends a decent bit amount of effort copying state from LayerChromiums to CCLayerImpls.

I'd like to move the code that calls updateContentsIfDirty() on each LayerChromium to happen before the draw state is updated, but unfortunately the large content layer support currently relies on being able to see the up-to-date draw transform and other state.  I think enne's tiling patch will fix this dependency (by eliminating the current large layer concept).  Once that is done I should be able to decouple the update and composite steps more thoroughly.

I'm not sure whether the mask and replica layers belong to the CCLayerImpl side of the world or should stay on the LayerChromium side of the world - it kind of feels like they are draw-time only concepts and should move, but I haven't looked in detail.  I also think that RenderSurfaceChromium should move over to the CCLayerImpl side of the world somehow but haven't started on it yet.
Comment 9 James Robinson 2011-02-10 18:55:47 PST
I'm also still relying on the LayerChromium tree for structure - CCLayerImpl has a superlayer() accessor that goes through LayerChromium but there's no way to iterate through the children of a CCLayerImpl yet.
Comment 10 James Robinson 2011-02-10 21:49:38 PST
Created attachment 82101 [details]
Patch
Comment 11 Nat Duca 2011-02-11 14:20:55 PST
Comment on attachment 82101 [details]
Patch

Summarizing a verbal conversation, iirw, the flow for a new frame is:
- LayerChromium::PaintContents (as in, WebKit paint) (on main thread)
- LayerChromium::Commit (called on lay
- CCLayerImpl::UpdateTransforms()
- CCLayerImpl::Draw()
Comment 12 James Robinson 2011-02-11 15:00:56 PST
(In reply to comment #11)
> (From update of attachment 82101 [details])
> Summarizing a verbal conversation, iirw, the flow for a new frame is:
> - LayerChromium::PaintContents (as in, WebKit paint) (on main thread)
> - LayerChromium::Commit (called on lay
> - CCLayerImpl::UpdateTransforms()
> - CCLayerImpl::Draw()

Sounds right.

I'm going to start landing bits of this as separate patches to keep reviewers sane, then flip review? once this patch is small enough to digest in reasonable isolation.
Comment 13 James Robinson 2011-02-22 11:33:43 PST
Created attachment 83352 [details]
Patch
Comment 14 James Robinson 2011-02-22 11:35:01 PST
Updated to show where I'm going.  I'll break bits of this off to smaller patches to review+land.
Comment 15 Nat Duca 2011-02-24 12:35:45 PST
Me likes. Thanks for landing this James.
Comment 16 James Robinson 2011-02-25 13:30:43 PST
Created attachment 83867 [details]
Patch
Comment 17 Vangelis Kokkevis 2011-03-01 13:02:00 PST
Comment on attachment 83867 [details]
Patch

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

Looks very good.  Just a couple of smaller comments.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:278
> +    // Unfortunately, updateLayersRecursive() currently both updates the layers and updates the draw state

Is this comment stale now? 

"updateLayersRecursive()" -> "updateDrawStateAndRenderSurfaces()" ? 

Also I'm not sure what you mean by "updates the layers"  ?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:287
> +void LayerRendererChromium::drawLayers(const Vector<CCLayerImpl*>& renderSurfaceLayerList)

These functions are now getting long and complex.  I think they would benefit from a brief comment blurb to describe what each of them does.

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

Should this be called something like:  "updateLayerContentsRecursive" to avoid confusion between "painting" and rendering?

> Source/WebKit/chromium/ChangeLog:5
> +        [chromium] Separate the &quot;update&quot; and &quot;draw&quot; portions of LayerRendererChromium&apos;s drawLayers function

&quot; -> ",  &apos; -> '
Comment 18 James Robinson 2011-03-01 14:58:39 PST
(In reply to comment #17)
> (From update of attachment 83867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83867&action=review
> "updateLayersRecursive()" -> "updateDrawStateAndRenderSurfaces()" ? 
> 
> Also I'm not sure what you mean by "updates the layers"  ?
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:695
> > +void LayerRendererChromium::paintLayersRecursive(LayerChromium* layer)
> 
> Should this be called something like:  "updateLayerContentsRecursive" to avoid confusion between "painting" and rendering?

I'm trying to make a stronger distinction between a few different ops:

"paint" - update the contents of each layer into the appropriate intermediate form for later uploading.  For content layers, this means doing a paint() into a transfer buffer of some form, but it could conceivably mean something like build a display list.  For texture-backed layers it means do the swapBuffers() equivalent for that type.  Always runs on the main thread.

"update" - synchronize the properties (transforms, opacity, etc) of each layer to their compositor implementation layer equivalent (CCLayerImpl, for now at least).  Has to be synchronized carefully :).

"draw" - compositor-side operation to actually issue the GraphicsContext3D calls in order to composite the frame.  Could also be called "composite" but that term is probably too confusing in this context.

Does that make sense?  It's a little confusing now because all three operations are taking place in the same file within a few lines of each other, but that will be less true as the code is divided up a bit more.  The ideal progression would be paint, update, then draw but currently I have to do update, paint, draw because part of the state that the "update" code currently updates RenderSurfaceChromiums (which is valid, IMO) and the "paint" operation for content layers depends on the RenderSurfaces and draw transforms (which seems wrong IMO).

I can try adding some more comments but I think the confusion has more to do with code structure than naming.  With a clearer structure the vocabulary wouldn't matter so much since the control flow would be easier to follow.
Comment 19 Vangelis Kokkevis 2011-03-01 17:56:55 PST
Comment on attachment 83867 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:695
>>> +void LayerRendererChromium::paintLayersRecursive(LayerChromium* layer)
>> 
>> Should this be called something like:  "updateLayerContentsRecursive" to avoid confusion between "painting" and rendering?
> 
> I'm trying to make a stronger distinction between a few different ops:
> 
> "paint" - update the contents of each layer into the appropriate intermediate form for later uploading.  For content layers, this means doing a paint() into a transfer buffer of some form, but it could conceivably mean something like build a display list.  For texture-backed layers it means do the swapBuffers() equivalent for that type.  Always runs on the main thread.
> 
> "update" - synchronize the properties (transforms, opacity, etc) of each layer to their compositor implementation layer equivalent (CCLayerImpl, for now at least).  Has to be synchronized carefully :).
> 
> "draw" - compositor-side operation to actually issue the GraphicsContext3D calls in order to composite the frame.  Could also be called "composite" but that term is probably too confusing in this context.
> 
> Does that make sense?  It's a little confusing now because all three operations are taking place in the same file within a few lines of each other, but that will be less true as the code is divided up a bit more.  The ideal progression would be paint, update, then draw but currently I have to do update, paint, draw because part of the state that the "update" code currently updates RenderSurfaceChromiums (which is valid, IMO) and the "paint" operation for content layers depends on the RenderSurfaces and draw transforms (which seems wrong IMO).
> 
> I can try adding some more comments but I think the confusion has more to do with code structure than naming.  With a clearer structure the vocabulary wouldn't matter so much since the control flow would be easier to follow.

That's a fine distinction however it's a bit inconsistent with how the relevant layer methods are called.  Notice that this "paint" method only calls "updateContents" methods on the layers. 

On that topic, I also find that "updateDrawState" is a bit confusing as it contains both the words "update" and "draw".

How about:
paint -> updateContents
update -> updateProperties
draw stays draw
Comment 20 James Robinson 2011-03-02 15:08:26 PST
Created attachment 84474 [details]
Patch
Comment 21 James Robinson 2011-03-02 15:11:26 PST
(In reply to comment #19)
> That's a fine distinction however it's a bit inconsistent with how the relevant layer methods are called.  Notice that this "paint" method only calls "updateContents" methods on the layers. 
> 
> On that topic, I also find that "updateDrawState" is a bit confusing as it contains both the words "update" and "draw".
> 
> How about:
> paint -> updateContents
> update -> updateProperties
> draw stays draw

Good feedback as always :).  I've updated the patch so the flow is now:

updateRootLayerContents()
updateRootLayerScrollbars() (note: for now, scrollbars are treated as a special type of contents)
update root RenderSurface
updatePropertiesAndRenderSurfaces()
updateContentsRecursive()
drawLayers()


The ideal flow would be closer to:

(main thread)
updateRootLayerContents()
updateContentsRecursive()

(with synchronization)
updateProperties()

(compositor side only)
updateRenderSurfaces()
drawLayers()


Untangling the property updates and RenderSurface updates and fixing the ordering will be my next task.
Comment 22 Vangelis Kokkevis 2011-03-04 12:06:19 PST
Comment on attachment 84474 [details]
Patch

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

Looks good!

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:436
> +void LayerRendererChromium::updatePropertiesAndRenderSurfaces(LayerChromium* layer, const TransformationMatrix& parentMatrix, Vector<CCLayerImpl*>& renderSurfaceLayerList, Vector<CCLayerImpl*>& layerList)

nit: Maybe rename to: updateLayerAndRenderSurfaceProperties since we don't actually update rendersurface contents here?
Comment 23 James Robinson 2011-03-04 13:40:24 PST
(In reply to comment #22)
> (From update of attachment 84474 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84474&action=review
> 
> Looks good!
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:436
> > +void LayerRendererChromium::updatePropertiesAndRenderSurfaces(LayerChromium* layer, const TransformationMatrix& parentMatrix, Vector<CCLayerImpl*>& renderSurfaceLayerList, Vector<CCLayerImpl*>& layerList)
> 
> nit: Maybe rename to: updateLayerAndRenderSurfaceProperties since we don't actually update rendersurface contents here?

I think this rename will be a little unnecessary since updatePropertiesAndRenderSurfaces() is going to get gutted in a patch in the very near future.
Comment 24 James Robinson 2011-03-07 09:44:18 PST
ping?
Comment 25 Kenneth Russell 2011-03-07 09:58:23 PST
Comment on attachment 84474 [details]
Patch

Only scanned the review; rs=me.
Comment 26 James Robinson 2011-03-07 11:37:48 PST
Committed r80482: <http://trac.webkit.org/changeset/80482>
Comment 27 WebKit Review Bot 2011-03-07 12:22:13 PST
http://trac.webkit.org/changeset/80482 might have broken Qt Linux Release
The following tests are not passing:
fast/viewport/viewport-100.html
fast/viewport/viewport-101.html
fast/viewport/viewport-102.html
fast/viewport/viewport-103.html
fast/viewport/viewport-104.html
fast/viewport/viewport-105.html
fast/viewport/viewport-106.html
fast/viewport/viewport-107.html
fast/viewport/viewport-108.html
fast/viewport/viewport-109.html
fast/viewport/viewport-110.html
fast/viewport/viewport-111.html
fast/viewport/viewport-112.html
fast/viewport/viewport-113.html
fast/viewport/viewport-114.html
fast/viewport/viewport-115.html
fast/viewport/viewport-116.html
fast/viewport/viewport-117.html
fast/viewport/viewport-118.html
fast/viewport/viewport-129.html
fast/viewport/viewport-29.html
fast/viewport/viewport-30.html
fast/viewport/viewport-31.html
fast/viewport/viewport-32.html
fast/viewport/viewport-35.html
fast/viewport/viewport-36.html
fast/viewport/viewport-38.html
fast/viewport/viewport-39.html
fast/viewport/viewport-40.html
fast/viewport/viewport-41.html
fast/viewport/viewport-42.html
fast/viewport/viewport-43.html
fast/viewport/viewport-44.html
fast/viewport/viewport-47.html
fast/viewport/viewport-48.html
fast/viewport/viewport-49.html
fast/viewport/viewport-61.html
fast/viewport/viewport-62.html
fast/viewport/viewport-63.html
fast/viewport/viewport-64.html
fast/viewport/viewport-66.html
fast/viewport/viewport-67.html
fast/viewport/viewport-68.html
fast/viewport/viewport-69.html
fast/viewport/viewport-70.html
fast/viewport/viewport-71.html
fast/viewport/viewport-72.html
fast/viewport/viewport-73.html
fast/viewport/viewport-74.html
fast/viewport/viewport-75.html
fast/viewport/viewport-76.html
fast/viewport/viewport-77.html
fast/viewport/viewport-78.html
fast/viewport/viewport-79.html
fast/viewport/viewport-80.html
fast/viewport/viewport-81.html
fast/viewport/viewport-83.html
fast/viewport/viewport-85.html
fast/viewport/viewport-88.html
fast/viewport/viewport-90.html