Bug 88363 – [chromium] Separate CCVideoDrawQuad and from the layer tree and video provider by removing ManagedTexture and WebVideoFrame pointers from the quad

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

[Dana Jansens]

[chromium] Separate CCVideoDrawQuad and from the layer tree and video provider by removing ManagedTexture and WebVideoFrame pointers from the quad

[Dana Jansens]

I've tested this by running a html5 player on vimeo.com and killing the GPU process mid playback, testing the MediaTest.* browser_tests, and layout tests. Any other testing suggestions welcome.

[Dana Jansens]

Created attachment 145869 [details]
Patch

[James Robinson]

Hm, are we doing a texture copy in the externally decoded video case today?  This code is a bit confusing...

[Dana Jansens]

I believe we are, since we copy the data() pointer for each plane, then don't use the textures anyways.

[Dana Jansens]

Well, I guess it depends if m_frame->planes() > 0. Maybe it's always 0 when a format with external texture is used?

[Ami Fischman]

(In reply to comment #5)
> Well, I guess it depends if m_frame->planes() > 0. Maybe it's always 0 when a format with external texture is used?

Yep.

[Dana Jansens]

Ah. I'll remove my FIXME :) Thanks Ami

[Dana Jansens]

Created attachment 145889 [details]
Patch

[James Robinson]

Comment on attachment 145889 [details]
Patch

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

The division of responsibilities here doesn't seem quite right (see inline comments).  Moving GC3D-aware stuff up to CC*LayerImpls is the wrong direction to be moving in.  We're syncing quads, not layers - right?  When we go to serialize the quads for a given video layer, we can take care of any uploads that need to happen at that point.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:82
> +    ASSERT(MaxPlanes == WebKit::WebVideoFrame::maxPlanes);

Both of these values are compile-time assertions, so this should also be a compile-time assertion (and probably not in this file).

Why do we even need a different enum - can't we just use WebKit::WebVideoFrame::maxPlanes directly?

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:264
> +void CCVideoLayerImpl::copyPlaneToTexture(GraphicsContext3D* context3d, const void* planeTextureData, int toPlaneIndex)

Having the texture copies on the CCVideoLayerImpl seems a lot worse than having it in LRC.  The layer impls shouldn't care what the context implementation is, that's a concern for the CCRenderer implementation (or something it holds on to, like a texture uploader class) to figure out.

[Dana Jansens]

(In reply to comment #9)
> (From update of attachment 145889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145889&action=review
> 
> The division of responsibilities here doesn't seem quite right (see inline comments).  Moving GC3D-aware stuff up to CC*LayerImpls is the wrong direction to be moving in.  We're syncing quads, not layers - right?  When we go to serialize the quads for a given video layer, we can take care of any uploads that need to happen at that point.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:82
> > +    ASSERT(MaxPlanes == WebKit::WebVideoFrame::maxPlanes);
> 
> Both of these values are compile-time assertions, so this should also be a compile-time assertion (and probably not in this file).

Oh.. COMPILE_ASSERT I guess? Fancy.

> Why do we even need a different enum - can't we just use WebKit::WebVideoFrame::maxPlanes directly?

Because we have m_textures[MaxPlanes] and it won't let us make it from a static const int. We could make the value in WebVideoFrame an enum instead of a static const, would that be a good thing to do?

> > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:264
> > +void CCVideoLayerImpl::copyPlaneToTexture(GraphicsContext3D* context3d, const void* planeTextureData, int toPlaneIndex)
> 
> Having the texture copies on the CCVideoLayerImpl seems a lot worse than having it in LRC.  The layer impls shouldn't care what the context implementation is, that's a concern for the CCRenderer implementation (or something it holds on to, like a texture uploader class) to figure out.

Hm, I was trying to more or less mimic the division of labour for other layers (CCTiledLayerImpl etc). The layer makes textures and puts contents into them as it deems appropriate, then puts texture ids into the quads. The quads/textures end up in an LRC somewhere and it sticks them all in a render target.

I was imagining the quad serializer being more simple I think, just put the quads into some pipe and ship it off over IPC. You are imagining this as being a texture upload step? Are you thinking of this also for tiled layers?

[Dana Jansens]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 145889 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145889&action=review
> > 
> > The division of responsibilities here doesn't seem quite right (see inline comments).  Moving GC3D-aware stuff up to CC*LayerImpls is the wrong direction to be moving in.  We're syncing quads, not layers - right?  When we go to serialize the quads for a given video layer, we can take care of any uploads that need to happen at that point.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:82
> > > +    ASSERT(MaxPlanes == WebKit::WebVideoFrame::maxPlanes);
> > 
> > Both of these values are compile-time assertions, so this should also be a compile-time assertion (and probably not in this file).
> 
> Oh.. COMPILE_ASSERT I guess? Fancy.
> 
> > Why do we even need a different enum - can't we just use WebKit::WebVideoFrame::maxPlanes directly?
> 
> Because we have m_textures[MaxPlanes] and it won't let us make it from a static const int. We could make the value in WebVideoFrame an enum instead of a static const, would that be a good thing to do?
> 
> > > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:264
> > > +void CCVideoLayerImpl::copyPlaneToTexture(GraphicsContext3D* context3d, const void* planeTextureData, int toPlaneIndex)
> > 
> > Having the texture copies on the CCVideoLayerImpl seems a lot worse than having it in LRC.  The layer impls shouldn't care what the context implementation is, that's a concern for the CCRenderer implementation (or something it holds on to, like a texture uploader class) to figure out.
> 
> Hm, I was trying to more or less mimic the division of labour for other layers (CCTiledLayerImpl etc). The layer makes textures and puts contents into them as it deems appropriate, then puts texture ids into the quads. The quads/textures end up in an LRC somewhere and it sticks them all in a render target.
> 
> I was imagining the quad serializer being more simple I think, just put the quads into some pipe and ship it off over IPC. You are imagining this as being a texture upload step? Are you thinking of this also for tiled layers?

Thinking further.. I do agree about doing this work in LRC. That seems right, I guess I was thinking we don't need to worry about it atm and just copying code. The "division of labour" issue seems unclear to me still - but with the current approach I can see adding/using public methods on the layerRenderer to copy the texture data into the GL textures (from CCVideoLayerImpl?) instead of accessing the context directly. I can try that out tomorrow, but I'd like to discuss further about deferring the uploads until serialization. I feel like we shouldn't have any pointers in Quads at all - including the texture data pointer from WebVideoFrame.

[James Robinson]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 145889 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145889&action=review
> > 
> > The division of responsibilities here doesn't seem quite right (see inline comments).  Moving GC3D-aware stuff up to CC*LayerImpls is the wrong direction to be moving in.  We're syncing quads, not layers - right?  When we go to serialize the quads for a given video layer, we can take care of any uploads that need to happen at that point.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:82
> > > +    ASSERT(MaxPlanes == WebKit::WebVideoFrame::maxPlanes);
> > 
> > Both of these values are compile-time assertions, so this should also be a compile-time assertion (and probably not in this file).
> 
> Oh.. COMPILE_ASSERT I guess? Fancy.

Yes, see Source/WebKit/chromium/src/AssertMatchingEnums.cpp

> 
> > Why do we even need a different enum - can't we just use WebKit::WebVideoFrame::maxPlanes directly?
> 
> Because we have m_textures[MaxPlanes] and it won't let us make it from a static const int. We could make the value in WebVideoFrame an enum instead of a static const, would that be a good thing to do?

D'oh.  Not sure about that - do you know of any particular reason this value is a const int instead of an enum, Ami?


> 
> > > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:264
> > > +void CCVideoLayerImpl::copyPlaneToTexture(GraphicsContext3D* context3d, const void* planeTextureData, int toPlaneIndex)
> > 
> > Having the texture copies on the CCVideoLayerImpl seems a lot worse than having it in LRC.  The layer impls shouldn't care what the context implementation is, that's a concern for the CCRenderer implementation (or something it holds on to, like a texture uploader class) to figure out.
> 
> Hm, I was trying to more or less mimic the division of labour for other layers (CCTiledLayerImpl etc). The layer makes textures and puts contents into them as it deems appropriate, then puts texture ids into the quads. The quads/textures end up in an LRC somewhere and it sticks them all in a render target.
> 
> I was imagining the quad serializer being more simple I think, just put the quads into some pipe and ship it off over IPC. You are imagining this as being a texture upload step? Are you thinking of this also for tiled layers?

I'm imagining that sometime has to figure out how to package up the quads into a pipe and send them over - it seems reasonable to me for that step to also take care of "serializing" the pixels into an appropriate format (stick them in shared memory, upload them to a texture, or whatever else is appropriate).  The layer should simply put enough information in the quad for this serialization to be possible.

One reason to do it this way is that we don't determine culling info, etc, until after we calculate the quads and passes.  CCLayerImpl::willDraw() is called for every layer in the tree.  This means it's quite common for us to call willDraw() on a video layer, then later realize that it's completely obscured by another layer or completely out of the viewport or whatnot.  In that case, we shouldn't bother with actually uploading the software-decoded contents into a texture - we should just release the frame back to the provider once the frame is over.  This sort of optimization is only possible if we make the decision about how to handle the resources when we're going through quads.

Tiled layers are different in that the texture resource decisions are made on the main thread, not the impl thread.

[James Robinson]

We could add some other indirection between the WebVideoFrame's data pointer and the Quad if it proves helpful, but think be sure to think about how to handle the culled/out of viewport quad case.

[Dana Jansens]

(In reply to comment #13)
> We could add some other indirection between the WebVideoFrame's data pointer and the Quad if it proves helpful, but think be sure to think about how to handle the culled/out of viewport quad case.

Ok yeh, good point thanks.

[Ami Fischman]

(In reply to comment #12)
> > > Why do we even need a different enum - can't we just use WebKit::WebVideoFrame::maxPlanes directly?
> > Because we have m_textures[MaxPlanes] and it won't let us make it from a static const int. We could make the value in WebVideoFrame an enum instead of a static const, would that be a good thing to do?
> D'oh.  Not sure about that - do you know of any particular reason this value is a const int instead of an enum, Ami?

Probably the only reason is that those static const int's predate my involvement with the project.
Please change them to enums.

[Dana Jansens]

So I think there's lots of ways we could approach this, and deferring upload to later is totally valid. But if we're going to go that route I'd like to do that at a later time. My goal right now is to make the separation between LRC/drawing and the layer tree complete, so that we can draw (without any serialization) without needing anything in the layer tree. Then start poking at serialization after.

Can we agree on that as a goal?

If so, I propose we control the texture upload in the layer's willDraw() using the CCRenderer's public APIs. Then the video quad will avoid any pointers. Bug #88435 will prevent this texture copy when the layer is occluded.

I used/added to the TextureCopier to copy the pixels into a texture. I'm not sure if there's a better place right now.

I moved one FIXME: I don't think we want to hold a lock on the WebVideoFrame until the host compositor is done with it. Instead, we want to double-buffer (or otherwise synchronize use of) the texture that a hardware decoding provider is giving us.

I wanted to get rid of the public use of "GC3Denum format" but we don't have the test infrastructure in place to do that with any kind of confidence, so I'll postpone trying to make it completely GC3D/GC2D agnostic until later.

I wanted to make the CCVideoLayerImpl code a bit more understandable and less GC3D'ish in its naming though, so I changed some variable names. So we now have an array "FramePlanes" instead of "Textures".

[Dana Jansens]

Created attachment 146111 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dana Jansens]

Created attachment 146112 [details]
Patch

missed removing the assert in the last upload.

[Dana Jansens]

Created attachment 146140 [details]
Patch

Please compare this with the previous patch. This version moves the texture upload out of TextureCopier and makes use of LayerTextureSubImage instead, as per some discussion with reveman.

The CCVideoLayerImpl then has to bind the textures itself, as we don't have any infrastructure that would take a textureId and do the upload. We also discussed the possibility of doing this texture upload outside of willDraw(), in some other scheduler upload state (like an impl->impl commit sorta), so that the time to draw a frame is predictable and does not include suddenly texture uploads as it would for video. This is something we should persue afterwards.

[Adrienne Walker]

Comment on attachment 146140 [details]
Patch

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

I agree with the general direction here, that uploading should be moved out of the quad and into the layer.  I'm still wrapping my head around how to solve the double buffering/synchronization FIXME, but it's clear to me that the quad can't be the responsible party.

> Source/WebCore/ChangeLog:22
> +        class. Instead, this method makes use of the LayerTextureSubImage class
> +        to copy the pixel data into the texture.

\o/

> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:52
> +    CCVideoLayerImpl::FramePlane* m_planes;

Whoa there.  The ChangeLog says "CCVideoDrawQuad should not contain any pointers".  m_planes is owned by the video layer.  m_frame used to be owned by the video layer.  Seems about the same to me.

I was expecting this patch to make CCVideoLayerImpl be a quad that directly contains all the data it needs with no pointer indirection.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:53
> +    unsigned m_frameProviderTextureId;

So, now that the quad doesn't do the upload, we have a quad that's overloaded for drawing two different things.  Can the single texture case just use CCTextureDrawQuad?

> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:55
>      const float* m_matrix;

Should this also not be a pointer and instead be a WebTransformationMatrix?

[Dana Jansens]

Comment on attachment 146140 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:52
>> +    CCVideoLayerImpl::FramePlane* m_planes;
> 
> Whoa there.  The ChangeLog says "CCVideoDrawQuad should not contain any pointers".  m_planes is owned by the video layer.  m_frame used to be owned by the video layer.  Seems about the same to me.
> 
> I was expecting this patch to make CCVideoLayerImpl be a quad that directly contains all the data it needs with no pointer indirection.

Oh, hm. I was thinking "this is an array not a pointer". But yeh it's still owned by the layer, thanks.

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:53
>> +    unsigned m_frameProviderTextureId;
> 
> So, now that the quad doesn't do the upload, we have a quad that's overloaded for drawing two different things.  Can the single texture case just use CCTextureDrawQuad?

hmm I'll take a peek

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:55
>>      const float* m_matrix;
> 
> Should this also not be a pointer and instead be a WebTransformationMatrix?

Ya same here, we should have our own array, or a WebTransformationMatrix as you say.

[Dana Jansens]

Created attachment 146897 [details]
Patch

Right-o here's a new version without any pointer copies into the CCVideoDrawQuad. We have our own array of FrameData in there now, and we're using the pleasant WebTransformationMatrix as well.

Also got rid of the setMatrix() method on the quad - everything comes in thru the constructor!

Also also moved the static constants used during draw out of the CCVideoLayerImpl class into LRC where they are used.

[Adrienne Walker]

Comment on attachment 146897 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:53
> +    unsigned m_frameProviderTextureId;
>      GC3Denum m_format;

I'm still interested in having fewer overloaded quads, but maybe that can be a follow-up patch.  I think we could have a YUV video quad (3 planes), streaming texture (id and matrix), and use CCTextureDrawQuad (id, texture transform) for RGBA and native.  I also wonder if anybody is using that texture matrix for anything more than uv translation and scale.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:248
> +void CCVideoLayerImpl::FramePlane::freeData(CCRenderer* layerRenderer)

Can you assert in the FramePlane destructor that this function has been called and textureId is 0?

[Dana Jansens]

Comment on attachment 146897 [details]
Patch

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

thanks.

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoDrawQuad.h:53
>>      GC3Denum m_format;
> 
> I'm still interested in having fewer overloaded quads, but maybe that can be a follow-up patch.  I think we could have a YUV video quad (3 planes), streaming texture (id and matrix), and use CCTextureDrawQuad (id, texture transform) for RGBA and native.  I also wonder if anybody is using that texture matrix for anything more than uv translation and scale.

I'll do the CCTextureDrawQuad first.

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:248
>> +void CCVideoLayerImpl::FramePlane::freeData(CCRenderer* layerRenderer)
> 
> Can you assert in the FramePlane destructor that this function has been called and textureId is 0?

yes!

[Dana Jansens]

Created attachment 146919 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 146919 [details]
Patch for landing

Clearing flags on attachment: 146919

Committed r120015: <http://trac.webkit.org/changeset/120015>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.