Bug 54047 – [chromium] Separate the "update" and "draw" portions of LayerRendererChromium's drawLayers function

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

[James Robinson]

[chromium] Separate the "update" and "draw" portions of LayerRendererChromium's drawLayers function

[James Robinson]

Created attachment 81718 [details]
WIP

[James Robinson]

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.

[Adrienne Walker]

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.

[Nat Duca]

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.

[James Robinson]

(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 :)

[Vangelis Kokkevis]

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).

[James Robinson]

Created attachment 82086 [details]
Patch

[James Robinson]

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.

[James Robinson]

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.

[James Robinson]

Created attachment 82101 [details]
Patch

[Nat Duca]

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()

[James Robinson]

(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.

[James Robinson]

Created attachment 83352 [details]
Patch

[James Robinson]

Updated to show where I'm going.  I'll break bits of this off to smaller patches to review+land.

[Nat Duca]

Me likes. Thanks for landing this James.

[James Robinson]

Created attachment 83867 [details]
Patch

[Vangelis Kokkevis]

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; -> '

[James Robinson]

(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.

[Vangelis Kokkevis]

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

[James Robinson]

Created attachment 84474 [details]
Patch

[James Robinson]

(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.

[Vangelis Kokkevis]

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?

[James Robinson]

(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.

[James Robinson]

ping?

[Kenneth Russell]

Comment on attachment 84474 [details]
Patch

Only scanned the review; rs=me.

[James Robinson]

Committed r80482: <http://trac.webkit.org/changeset/80482>

[WebKit Review Bot]

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