[wip] [chromium] Remove the WebVideoFrame, WebVideoFrameProvider, and WebVideoLayer classes.
Created attachment 193405 [details] Patch
This isn't for landing. This will have to land in pieces. But I'd like to do review on the final appearance before I split it up, to make sure I'm on the right page.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 193405 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/Platform.gypi', u'Source/Platform/chromium/public/WebCompositorSupport.h', u'Source/Platform/chromium/public/WebLayer.h', u'Source/Platform/chromium/public/WebVideoFrame.h', u'Source/Platform/chromium/public/WebVideoFrameProvider.h', u'Source/Platform/chromium/public/WebVideoLayer.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/WebKit.gypi', u'Source/WebKit/chromium/public/WebMediaPlayer.h', u'Source/WebKit/chromium/public/WebStreamTextureClient.h', u'Source/WebKit/chromium/public/WebVideoFrame.h', u'Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp', u'Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h', u'Source/WebKit/chromium/tests/WebMediaPlayerClientImplTest.cpp']" exit_code: 1 Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 193405 [details] Patch Attachment 193405 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17200298
Comment on attachment 193405 [details] Patch Attachment 193405 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17189580
Comment on attachment 193405 [details] Patch Attachment 193405 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17234034
Comment on attachment 193405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193405&action=review > Source/Platform/chromium/public/WebLayer.h:206 > + // True if the layer is not attached to a LayerTreeHost. There's no LayerTreeHost in the WebKit API and there's no real notion of 'attached'. There is a WebLayerTreeView which has a WebLayer that's set as the root layer, so I think you could define this function in terms of that > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:100 > -#if USE(ACCELERATED_COMPOSITING) > - if (m_videoFrameProviderClient) > - m_videoFrameProviderClient->stopUsingProvider(); > // No need for a lock here, as getCurrentFrame/putCurrentFrame can't be > // called now that the client is no longer using this provider. Also, load() > // and this destructor are called from the same thread. > if (m_webMediaPlayer) > m_webMediaPlayer->setStreamTextureClient(0); How does the teardown path work in this new world? We've had a lot of deadlocks/crashes around this logic in the past.
Created attachment 193625 [details] Patch
Comment on attachment 193405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193405&action=review >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:100 >> m_webMediaPlayer->setStreamTextureClient(0); > > How does the teardown path work in this new world? We've had a lot of deadlocks/crashes around this logic in the past. Thanks for this great question. I went through before/after of the teardown paths, and found some problems and missing mutexes and have cleaned this up. Here's the state of the world with patchset #2 (also updated the chromium side patch). BEFORE - If the video layer is destroyed: ~VideoLayerImpl() => VideoFPClientImpl::Stop() => WebMediaPlayerClientImpl::setVideoFrameProviderClient(NULL) locks access to ~WebMediaPlayerImpl, setVFPClient, getCurrentFrame, putCurrentFrame. => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL; with provider_ lock. sets m_videoFrameProviderClient = NULL => WebMediaPlayerImpl::setStreamTextureClient(NULL) AFTER - If the video layer is destroyed: ~VideoLayerImpl() => VideoFPClientImpl::Stop() => WebMediaPlayerImpl::SetVideoFrameProviderClient(NULL) locks access to ~WebMediaPlayerImpl, SetVideoFrameProviderClient => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL; with provider_lock. sets video_frame_provider_client_ = NULL => WebMediaPlayerImpl::setStreamTextureClient(NULL) Then, when WebMediaPlayImpl later shuts down, it has no pointer to the video layer. And the layer has no pointer to the media player. ================= BEFORE - If the WebMediaPlayerClientImpl is destroyed: ~WebMediaPlayerClientImpl() =>VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL; with provider_ lock. =>WebMediaPlayImpl::SetStreamTextuerClient(NULL) =>~WebMediaPlayerImpl() AFTER - If the WebMediaPlayerClientImpl is destroyed, then ~WebMediaPlayerClientImpl() =>~WebMediaPlayerImpl() =>SetVideoFrameProviderClient(NULL) locks access to ~WebMediaPlayerImpl, SetVideoFrameProviderClient => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL; with provider_lock. => WebMediaPlayerImpl::setStreamTextureClient(NULL) ======================== BEFORE - If the WebMediaPlayerClientImpl::loadRequested() destoys the media player. WebMediaPlayerClientImpl::loadRequested() locks access to setVideoFrameProviderClient, getCurrentFrame, putCurrentFrame =>~WebMediaPlayerImpl() =>replace WebMediaPlayerImpl* no need to replace VFProvider pointer. AFTER - If the WebMediaPlayerClientImpl::loadRequested() destoys the media player. WebMediaPlayerClientImpl::loadRequested() =>~WebMediaPlayerImpl() =>SetVideoFrameProviderClient(NULL) locks access to ~WebMediaPlayerImpl, SetVideoFrameProviderClient => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL; with provider_lock. => WebMediaPlayerImpl::setStreamTextureClient(NULL) >>>>>The video layer needs to be recreated with the new provider<<<<< =>unRegister the video layer and destroy it. =>replace WebMediaPlayerImpl*
I'd like to verify the WedMediaPlayerClientImpl::loadRequested() path that recreates a new WebMediaPlayer works correctly, but I'm not exactly sure how to invoke it. Any test cases would be appreciated. @scherkus?
Comment on attachment 193625 [details] Patch Attachment 193625 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17235073
Comment on attachment 193625 [details] Patch Attachment 193625 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17114990
Comment on attachment 193625 [details] Patch Attachment 193625 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17172328
Created attachment 193633 [details] Patch isOrphan => True if the layer is not part of a tree attached to a WebLayerTreeView.
Comment on attachment 193633 [details] Patch Attachment 193633 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17116669
Comment on attachment 193633 [details] Patch Attachment 193633 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17184358
Comment on attachment 193633 [details] Patch Attachment 193633 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17130616
I'm not a WK pro by any means, but this combined with the Chromium CL looks good (in other words, I can't spot anything horribly wrong)
Created attachment 193701 [details] Patch Remove WebStreamTextureClient
Comment on attachment 193701 [details] Patch Attachment 193701 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17197481
Comment on attachment 193701 [details] Patch Attachment 193701 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17231194
Comment on attachment 193701 [details] Patch Attachment 193701 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17197480
if webStreamTextureClient is removed, we need a replacement strategy for streamTextureProxy to communicate with videolayerimpl when a video frame arrives. (In reply to comment #20) > Created an attachment (id=193701) [details] > Patch > > Remove WebStreamTextureClient
(In reply to comment #24) > if webStreamTextureClient is removed, we need a replacement strategy for streamTextureProxy to communicate with videolayerimpl when a video frame arrives. Please take a look at https://codereview.chromium.org/12902002/. I have StreamTextureProxy going directly to cc::VideoFrameProvider::Client.
here's an updated look at where we're at now that I have a better grasp of what's going on with stream texture and locking and whatnot: BEFORE - If the video layer is destroyed: ~VideoLayerImpl() => VideoFPClientImpl::Stop() => WebMediaPlayerClientImpl::setVideoFrameProviderClient(NULL) locks m_videoFrameProviderClient => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL sets m_videoFrameProviderClient = NULL AFTER - If the video layer is destroyed: ~VideoLayerImpl() => VideoFPClientImpl::Stop() => WebMediaPlayerImpl::SetVideoFrameProviderClient(NULL) locks video_frame_provider_client_ => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL sets video_frame_provider_client_ = NULL Then, when WebMediaPlayImpl later shuts down, it has no pointer to the video layer. And the layer has no pointer to the media player. After the video layer is destroyed the compositor will not use the VFProvider that was attached to the layer anymore, so we don't need to worry about GetCurrentFrame() being called on the WMPImpl anymore. The VideoLayerImpl ensures the main thread is blocked if it sets the VFPClient to NULL, so the lock in SetVideoFrameProviderClient is only there to ensure the value changed on the compositor thread is updated correctly on the main thread - not for blocking the main thread. ================= BEFORE - If the WebMediaPlayerClientImpl is destroyed: ~WebMediaPlayerClientImpl() =>VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL =>~WebMediaPlayerImpl() AFTER - If the WebMediaPlayerClientImpl is destroyed, then ~WebMediaPlayerClientImpl() =>~WebMediaPlayerImpl() =>SetVideoFrameProviderClient(NULL) locks video_frame_provider_client_ => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL This is as safe as before. Previously, access to the m_videoFrameProviderClient was not guarded by the m_webMediaPlayerMutex, which could cause a crash. But this was safe-guarded by compositor only setting the VideoFrameProviderClient to NULL when the main thread was blocked. The video layer impl's connection to its VFProvider is broken still. The VideoLayerImpl ensures the main thread is blocked if it sets the VFPClient to NULL, so the lock in SetVideoFrameProviderClient is only there to ensure the value changed on the main thread is updated correctly on the compositor thread - not for blocking the compositor thread. ======================== BEFORE - If the WebMediaPlayerClientImpl::loadRequested() destroys the media player. WebMediaPlayerClientImpl::loadRequested() locks access to m_webMediaPlayer =>~WebMediaPlayerImpl() =>replace WebMediaPlayerImpl* No need to replace VFProviderClient pointer since it points to the WMPClientImpl. Ensures that getCurrentFrame() does not happen while the WMPImpl is destroyed with the m_webMediaPlayerMutex. AFTER - If the WebMediaPlayerClientImpl::loadRequested() destroys the media player. WebMediaPlayerClientImpl::loadRequested() =>~WebMediaPlayerImpl() =>SetVideoFrameProviderClient(NULL) locks video_frame_provider_client_ => VideoFPClientImpl::StopUsingProvider() sets provider_ = NULL >>>>>The video layer needs to be recreated with the new provider<<<<< =>unRegister the video layer and destroy it. =>replace WebMediaPlayerImpl* >>>>>Later, the video layer will be recreated and pointed at the WMPImpl as its VFProviderClient<<<<< The video layer impl's connection to its VFProvider (the WMPImpl) is broken before destroying the WMPImpl. If the compositor tries to GetCurrentFrame() afterward it will fail in VFPClientImpl. On the next commit the VideoLayerImpl will be destroyed. Later, when the ready state changes, a new video layer is added to the compositing tree and pointed at the new WMPImpl which was created here.
Comment on attachment 193701 [details] Patch I really like the direction this is going on. One small change I'd like to see as we discussed on IRC is moving the ownership of the WebLayer exclusively to the WebMediaPlayer and exposing only a non-owning pointer to WebMediaPlayerClientImpl. It looks like the way that would be most consistent with how plugins work would be to add virtual void setWebLayer(WebLayer*) to WebMediaPlayerClient and in the implementation figure out whether that means to register/unregister the layer and then store a (raw, weak) pointer in a member on WebMediaPlayerClientImpl. (r- for now to get out of the queue, but otherwise this looks super nice)
Created attachment 194137 [details] Patch moved ownership
Comment on attachment 194137 [details] Patch Attachment 194137 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17246139
Comment on attachment 194137 [details] Patch Attachment 194137 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17224207
Comment on attachment 194137 [details] Patch Attachment 194137 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17145760
Comment on attachment 194137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194137&action=review This looks great. How do we stage this for landing? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:283 > +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer) I think you need to kick a synthetic style recalc too so the video layer gets added to the compositing tree. See WebPluginContainerImpl::setWebLayer(). is this happening somewhere else currently? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:289 > + m_videoLayer->setOpaque(m_opaque); I think this would be better set on the chromium side of the world - in fact m_opaque doesn't need to exist in the WebKit side at all. but you don't have to do that in the first patch.
Comment on attachment 194137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194137&action=review I'm thinking that tomorrow morning I will basically put #ifndef NEW_MEDIA_OWNERSHIP around all the - lines in the chromium diff, and #ifdef NEW_MEDIA_OWNERSHIP around all the + lines in the chromium diff, and go for landing that first. Then, once it's landed, stick a #define NEW_MEDIA_OWNERSHIP in WebMediaPlayer.h or something, and land this patch as is. Finally, after a webkit roll, go back and remove all the #ifndefs, as well as the #ifdefs and all unused code inside them. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:283 >> +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer) > > I think you need to kick a synthetic style recalc too so the video layer gets added to the compositing tree. See WebPluginContainerImpl::setWebLayer(). is this happening somewhere else currently? I don't think anything has been doing that before, and I believe it's because we just don't make a webLayer if we're not going to use compositing (see supportsAcceleratedRendering()). Going a step at a time I've cleaned up the whole "are we going to composite this?" mess, now it looks like this: WMPI asks WMPCI if needsWebLayer() which is true if we're using compositing. WMPI only makes a WebLayer if ^ is true, and calls setWebLayer() with it. WMPCI only reports supportsAcceleratedRendering() if it has a WebLayer. Previously, WMPI would change supportsAcceleratedRendering()'s value from true to false if the content has no video in it. Now it just doesn't make a weblayer, so you get the same result. There is a subtle change, but I tested things and they seem to be working - both with and without --disable-accelerated-video. The change is that before getting the video metadata, this function would have reported "true" before even though it did not know yet. Now it reports false. But this seems to do the right thing, as RLCompositor gets the right answer once we have a video layer. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:289 >> + m_videoLayer->setOpaque(m_opaque); > > I think this would be better set on the chromium side of the world - in fact m_opaque doesn't need to exist in the WebKit side at all. but you don't have to do that in the first patch. Agree. It should be easy after this.
Created attachment 194171 [details] Patch Cleaned up decision to create weblayer with ownership of the layer in WMPI
Comment on attachment 194171 [details] Patch Attachment 194171 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17290034
Comment on attachment 194171 [details] Patch Attachment 194171 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17251106
Comment on attachment 194171 [details] Patch Attachment 194171 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17255013
Review for landing moving to https://bugs.webkit.org/show_bug.cgi?id=112971. This CL will be a followup to remove the then-unneeded WebVideo* classes.
Created attachment 194962 [details] Patch
Created attachment 194963 [details] Patch
Latest patch just removes code that won't be used once https://codereview.chromium.org/12902002/ lands/rolls.
Created attachment 195195 [details] Patch Chromium side has rolled into webkit. Updating for EWS and review
Comment on attachment 195195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195195&action=review R+ for awesome, CQ- for doubled changelogs > Source/Platform/ChangeLog:18 > +2013-03-21 Dana Jansens <danakj@chromium.org> double changelog all the way > Source/WebKit/chromium/ChangeLog:17 > +2013-03-21 Dana Jansens <danakj@chromium.org> ditto here
Created attachment 195199 [details] Patch Single changelogs!
Comment on attachment 195199 [details] Patch Clearing flags on attachment: 195199 Committed r146957: <http://trac.webkit.org/changeset/146957>