Bug 46765 - Render textures directly in VideoFrameChromium
Summary: Render textures directly in VideoFrameChromium
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: Hin-Chung Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-28 16:31 PDT by Hin-Chung Lam
Modified: 2010-10-18 13:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.18 KB, patch)
2010-09-28 16:40 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2010-09-28 17:04 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (14.68 KB, patch)
2010-09-29 17:11 PDT, Hin-Chung Lam
jamesr: review-
Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2010-10-01 15:47 PDT, Hin-Chung Lam
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2010-09-28 16:31:15 PDT
Render textures directly in VideoFrameChromium
Comment 1 Hin-Chung Lam 2010-09-28 16:40:57 PDT
Created attachment 69131 [details]
Patch
Comment 2 James Robinson 2010-09-28 16:56:56 PDT
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.
Comment 3 Hin-Chung Lam 2010-09-28 17:04:29 PDT
Created attachment 69137 [details]
Patch
Comment 4 Hin-Chung Lam 2010-09-28 17:09:35 PDT
(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.
Comment 5 James Robinson 2010-09-28 17:13:03 PDT
(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.
Comment 6 Hin-Chung Lam 2010-09-28 17:22:46 PDT
(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.
Comment 7 Hin-Chung Lam 2010-09-28 17:26:25 PDT
(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.
Comment 8 James Robinson 2010-09-28 17:43:48 PDT
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.
Comment 9 Hin-Chung Lam 2010-09-29 17:11:06 PDT
Created attachment 69284 [details]
Patch
Comment 10 Hin-Chung Lam 2010-09-29 17:14:55 PDT
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..
Comment 11 WebKit Review Bot 2010-09-29 17:32:40 PDT
Attachment 69284 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4163030
Comment 12 Hin-Chung Lam 2010-09-29 17:34:29 PDT
(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.
Comment 13 Vangelis Kokkevis 2010-09-29 17:39:20 PDT
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.
Comment 14 Hin-Chung Lam 2010-09-29 17:42:54 PDT
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.
Comment 15 Vangelis Kokkevis 2010-09-29 17:47:49 PDT
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.
Comment 16 James Robinson 2010-10-01 15:23:56 PDT
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').
Comment 17 Hin-Chung Lam 2010-10-01 15:47:47 PDT
Created attachment 69531 [details]
Patch
Comment 18 Hin-Chung Lam 2010-10-01 15:49:11 PDT
(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.
Comment 19 James Robinson 2010-10-01 15:58:45 PDT
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.
Comment 20 Vangelis Kokkevis 2010-10-05 10:14:46 PDT
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