Bug 112483 – [chromium] Remove the WebVideoFrame, WebVideoFrameProvider, WebStreamTextureClient, and WebVideoLayer classes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[wip] [chromium] Remove the WebVideoFrame, WebVideoFrameProvider, and WebVideoLayer classes.

[Dana Jansens]

Created attachment 193405 [details]
Patch

[Dana Jansens]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[James Robinson]

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.

[Dana Jansens]

Created attachment 193625 [details]
Patch

[Dana Jansens]

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*

[Dana Jansens]

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?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Dana Jansens]

Created attachment 193633 [details]
Patch

isOrphan => True if the layer is not part of a tree attached to a WebLayerTreeView.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Andrew Scherkus]

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)

[Dana Jansens]

Created attachment 193701 [details]
Patch

Remove WebStreamTextureClient

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Min Qin]

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

[Dana Jansens]

(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.

[Dana Jansens]

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.

[James Robinson]

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)

[Dana Jansens]

Created attachment 194137 [details]
Patch

moved ownership

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[James Robinson]

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.

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 194171 [details]
Patch

Cleaned up decision to create weblayer with ownership of the layer in WMPI

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 194962 [details]
Patch

[Dana Jansens]

Created attachment 194963 [details]
Patch

[Dana Jansens]

Latest patch just removes code that won't be used once https://codereview.chromium.org/12902002/ lands/rolls.

[Dana Jansens]

Created attachment 195195 [details]
Patch

Chromium side has rolled into webkit. Updating for EWS and review

[James Robinson]

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
> [phone]  Dana Jansens  <[e-mail]>

double changelog all the way

> Source/WebKit/chromium/ChangeLog:17
> [phone]  Dana Jansens  <[e-mail]>

ditto here

[Dana Jansens]

Created attachment 195199 [details]
Patch

Single changelogs!

[WebKit Review Bot]

Comment on attachment 195199 [details]
Patch

Clearing flags on attachment: 195199

Committed r146957: <http://trac.webkit.org/changeset/146957>