Bug 76720 – [chromium] Refactor video drawing to be more data driven

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tim Dresser]

From bug 73059:

CCLayerImpl-derived classes all currently handle drawing themselves internally, making direct GL calls.  This is problematic for several reasons.  Testing these layers is tricky because testing must be done at a mock GL interface level rather than at a higher abstract level.  It makes culling closely dependent on how each layer draws.  It also makes some optimizations like layer squashing impossible.

To fix this, CCLayerImpl-derived classes should produce a set of quads to render with some basic information in them about how they are to be drawn.  This list can then be processed and drawn in a more data-driven and abstract manner.


CCVideoLayerImpl still handles drawing internally and should be transitioned to the data-driven approach.
This builds on the initial refactor from bug 76274 and is closely related to bug 76635, which refactors CCCanvasLayerImpl, and bug 76715, which refactors CCPluginLayerImpl.

[Tim Dresser]

I'm not sure that its possible to avoid quads sharing writable objects in this case.

In the draw code for video quads, the VideoFrameProvider is guarded by a Mutex, with a lock that lasts from the time a frame is requested from the VideoFrameProvider, until the time that the frame has been drawn and given back to the VideoFrameProvider. Based on the documentation in VideoFrameProvider.h, it sounds to me like this lock has to last this long, and encompass the actual drawing code.

If this is true, I don't see how to avoid sharing the VideoFrameProvider and its associated Mutex. In addition, since copyFrameToTextures needs to be called within the lock, nativeTextureSize and nativeTextureId must be shared and writable. 

If these assumptions are true (which I hope they aren't), then I think that a backpointer to the layer might be the least ugly approach.

Any ideas?

[Tim Dresser]

Created attachment 123595 [details]
Patch

[Adrienne Walker]

This suggestion might be a solution for the plugin layer as well, but I think this would be a whole lot cleaner if you separated out texture upload from quad drawing.

I think it'd be way better to add a CCLayerImpl::updateCompositorResources function and call it on each layer in calculateRenderPasses before you append quads.  I am nearly positive that the lock only needs to last through the uploads and not through the drawing.

[James Robinson]

In the H/W decoded case, there is no upload and the put() call has to be deferred until the draw is issued, IIUC.

[James Robinson]

In the H/W decoded case, there is no upload and the put() call has to be deferred until the draw is issued, IIUC.

[Ami Fischman]

(In reply to comment #5)
> In the H/W decoded case, there is no upload and the put() call has to be deferred until the draw is issued, IIUC.

That's right.

To recap: the {get,put}CurrentFrame() calls have acquire/release semantics for the frame involved.  The frame handed back by getCF() is only valid until it is returned by putCF() (at which point the underlying texture or memory can be reused for another frame).

(In reply to comment #1)
{get,put}CF() do not require holding a lock.
CCVLI::m_providerMutex is there to guard the m_provider member (so that it can be cleared safely), not individual frames.  Unless you're trying to enable clearing (deleting) a provider between getCF & putCF, I don't expect this to be relevant.

Happy to chat if this is confusing or seems wrong.

[Adrienne Walker]

(In reply to comment #6)
> (In reply to comment #5)
> > In the H/W decoded case, there is no upload and the put() call has to be deferred until the draw is issued, IIUC.
> 
> That's right.

Thanks for the clarification! :)

At an abstraction level, I feel like it's LayerRendererChromium's job to only turn quads into GL calls.  I think it's the layer's job to manage resources and generate quads.  That's why pushing all of that code into LayerRendererChromium feels like a layering violation to me.

I think texture upload could still be separated out from drawing if we added a CCLayerImpl::postDraw() function then, instead of a pre-draw one.  The guarantee would be that any layer that had appendQuads called on it would also get a postDraw() call.  Then, CCVideoLayerImpl::appendQuads could upload textures and take a lock, generating quads with not much more than texture ids and rects, while CCVideoLayerImpl::postDraw() would be able to hand back the frame.

[Ami Fischman]

It's potentially interesting that CCVLI::draw() has two, assymetric, codepaths:
1) NativeTexture frames: there is no "upload" needed, since the frame is already on a texture, but the underlying texture is reused as soon as putCurrentFrame() is called, so pCF() *must not* be called until the draw is done.
2) Non-NativeTexture frames: these must be uploaded to textures before pCF() is called, but then pCF() *can* be called *before* the draw is done, b/c CCVLI owns the texture in question, not the frame provider.

Today both paths through CCVLI::draw() have the same serialization look:
getCF() -> upload (no-op for native textures) -> draw -> putCF()

[Tim Dresser]

(In reply to comment #7)
> I think texture upload could still be separated out from drawing if we added a CCLayerImpl::postDraw() function then, instead of a pre-draw one.  The guarantee would be that any layer that had appendQuads called on it would also get a postDraw() call.  Then, CCVideoLayerImpl::appendQuads could upload textures and take a lock, generating quads with not much more than texture ids and rects, while CCVideoLayerImpl::postDraw() would be able to hand back the frame.

appendQuads can't upload the textures, since it doesn't have access to the GraphicsContext3D, TextureAllocator, LayerRenderCapabilities, or TextureManager. 

Based on the comments on bug 76715, it feels like the texture upload should happen in LayerChromium::updateCompositorResources. I don't see how I can do this, as the TextureAllocator (required by CCVideoLayerImpl::copyPlaneToTexture)  is created in LayerRendererChromium::initializeSharedObjects, so I can't access it in updateCompositorResources.

I don't see how textures can be uploaded in either appendQuads or in updateCompositorResources. 

If we can figure out how to upload textures in updateCompositorResources, then I think we would want to create LayerChromium::postDraw() instead of CCLayerImpl::postDraw().

Any suggestions?

[Dana Jansens]

(In reply to comment #9)
> (In reply to comment #7)
> > I think texture upload could still be separated out from drawing if we added a CCLayerImpl::postDraw() function then, instead of a pre-draw one.  The guarantee would be that any layer that had appendQuads called on it would also get a postDraw() call.  Then, CCVideoLayerImpl::appendQuads could upload textures and take a lock, generating quads with not much more than texture ids and rects, while CCVideoLayerImpl::postDraw() would be able to hand back the frame.
> 
> appendQuads can't upload the textures, since it doesn't have access to the GraphicsContext3D, TextureAllocator, LayerRenderCapabilities, or TextureManager. 
> 
> Based on the comments on bug 76715, it feels like the texture upload should happen in LayerChromium::updateCompositorResources. I don't see how I can do this, as the TextureAllocator (required by CCVideoLayerImpl::copyPlaneToTexture)  is created in LayerRendererChromium::initializeSharedObjects, so I can't access it in updateCompositorResources.

updateCompositorResources is where textures are uploaded that are painted on the main/webkit thread. IIUC video is creating frames on the impl/draw thread so it wouldn't belong in here.

> I don't see how textures can be uploaded in either appendQuads or in updateCompositorResources. 
> 
> If we can figure out how to upload textures in updateCompositorResources, then I think we would want to create LayerChromium::postDraw() instead of CCLayerImpl::postDraw().

LayerChromium exists on the main thread side (hence no Impl suffix) so it is not part of the "draw" process at all.

> Any suggestions?

TextureManager/Allocator are also constructs from the main thread side of the compositor. Without digging deeper I think you probably want to take how the CCVLI draw stuff was uploading textures and just move that into appendQuads modulo appropriate plumbing.

Hope this helps until enne gets in :)

[Adrienne Walker]

(In reply to comment #10)

> TextureManager/Allocator are also constructs from the main thread side of the compositor. Without digging deeper I think you probably want to take how the CCVLI draw stuff was uploading textures and just move that into appendQuads modulo appropriate plumbing.

Yeah, exactly.  All of this needs to stay on the CCLayerImpl side of the fence.

The only thing appendQuads is missing is a GraphicsContext3D*.  So, maybe add a CCLayerImpl::preDraw(LayerRendererChromium*) as well as a CCLayerImpl::postDraw(LayerRendererChromium*).  Call preDraw immediately before appendQuads and postDraw on all layers after the render passes have been sent to the LayerRendererChromium.

That'll keep appendQuads from being too overloaded, but it's effectively the same as moving it there with additional plumbing.

[Tim Dresser]

Created attachment 123988 [details]
Patch

[Ami Fischman]

Comment on attachment 123988 [details]
Patch

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

drive-by

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:191
> +        m_provider->putCurrentFrame(m_frame);

Here and in the other early return below, 
m_frame = 0;

[Tim Dresser]

Created attachment 123989 [details]
Patch

[James Robinson]

Comment on attachment 123989 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:77
> +    virtual void preDraw(LayerRendererChromium*) { };
> +    virtual void postDraw() { };

nit: we typically don't put trailing semicolons on the end of lines like this, since they don't do anything

WebKit's typical terminology for these sorts of calls is "willDraw(...) / didDraw()"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:268
> +    for (unsigned layerIndex = 0; layerIndex < layerList.size(); ++layerIndex) {

this looks like it'll only visit layers that are in the root layer's render surface, and not layers in other render surfaces. that can't be right!

can we use the layer iterators for this?

can we have tests for postDraw() being called on layers in render surfaces other than the root?

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:196
> +    if (!copyFrameToTextures(m_frame, format, layerRendererChromium)) {

why do we have to do this here? this means we'll be doing uploads even if we end up not drawing the quad (say it's culled out)

what about having this function just call getCurrentFrame() and give a pointer to the frame object to the quad. then, if we do draw the quad, we can copyFrameToTexture() at draw time where we already have access to the LRC.

if we do that then we don't need the LRC at all until we draw the quad, and we can in fact fold this entire function into appendQuads() and not need an additional pre-draw pass at all.

[Tim Dresser]

(In reply to comment #15)
> what about having this function just call getCurrentFrame() and give a pointer to the frame object to the quad. then, if we do draw the quad, we can copyFrameToTexture() at draw time where we already have access to the LRC.

copyFrameToTexture() requires mutating the layer. The layer's list of textures, nativeTextureId, m_nativeTextureSize, etc. have to be modified, and the results need to be shared among the quads. To call copyFrameToTexture() at draw time would require quads to have a backpointer to the layer, which we're trying to avoid.

Any suggestions on how to move the copyFrameToTexture() call into LayerRendererChromium::drawVideoQuad() without requiring a backpointer to the layer?

[James Robinson]

(In reply to comment #16)
> (In reply to comment #15)
> > what about having this function just call getCurrentFrame() and give a pointer to the frame object to the quad. then, if we do draw the quad, we can copyFrameToTexture() at draw time where we already have access to the LRC.
> 
> copyFrameToTexture() requires mutating the layer. The layer's list of textures, nativeTextureId, m_nativeTextureSize, etc. have to be modified, and the results need to be shared among the quads.

No, these are all draw-only state and they should live on the one quad that the video layer produces.

[Tim Dresser]

(In reply to comment #17)
> No, these are all draw-only state and they should live on the one quad that the video layer produces.

I was under the impression that each frame, the CCVideoLayerImpl produces a brand new quad, but the ManagedTexture objects are used in multiple frames.

Currently, the ManagedTexture objects are allocated in CCVideoLayerImpl::reserveTextures. A new ManagedTexture is only created if the layer doesn't have a texture for a specific plane. If my understanding is correct, that means that ManagedTexture objects will be reused for multiple frames. If we switch to calling reserveTextures in the quad, without a backpointer to the layer, then we'll only be able to use each ManagedTexture for one frame.

Does that sound correct?

[Adrienne Walker]

(In reply to comment #18)
> (In reply to comment #17)
> > No, these are all draw-only state and they should live on the one quad that the video layer produces.
> 
> I was under the impression that each frame, the CCVideoLayerImpl produces a brand new quad, but the ManagedTexture objects are used in multiple frames.
> 
> Currently, the ManagedTexture objects are allocated in CCVideoLayerImpl::reserveTextures. A new ManagedTexture is only created if the layer doesn't have a texture for a specific plane. If my understanding is correct, that means that ManagedTexture objects will be reused for multiple frames. If we switch to calling reserveTextures in the quad, without a backpointer to the layer, then we'll only be able to use each ManagedTexture for one frame.
> 
> Does that sound correct?

Yeah, you have the right picture.  We shouldn't need to get a new ManagedTexture each frame.

I think you could just call reserveTextures as a part of the willDraw() function, and pass the texture ids and frame to the quad, where it can do the upload as needed without needing to touch the layer anymore.

Or, in other words, separate out copyFrameToTexture's responsibilities into two pieces: the layer mutation piece (which lives on the layer) and the upload piece (which could live in LRC as a part of quad drawing).

[Tim Dresser]

Created attachment 124742 [details]
Patch

[WebKit Review Bot]

Attachment 124742 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   7140fe4..dad6b22  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106360 = 7140fe4a50c1f3a4ce373c6fcee4f0968c84e929
r106361 = dad6b22a289679e959ba3d3a10e83d72b2ea315c
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrienne Walker]

Comment on attachment 124742 [details]
Patch

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

Thanks for all of this.  Since you added willDraw in both this patch and the plugin patch, I think you'll have to let one of them land first and then rebase the other on top of ToT.

jamesr, what do you think?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:739
> +void LayerRendererChromium::drawCommon(const CCVideoDrawQuad* quad, Program* program, float widthScaleFactor, Platform3DObject textureId)

nit: Can you call this something else, like drawSingleTextureVideoQuad? drawCommon is a lot less meaningful of a name when moved to LRC.

(I also feel like this is yet another set of Programs that could be merged together, but I suppose those can all be done in another changelist.)

[Tim Dresser]

(In reply to comment #22)
> (From update of attachment 124742 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124742&action=review
> 
> Thanks for all of this.  Since you added willDraw in both this patch and the plugin patch, I think you'll have to let one of them land first and then rebase the other on top of ToT.
> 
> jamesr, what do you think?
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:739
> > +void LayerRendererChromium::drawCommon(const CCVideoDrawQuad* quad, Program* program, float widthScaleFactor, Platform3DObject textureId)
> 
> nit: Can you call this something else, like drawSingleTextureVideoQuad? drawCommon is a lot less meaningful of a name when moved to LRC.
> 
> (I also feel like this is yet another set of Programs that could be merged together, but I suppose those can all be done in another changelist.)

Yeah, I've been expecting to land the plugin patch first, and then resubmit the video patch. 

I'll submit 4 new bugs when this and the plugin patch have landed.

1. Merge PluginProgramBindings.
2. Merge Programs for Video.
3. Double Buffer Plugin Layer.
4. Remove the draw call from CCLayerImpl.

[Tim Dresser]

Created attachment 125010 [details]
Patch

[Tim Dresser]

Created attachment 125331 [details]
Patch

[Tim Dresser]

Note: this patch also removes CCLayerImpl::draw.

[Adrienne Walker]

Comment on attachment 125331 [details]
Patch

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

Other than the one below comment, this looks good to me.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:191
> -void CCLayerTreeHostImpl::calculateRenderPasses(CCRenderPassList& passes)
> +PassOwnPtr<CCLayerTreeHostImpl::CCLayerList> CCLayerTreeHostImpl::calculateRenderPasses(CCRenderPassList& passes)

style nit: It's weird to have this function return one value as a return value and another value by modifying a reference.  I kind of feel like the caller should create the CCLayerList on the stack and pass that in as a reference along with CCRenderPassList.

[James Robinson]

I'd prefer to hold off on this until we resolve a few other bugs with video for m18, to keep merges saner.

[Tim Dresser]

Created attachment 125858 [details]
Patch

[Tim Dresser]

Created attachment 127375 [details]
Patch

[James Robinson]

Comment on attachment 127375 [details]
Patch

Good stuff!

[WebKit Review Bot]

Comment on attachment 127375 [details]
Patch

Clearing flags on attachment: 127375

Committed r108093: <http://trac.webkit.org/changeset/108093>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.