Bug 46765 – Render textures directly in VideoFrameChromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Hin-Chung Lam]

Render textures directly in VideoFrameChromium

[Hin-Chung Lam]

Created attachment 69131 [details]
Patch

[James Robinson]

Comment on attachment 69131 [details]
Patch

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

What's the lifetime of the texture if it is provided by the frame?  I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer).

> WebCore/platform/graphics/chromium/VideoFrameChromium.h:66
> +        TypeGlTexture,

GlTexture is redundant - in WebCore the only type of texture is a GraphicsContext3D texture which is GL.

[Hin-Chung Lam]

Created attachment 69137 [details]
Patch

[Hin-Chung Lam]

(In reply to comment #2)
> (From update of attachment 69131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review
> 
> What's the lifetime of the texture if it is provided by the frame?  I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer).

The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later.

However we don't keep the textures for a long period of time, we should keep the texture until swap buffers happen. 

> 
> > WebCore/platform/graphics/chromium/VideoFrameChromium.h:66
> > +        TypeGlTexture,
> 
> GlTexture is redundant - in WebCore the only type of texture is a GraphicsContext3D texture which is GL.

Renamed it to TypeTexture.

[James Robinson]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 69131 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review
> > 
> > What's the lifetime of the texture if it is provided by the frame?  I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer).
> 
> The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later.

That's not true - layers outlive their associated elements.

[Hin-Chung Lam]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 69131 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review
> > > 
> > > What's the lifetime of the texture if it is provided by the frame?  I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer).
> > 
> > The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later.
> 
> That's not true - layers outlive their associated elements.

I see. In the case where we putVideoFrame() after swap buffers, what happens is that HTMLMediaElement will be destroyed, but the corresponding backend of media engine will still alive waiting for the video frame to be released. So we are safe to use the textures until we call putVideoFrame(). This patch need a later fix to hook to the swap buffers signal.

[Hin-Chung Lam]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #2)
> > > > (From update of attachment 69131 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=69131&action=review
> > > > 
> > > > What's the lifetime of the texture if it is provided by the frame?  I'm curious how that is tied in to the lifetime of the compositor (specifically how long it is safe to stash it in a member of the layer).
> > > 
> > > The lifetime tires directly to HTMLMediaElement, it is no longer than the compositor. Since the lifetime of VideoLayerChromium tires to HTMLMediaElement, we should be safe to use the texture as long as VideoLayerChromium is alive. There might be some edge cases that we need to deal with later.
> > 
> > That's not true - layers outlive their associated elements.
> 
> I see. In the case where we putVideoFrame() after swap buffers, what happens is that HTMLMediaElement will be destroyed, but the corresponding backend of media engine will still alive waiting for the video frame to be released. So we are safe to use the textures until we call putVideoFrame(). This patch need a later fix to hook to the swap buffers signal.

Oh and by the way, this code path will be exercised only behind a flag when hardware video decoding is enabled.

[James Robinson]

Comment on attachment 69137 [details]
Patch

If putCurrentFrame() can delete the texture then this will cause the compositor to attempt to use a texture after it's deleted since the draw call happens after updateContents.  IMO this should be fixed before landing.

[Hin-Chung Lam]

Created attachment 69284 [details]
Patch

[Hin-Chung Lam]

There are few main points in the latest patch:
1. Release the frame after the render calls if it contains textures
2. Add method to VideoLayerChromium so that it releases the frame if the owner of the frame is going away
3. VideoFrameProvider shouldn't be a OwnPtr in VideoLayerChromium because it is implemented by WebMediaPlayerClientImpl which is owned by MediaPlayer. I don't know why it didn't blow up before..

[WebKit Review Bot]

Attachment 69284 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4163030

[Hin-Chung Lam]

(In reply to comment #11)
> Attachment 69284 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/4163030

Need to roll deps to chromium to pull in new code that implements that method.

[Vangelis Kokkevis]

Comment on attachment 69284 [details]
Patch

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

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:404
> +    for (unsigned plane = 0; plane < VideoFrameChromium::maxPlanes; plane++) {

It seems that if frame->surfaceType() != VideoFrameChromium::TypeTexture then m_textures will contain ids of textures created by the compositor which you will need to delete before zero-ing them out.

[Hin-Chung Lam]

Comment on attachment 69284 [details]
Patch

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

>> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:404
>> +    for (unsigned plane = 0; plane < VideoFrameChromium::maxPlanes; plane++) {
> 
> It seems that if frame->surfaceType() != VideoFrameChromium::TypeTexture then m_textures will contain ids of textures created by the compositor which you will need to delete before zero-ing them out.

This method is called only in construct and in releaseCurrentFrame(). releaseCurrentFrame() will early return for the case of TypeTexture because a frame is never saved in this case.

[Vangelis Kokkevis]

Comment on attachment 69284 [details]
Patch

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

>>> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:404
>>> +    for (unsigned plane = 0; plane < VideoFrameChromium::maxPlanes; plane++) {
>> 
>> It seems that if frame->surfaceType() != VideoFrameChromium::TypeTexture then m_textures will contain ids of textures created by the compositor which you will need to delete before zero-ing them out.
> 
> This method is called only in construct and in releaseCurrentFrame(). releaseCurrentFrame() will early return for the case of TypeTexture because a frame is never saved in this case.

Ah, makes sense. I missed the early return part. Thanks for clarifying.

[James Robinson]

Comment on attachment 69284 [details]
Patch

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

> WebCore/ChangeLog:4
> +        Reviewed by NOBODY (OOPS!).
> +

This needs a one-line patch description (usually the bug title) and a link to the bug

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:257
> +
> +    // FIXME: If surface type is GL texture we should call putCurrentFrame() only after swap buffers happened.
>      m_provider->putCurrentFrame(frame);

I think this goes away now

> WebKit/chromium/public/WebVideoFrame.h:56
> +        SurfaceTypeGlTexture,

This should probably match the other SurfaceType enum (i.e. not have 'Gl').

[Hin-Chung Lam]

Created attachment 69531 [details]
Patch

[Hin-Chung Lam]

(In reply to comment #17)
> Created an attachment (id=69531) [details]
> Patch

DEPS chromium has already been rolled forward so it's not necessary to do it in this patch.

[James Robinson]

Comment on attachment 69531 [details]
Patch

Looks good to me.  Marking R+, although it would be great if Vangelis would give this a once-over before landing as well.

[Vangelis Kokkevis]

Comment on attachment 69531 [details]
Patch

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

Only nits from my side. Otherwise looks good.

> WebCore/platform/graphics/chromium/VideoLayerChromium.h:52
> +    // This function is called by VideFrameProvider. When this method is called

typo: VideoFrameProvider

> WebKit/chromium/ChangeLog:8
> +

Missing link to bug