Bug 78398 – [Chromium] Add video stream texture support

• Component: New Bugs
• Status: RESOLVED FIXED

[Daniel Sievers]

[Chromium] Add video stream texture support

[Daniel Sievers]

Created attachment 126602 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

Comment on attachment 126602 [details]
Patch

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

I've seen the compositor stuff before and think it's generally on track. I'll have to think through the proxy setup/teardown paths a bit to make sure I understand them and they're completely safe. Two general notes on copyright headers: 1) It's 2012 now 2) try to use 2-clause for new files

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:105
> +    , m_layerTreeHostImpl(0)

what's supposed to initialize this? or is that a FIXME?

[Daniel Sievers]

(In reply to comment #3)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:105
> > +    , m_layerTreeHostImpl(0)
> 
> what's supposed to initialize this? or is that a FIXME?

Yes, it would need another change to properly set this.
Should I take a look at that? I think you said we can simply set this when we create the layer impl since it doesn't change?

[Daniel Sievers]

Also maybe the new shaders in ShaderChromium should be renamed to not have Android in the name? Since everything else is named more generically, and this is more of an 'external texture' (GL_OES_EGL_image_external) shader.

[Min Qin]

FragmentShaderRGBATexAlphaAndroid, should we remove those?

[Darin Fisher (:fishd, Google)]

Comment on attachment 126602 [details]
Patch

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

> Source/WebKit/chromium/public/WebMediaPlayer.h:166
> +    virtual WebStreamTextureProxy* createStreamTextureProxy() { return 0; }

nit: normally the peer to a created interface is a *Client interface, and normally a *Client instance is passed as a parameter to the create* function.  See WebKitPlatformSupport::createURLLoader for example.

> Source/WebKit/chromium/public/WebStreamTextureProxy.h:37
> +class WebStreamTextureProxy {

what is a WebStreamTextureProxy?  It seems to have the properties that it can tell its client when a "frame" is available and it can also push a new matrix.  where is the data for the frame?

> Source/WebKit/chromium/public/WebStreamTextureProxy.h:44
> +        virtual void onFrameAvailable() = 0;

nit: we don't use the "on" prefix in the webkit api.  we use did/will prefixes so that we are more precise about if the notification is happening before or after a state change.

[Daniel Sievers]

Created attachment 126860 [details]
Patch

[Daniel Sievers]

(In reply to comment #7)
> (From update of attachment 126602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126602&action=review
> 
> > Source/WebKit/chromium/public/WebMediaPlayer.h:166
> > +    virtual WebStreamTextureProxy* createStreamTextureProxy() { return 0; }
> 
> nit: normally the peer to a created interface is a *Client interface, and normally a *Client instance is passed as a parameter to the create* function.  See WebKitPlatformSupport::createURLLoader for example.
> 


See new patch. Is that roughly what you had in mind?


> > Source/WebKit/chromium/public/WebStreamTextureProxy.h:37
> > +class WebStreamTextureProxy {
> 
> what is a WebStreamTextureProxy?  It seems to have the properties that it can tell its client when a "frame" is available and it can also push a new matrix.  where is the data for the frame?
> 

In the implementation in chromium it corresponds to a proxy object that attaches to the gpu channel and receives notifications. The reason I'm exposing it here is that it needs to live on the correct thread, and its easiest to have the listener/client simply own it to manage lifetime. I added a comment, esp. since it doesn't really have members anymore now:

    // A proxy object held by StreamTextureClients that allows for managing  internal
    // state and the lifetime of notifications.
    class StreamTextureProxy {


> > Source/WebKit/chromium/public/WebStreamTextureProxy.h:44
> > +        virtual void onFrameAvailable() = 0;
> 
> nit: we don't use the "on" prefix in the webkit api.  we use did/will prefixes so that we are more precise about if the notification is happening before or after a state change.

Done.

[James Robinson]

Comment on attachment 126860 [details]
Patch

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

Not sure I fully grok the Proxy ownership issue.  What object are we trying to notify about destruction on which thread?  We have a notification set up for when CCVideoLayerImpls die via the VideoFrameProvider interface which is wired up to WebMediaPlayerClientImpl - is that not far enough?

> Source/WebCore/platform/graphics/chromium/StreamTextureClient.h:38
> +public:

please add a protected virtual d'tor

> Source/WebKit/chromium/src/StreamTextureProxyImpl.h:53
> +        ~StreamTextureClientImpl() { }

virtual, please

[Daniel Sievers]

(In reply to comment #10)
> (From update of attachment 126860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126860&action=review
> 
> Not sure I fully grok the Proxy ownership issue.  What object are we trying to notify about destruction on which thread?  

We are actually not notifying anyone about destruction, since this would leave races. Instead this is implemented similar to the RendererGLContext messages with messages being posted from io to compositor thread (bypassing main thread entirely). Essentially, the proxy wraps a (smart) weak pointer so that it handles the recipient going away without races. 

>> We have a notification set up for when CCVideoLayerImpls die via the VideoFrameProvider interface which is wired up to WebMediaPlayerClientImpl - is that not far enough?

I think the listener proxy wants to live on the compositor thread in this case, since it needs to detach the listener (or client in this case) without races, and actually the proxy object itself needs to be deleted on the compositor thread also (due to the weak ptr usage).

I guess the funky thing about this client interface is that it is called on the compositor thread. And that we post to a node like the cc layer impl directly, which is both implemented through the proxy object.

[Daniel Sievers]

James, I looked at how the proxy ownership could be moved towards the provider by using the existing interface. The case that would not work is the CCVideoLayerImpl outliving the provider. 

In this case we cannot safely disconnect the impl layer (listener) from the message proxy object on the impl thread.
And disconnecting the listener from the message proxy from the main thread when the provider goes away in that case does not work either without having a lock around access to the listener pointer in the proxy object (i.e. having to acquire a lock every time we call into the listener).

[Nat Duca]

How does this feature set relate to HW decode paths as well as software video decode on another thread? Or is this 100% specific to Android?

[Daniel Sievers]

(In reply to comment #13)
> How does this feature set relate to HW decode paths as well as software video decode on another thread? Or is this 100% specific to Android?

The main functionality I'm trying to upstream is setting a video layer as 'needs draw' on the impl thread, when a new frame has been streamed. (There is a second callback for a change in the transformation matrix, which can be used to implement y-flips and other transforms needed for hw-decoded pictures.) 

It seems it's potentially useful for any implementation that decodes frames off the main thread, but I have no proof to say that it is practical for platforms other than Android.

This was done this way in order to
- bypass the main thread when new frames become available and we want to trigger a draw
- route the message to the layer node directly, so that the updates allow for being implemented efficiently (limiting damage, collapsing updates through scheduler)


The other changes (mainly in CCVideoLayerImpl and ShaderChromium) are to allow for rendering from GL_OES_EGL_image_external targets.

[Daniel Sievers]

Created attachment 128086 [details]
Patch

[Daniel Sievers]

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

This patch removes the 'CCVideoLayerImpl owns a proxy'. Instead have a client interface for the two notifications that WebMediaPlayerImpl implements in WebKit (and that forwards to the VideoFrameProvider::Client interface).

Also it's refactored after https://bugs.webkit.org/show_bug.cgi?id=76720.

[James Robinson]

Comment on attachment 128086 [details]
Patch

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

Compositor changes LGTM.  I like how small this patch is now :)

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:686
> +    // No lock since this gets called on the client's thread.

can we have ASSERT()s for thread correctness?

[Daniel Sievers]

Created attachment 128093 [details]
Patch

[Daniel Sievers]

(In reply to comment #17)
> > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:686
> > +    // No lock since this gets called on the client's thread.
> 
> can we have ASSERT()s for thread correctness?

Done. Also updated the comments in CCVideoLayerImpl indicating which thread the VideoFrameProvider::Client APIs get called on.

[James Robinson]

Comment on attachment 128093 [details]
Patch

OK, then looks fine from my end.  R=me, but give it a day in case Darin or anyone from the video team have comments.

[Ami Fischman]

Comment on attachment 128093 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/VideoFrameChromium.h:64
> +        ExternalTexture,

Is it obvious what the difference between "NativeTexture" and "ExternalTexture" is?  (it isn't to me)

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:76
> +    , m_layerTreeHostImpl(0)

Where is this set to non-0?

[Daniel Sievers]

(In reply to comment #21)
> (From update of attachment 128093 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128093&action=review
> 
> > Source/WebCore/platform/graphics/chromium/VideoFrameChromium.h:64
> > +        ExternalTexture,
> 
> Is it obvious what the difference between "NativeTexture" and "ExternalTexture" is?  (it isn't to me)

I picked it based on it using GL_OES_EGL_image_external, but am open to other suggestions!

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:76
> > +    , m_layerTreeHostImpl(0)
> 
> Where is this set to non-0?

Nowhere yet upstream :)  I was going to work on a separate patch for this, since James suggested every layer impl should maybe just have this backpointer.

[Ami Fischman]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 128093 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128093&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/VideoFrameChromium.h:64
> > > +        ExternalTexture,
> > 
> > Is it obvious what the difference between "NativeTexture" and "ExternalTexture" is?  (it isn't to me)
> 
> I picked it based on it using GL_OES_EGL_image_external, but am open to other suggestions!

I wonder whether these can be a single type, or whether there's some key difference between them (such as the type of texture, or the way they're accessed, or something else).
If there is enough of a difference between them to require two different types, can commentary be added indicating each type's purpose that makes it clear what the difference is?
(I'd suggest something more specific but like I said, I don't know what diffs are :))

[James Robinson]

As I understand the differences are the shader you have to use and the texture target to bind to (TEXTURE_2D vs TEXTURE_EXTERNAL_OES).

[Daniel Sievers]

(In reply to comment #24)
> As I understand the differences are the shader you have to use and the texture target to bind to (TEXTURE_2D vs TEXTURE_EXTERNAL_OES).

If the VideoFrameChromium also had the texture target (which then gets also set on the draw quad), everything could be a NativeTexture and we differentiate based on the target. Sounds better?

[Ami Fischman]

Yes, that sounds better to me.
(presumably you're planning to switch shaders based on the target?  WFM but I know very little about the world of shaders)

[Daniel Sievers]

Created attachment 128119 [details]
Patch

[Daniel Sievers]

(In reply to comment #26)
> Yes, that sounds better to me.
> (presumably you're planning to switch shaders based on the target?  WFM but I know very little about the world of shaders)

Done. Needs https://chromiumcodereview.appspot.com/9416087/ to be functional though.

I've renamed LayerRendererChromium::drawNativeTexure() to drawNativeTexture2D().
It still passes through the texture target in the draw quad's format field for native textures, as this is what it did before.

There's also a small bugfix I added, which causes me crashes:
126 void CCVideoLayerImpl::willDraw(LayerRendererChromium* layerRenderer)
127 {
...
131 
132     if (!m_provider) {
133         m_frame = 0;  // <- I added this line or LayerRendererChromium might access a stale pointer
134         return;
135     }

[Ami Fischman]

Comment on attachment 128119 [details]
Patch

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

> Source/WebKit/chromium/public/WebVideoFrame.h:54
> +        FormatExternalTexture,

Can drop this now?

[WebKit Review Bot]

Comment on attachment 128119 [details]
Patch

Attachment 128119 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11559406

New failing tests:
media/video-load-preload-none.html

[Daniel Sievers]

Created attachment 128281 [details]
Patch

[Daniel Sievers]

(In reply to comment #29)
> (From update of attachment 128119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128119&action=review
> 
> > Source/WebKit/chromium/public/WebVideoFrame.h:54
> > +        FormatExternalTexture,
> 
> Can drop this now?

Oops, thanks. Done.

Do you think the test failure on the review bot will go away after landing http://codereview.chromium.org/9416087/?

[Ami Fischman]

(In reply to comment #32)
> (In reply to comment #29)
> > (From update of attachment 128119 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128119&action=review
> > 
> > > Source/WebKit/chromium/public/WebVideoFrame.h:54
> > > +        FormatExternalTexture,
> > 
> > Can drop this now?
> 
> Oops, thanks. Done.
> 
> Do you think the test failure on the review bot will go away after landing http://codereview.chromium.org/9416087/?

http/tests/media/video-referer.html is another open bug; I suspect the bot just needs to pick up a newer test_expectations.txt.
media/video-load-preload-none.html I don't know about; you should probably run it on your machine and see what's up with that crash.

[WebKit Review Bot]

Comment on attachment 128281 [details]
Patch

Attachment 128281 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11562621

New failing tests:
media/video-load-preload-none.html

[Daniel Sievers]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #29)
> > > (From update of attachment 128119 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=128119&action=review
> > > 
> > > > Source/WebKit/chromium/public/WebVideoFrame.h:54
> > > > +        FormatExternalTexture,
> > > 
> > > Can drop this now?
> > 
> > Oops, thanks. Done.
> > 
> > Do you think the test failure on the review bot will go away after landing http://codereview.chromium.org/9416087/?
> 
> http/tests/media/video-referer.html is another open bug; I suspect the bot just needs to pick up a newer test_expectations.txt.
> media/video-load-preload-none.html I don't know about; you should probably run it on your machine and see what's up with that crash.

Ok, that crash was a missing check for m_webMediaPlayer to be non-null in ~WebMediaPlayerClientImpl().

I guess I also want to add that check in setStreamTextureClient(), but I just realized that load() (on the main thread) and setStreamTextureClient() (on impl) thread can potentially race. So it seems like I also have to take the provider lock where we set or reset m_webMediaPlayer.

[Daniel Sievers]

Created attachment 128298 [details]
Patch

[James Robinson]

Comment on attachment 128298 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 128298 [details]
Patch

Clearing flags on attachment: 128298

Committed r108675: <http://trac.webkit.org/changeset/108675>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.