| Summary: | [Chromium] Add video stream texture support | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Sievers <sievers> | ||||||||||||||||
| Component: | New Bugs | Assignee: | Daniel Sievers <sievers> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | cc-bugs, dglazkov, enne, fischman, fishd, jamesr, nduca, qinmin, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Daniel Sievers
2012-02-10 16:14:18 PST
Created attachment 126602 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. 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? (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? 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. FragmentShaderRGBATexAlphaAndroid, should we remove those? 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. Created attachment 126860 [details]
Patch
(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. 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 (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. 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). 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? (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. Created attachment 128086 [details]
Patch
(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. 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? Created attachment 128093 [details]
Patch
(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. 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.
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? (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. (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 :)) 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). (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? 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) Created attachment 128119 [details]
Patch
(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 } 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? 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 Created attachment 128281 [details]
Patch
(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/? (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. 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 (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. Created attachment 128298 [details]
Patch
Comment on attachment 128298 [details]
Patch
R=me
Comment on attachment 128298 [details] Patch Clearing flags on attachment: 128298 Committed r108675: <http://trac.webkit.org/changeset/108675> All reviewed patches have been landed. Closing bug. |