Remove WebVideoFrame, WebVideoFrameProvider, and WebVideoLayer.

Remove WebVideoFrame, WebVideoFrameProvider, and WebVideoLayer.

These class are no longer used. Removes the #ifdefs guarding the old
unused paths, and removes the WebVideo* files entirely.

R=jamesr
BUG=196810
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190706

[danakj, 2013-03-16 00:30:18]

I don't understand WebMediaPlayerImpl vs WebMediaPlayerMS, so I just duplicated
the changes I made in one into the other..

[scherkus (not reviewing), 2013-03-18 17:17:14]

WebMediaPlayerMS is WebRTC's implementation

MS = MediaStream ... yeah it should probably be renamed :\

[scherkus (not reviewing), 2013-03-18 21:38:24]

I'm not the most familiar w/ this code path anymore, but it's looking good

https://codereview.chromium.org/12902002/diff/5001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_impl.cc:329: bool WebLayerImpl::isOrphan()
const { return !layer_->layer_tree_host(); }
q:
  1) is layer_ guaranteed to be non-null? OR
  2) are we guaranteed clients won't call isOrphan() when layer_ is null? (i.e.,
doing so would be programmer error)

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:220: setStreamTextureClient(NULL);
isn't this handled by the call to SVFPC(NULL)?

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_ms.cc:72: setStreamTextureClient(NULL);
ditto

[danakj, 2013-03-18 21:57:59]

https://codereview.chromium.org/12902002/diff/5001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_impl.cc:329: bool WebLayerImpl::isOrphan()
const { return !layer_->layer_tree_host(); }
On 2013/03/18 21:38:24, scherkus wrote:
> q:
>   1) is layer_ guaranteed to be non-null? OR
>   2) are we guaranteed clients won't call isOrphan() when layer_ is null?
(i.e.,
> doing so would be programmer error)

yes layer_ is not null. it is set to a valid pointer in the constructor.
Layer::Create() always returns a new layer. (see the use of layer_ unguarded in
the current destructor).

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:220: setStreamTextureClient(NULL);
On 2013/03/18 21:38:24, scherkus wrote:
> isn't this handled by the call to SVFPC(NULL)?

Oh, yes! This is the one sketchy thing about this change I feel.

Before, when detroying the WebMediaPlayerClientImpl, it would call
stopUsingProvider on the VFPClient. But would not not set the vfpClient to NULL.
Then it would setStreamTextureClient(NULL).

In between, the compositor thread could be calling didReceiveFrame() which would
call the vfpClient and work fine. Now, we set the vfpClient pointer to NULL and
it would crash.

To fix this I think we should either:
1) Lock inside didReceiveFrame on the provider_lock_ and only call
vfpClient->DidReceiveFrame() if its not null.

or

2) Not set the vfpClient to NULL when shutting down, and explicitly just
vfpClient->StopUsingProvider() and setStreamTextureClient(NULL) here.

[scherkus (not reviewing), 2013-03-18 23:34:18]

https://codereview.chromium.org/12902002/diff/5001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_impl.cc:329: bool WebLayerImpl::isOrphan()
const { return !layer_->layer_tree_host(); }
On 2013/03/18 21:57:59, danakj wrote:
> On 2013/03/18 21:38:24, scherkus wrote:
> > q:
> >   1) is layer_ guaranteed to be non-null? OR
> >   2) are we guaranteed clients won't call isOrphan() when layer_ is null?
> (i.e.,
> > doing so would be programmer error)
> 
> yes layer_ is not null. it is set to a valid pointer in the constructor.
> Layer::Create() always returns a new layer. (see the use of layer_ unguarded
in
> the current destructor).

ah! thanks for the explanation

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:220: setStreamTextureClient(NULL);
On 2013/03/18 21:57:59, danakj wrote:
> On 2013/03/18 21:38:24, scherkus wrote:
> > isn't this handled by the call to SVFPC(NULL)?
> 
> Oh, yes! This is the one sketchy thing about this change I feel.
> 
> Before, when detroying the WebMediaPlayerClientImpl, it would call
> stopUsingProvider on the VFPClient. But would not not set the vfpClient to
NULL.
> Then it would setStreamTextureClient(NULL).
> 
> In between, the compositor thread could be calling didReceiveFrame() which
would
> call the vfpClient and work fine. Now, we set the vfpClient pointer to NULL
and
> it would crash.

I'm a bit confused. An IM/VC convo might be in order :)

Are you saying ~WMPI() can be in the middle of execution (e.g., after setting
video_frame_provider_client_ to NULL) while WMPI::didReceiveFrame() is being
called?

If it's helpful, I believe dRF() is only used in the stream texture case, which
is only present on Android. I believe (hope?) WebMediaPlayerAndroid can
guarantee that it tears down the stream texture machinery so that we're not
racing inside a destructor
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/media/andro...

(note: I just remembered you'll have to fix up the Android WebMediaPlayers as
well)

[danakj, 2013-03-18 23:37:15]

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12902002/diff/5001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:220: setStreamTextureClient(NULL);
On 2013/03/18 23:34:18, scherkus wrote:
> On 2013/03/18 21:57:59, danakj wrote:
> > On 2013/03/18 21:38:24, scherkus wrote:
> > > isn't this handled by the call to SVFPC(NULL)?
> > 
> > Oh, yes! This is the one sketchy thing about this change I feel.
> > 
> > Before, when detroying the WebMediaPlayerClientImpl, it would call
> > stopUsingProvider on the VFPClient. But would not not set the vfpClient to
> NULL.
> > Then it would setStreamTextureClient(NULL).
> > 
> > In between, the compositor thread could be calling didReceiveFrame() which
> would
> > call the vfpClient and work fine. Now, we set the vfpClient pointer to NULL
> and
> > it would crash.
> 
> I'm a bit confused. An IM/VC convo might be in order :)
> 
> Are you saying ~WMPI() can be in the middle of execution (e.g., after setting
> video_frame_provider_client_ to NULL) while WMPI::didReceiveFrame() is being
> called?

Without looking at the android code, it would be possible from the WMPI api
alone. But I guess I should look at the android code then.

> If it's helpful, I believe dRF() is only used in the stream texture case,
which
> is only present on Android. I believe (hope?) WebMediaPlayerAndroid can
> guarantee that it tears down the stream texture machinery so that we're not
> racing inside a destructor
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/media/andro...
> 
> (note: I just remembered you'll have to fix up the Android WebMediaPlayers as
> well)

Oh! Does this mean only the android version should even set up the
streamTextureClient?

[scherkus (not reviewing), 2013-03-19 00:02:01]

> > (note: I just remembered you'll have to fix up the Android WebMediaPlayers
as
> > well)
> 
> Oh! Does this mean only the android version should even set up the
> streamTextureClient?

Believe so!

[danakj, 2013-03-19 00:23:34]

+qinmin

OK that simplifies things lots! This includes changes to the WMPAndroid now.
Also removes the WebStreamTextureClient entirely.

[jamesr, 2013-03-21 00:59:11]

I'm a little worried about this patch moving the destruction of the
media::VideoFrame for WebMediaPlayerMS implementation outside of
WebMediaPlayerMS::current_frame_lock_.  What is the intended purpose of this
lock? It's taken inconsistently.

Otherwise, this looks really great.

https://codereview.chromium.org/12902002/diff/41024/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/12902002/diff/41024/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.h:248:
cc::VideoFrameProvider::Client* video_frame_provider_client_;
Most of the other members in this class seem to have comments and it seems this
one could use it too - maybe document when it'll be null and non-null?

https://codereview.chromium.org/12902002/diff/41024/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/12902002/diff/41024/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:356: cc::VideoFrameProvider::Client*
video_frame_provider_client_;
could use some member comments here - at least indicate that these two fields
are used to support composited video playback since (unlike
WebMediaPlayerAndroid) this class is used for both composited and pure software
video playback

[danakj, 2013-03-21 03:05:16]

On 2013/03/21 00:59:11, jamesr wrote:
> I'm a little worried about this patch moving the destruction of the
> media::VideoFrame for WebMediaPlayerMS implementation outside of
> WebMediaPlayerMS::current_frame_lock_.  What is the intended purpose of this
> lock? It's taken inconsistently.

The lock *appears* to lock |current_frame_used_|, and
|current_frame_->ref_count_|.

But media::VideoFrame is a base::RefCountedThreadSafe<VideoFrame>. So the
protection of the current_frame_'s refcount seems redundant to me.

The current_frame_ pointer itself is not guarded by the lock, and dropping the
last reference from the compositor thread won't affect the value of the
current_frame_ which would already have been replaced on the main thread if that
was the case (comment on the variable states it is only updated on the main
thread).

If the main thread is trying to do something with the data inside
current_frame_, then it would still have a reference to it. So, I think it is
safe, pending some WebMediaPlayerMS expert insight.

[danakj, 2013-03-21 03:05:26]

https://codereview.chromium.org/12902002/diff/41024/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/12902002/diff/41024/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.h:248:
cc::VideoFrameProvider::Client* video_frame_provider_client_;
On 2013/03/21 00:59:11, jamesr wrote:
> Most of the other members in this class seem to have comments and it seems
this
> one could use it too - maybe document when it'll be null and non-null?

Done, also in the other implementations.

https://codereview.chromium.org/12902002/diff/41024/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/12902002/diff/41024/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:356: cc::VideoFrameProvider::Client*
video_frame_provider_client_;
On 2013/03/21 00:59:11, jamesr wrote:
> could use some member comments here - at least indicate that these two fields
> are used to support composited video playback since (unlike
> WebMediaPlayerAndroid) this class is used for both composited and pure
software
> video playback

Done.

[danakj, 2013-03-21 04:23:54]

qinmin@ could you please review the android specific parts of this CL now that
it's looking more final?

If people are happy with it, I'll put up a version that does exactly what this
CL does but with #ifdefs that will get turned on/off by the WebKit side CL.

[scherkus (not reviewing), 2013-03-21 20:38:59]

lgtm++ w/ nits

qinmin should still take a peek at the android / stream texture stuff

https://codereview.chromium.org/12902002/diff/51001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12902002/diff/51001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:426: current__frame_ =
VideoFrame::CreateHoleFrame(
typo (extra _)

https://codereview.chromium.org/12902002/diff/51001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:456: && stream_id_ &&
!needs_external_surface_) {
leading && should be on previous line

[qinmin, 2013-03-21 20:50:36]

lgtm for android related changed 

On 2013/03/21 20:38:59, scherkus wrote:
> lgtm++ w/ nits
> 
> qinmin should still take a peek at the android / stream texture stuff
> 
>
https://codereview.chromium.org/12902002/diff/51001/webkit/media/android/webm...
> File webkit/media/android/webmediaplayer_android.cc (right):
> 
>
https://codereview.chromium.org/12902002/diff/51001/webkit/media/android/webm...
> webkit/media/android/webmediaplayer_android.cc:426: current__frame_ =
> VideoFrame::CreateHoleFrame(
> typo (extra _)
> 
>
https://codereview.chromium.org/12902002/diff/51001/webkit/media/android/webm...
> webkit/media/android/webmediaplayer_android.cc:456: && stream_id_ &&
> !needs_external_surface_) {
> leading && should be on previous line

[no sievers, 2013-03-21 21:43:13]

lgtm

[danakj, 2013-03-21 21:50:47]

Plan is:

cc/ changes are going here: https://codereview.chromium.org/12596015
removing some headers is going here: https://codereview.chromium.org/12895006/
implementing everything new in this patch behind ifdefs is going here:
https://codereview.chromium.org/12676004/

Then this CL rebased will just be removing code.

[scherkus (not reviewing), 2013-03-21 21:55:49]

On 2013/03/21 03:05:16, danakj wrote:
> On 2013/03/21 00:59:11, jamesr wrote:
> > I'm a little worried about this patch moving the destruction of the
> > media::VideoFrame for WebMediaPlayerMS implementation outside of
> > WebMediaPlayerMS::current_frame_lock_.  What is the intended purpose of this
> > lock? It's taken inconsistently.
> 
> The lock *appears* to lock |current_frame_used_|, and
> |current_frame_->ref_count_|.
> 
> But media::VideoFrame is a base::RefCountedThreadSafe<VideoFrame>. So the
> protection of the current_frame_'s refcount seems redundant to me.
> 
> The current_frame_ pointer itself is not guarded by the lock, and dropping the
> last reference from the compositor thread won't affect the value of the
> current_frame_ which would already have been replaced on the main thread if
that
> was the case (comment on the variable states it is only updated on the main
> thread).
> 
> If the main thread is trying to do something with the data inside
> current_frame_, then it would still have a reference to it. So, I think it is
> safe, pending some WebMediaPlayerMS expert insight.

danakj's got it: the lock is more about protecting current_frame_used_ than it
is the bits inside current_frame_ ... but I agree the code could be a little
clearer

wjia: anything else to add about WebMediaPlayerMS and current_frame_used_ /
current_frame_ / current_frame_lock_?

[wjia(left Chromium), 2013-03-21 23:21:26]

On Thu, Mar 21, 2013 at 2:55 PM, <scherkus@chromium.org> wrote:

> On 2013/03/21 03:05:16, danakj wrote:
>
>> On 2013/03/21 00:59:11, jamesr wrote:
>> > I'm a little worried about this patch moving the destruction of the
>> > media::VideoFrame for WebMediaPlayerMS implementation outside of
>> > WebMediaPlayerMS::current_**frame_lock_.  What is the intended purpose
>> of this
>> > lock? It's taken inconsistently.
>>
>
>  The lock *appears* to lock |current_frame_used_|, and
>> |current_frame_->ref_count_|.
>>
>
>  But media::VideoFrame is a base::RefCountedThreadSafe<**VideoFrame>. So
>> the
>> protection of the current_frame_'s refcount seems redundant to me.
>>
>
>  The current_frame_ pointer itself is not guarded by the lock, and
>> dropping the
>> last reference from the compositor thread won't affect the value of the
>> current_frame_ which would already have been replaced on the main thread
>> if
>>
> that
>
>> was the case (comment on the variable states it is only updated on the
>> main
>> thread).
>>
>
>  If the main thread is trying to do something with the data inside
>> current_frame_, then it would still have a reference to it. So, I think
>> it is
>> safe, pending some WebMediaPlayerMS expert insight.
>>
>
> danakj's got it: the lock is more about protecting current_frame_used_
> than it
> is the bits inside current_frame_ ... but I agree the code could be a
> little
> clearer
>
> wjia: anything else to add about WebMediaPlayerMS and current_frame_used_ /
> current_frame_ / current_frame_lock_?
>
>
On lifetime management of current_frame_, the lock_ is not needed in
PutCurrentFrame(). However, the lock_ is indeed used to protect the
integrity of current_frame_ since its timestamp is set in a separate
operation. So it's not redundant if you consider not only current_frame_'s
lifetime, but also its integrity. Without the lock_, the client could get
wrong TS of current_frame_.

https://codereview.chromium.**org/12902002/<https://codereview.chromium.org/1...
>

[danakj, 2013-03-21 23:24:13]

On Thu, Mar 21, 2013 at 7:21 PM, Wei Jia <wjia@chromium.org> wrote:

>
>
>
> On Thu, Mar 21, 2013 at 2:55 PM, <scherkus@chromium.org> wrote:
>
>> On 2013/03/21 03:05:16, danakj wrote:
>>
>>> On 2013/03/21 00:59:11, jamesr wrote:
>>> > I'm a little worried about this patch moving the destruction of the
>>> > media::VideoFrame for WebMediaPlayerMS implementation outside of
>>> > WebMediaPlayerMS::current_**frame_lock_.  What is the intended
>>> purpose of this
>>> > lock? It's taken inconsistently.
>>>
>>
>>  The lock *appears* to lock |current_frame_used_|, and
>>> |current_frame_->ref_count_|.
>>>
>>
>>  But media::VideoFrame is a base::RefCountedThreadSafe<**VideoFrame>. So
>>> the
>>> protection of the current_frame_'s refcount seems redundant to me.
>>>
>>
>>  The current_frame_ pointer itself is not guarded by the lock, and
>>> dropping the
>>> last reference from the compositor thread won't affect the value of the
>>> current_frame_ which would already have been replaced on the main thread
>>> if
>>>
>> that
>>
>>> was the case (comment on the variable states it is only updated on the
>>> main
>>> thread).
>>>
>>
>>  If the main thread is trying to do something with the data inside
>>> current_frame_, then it would still have a reference to it. So, I think
>>> it is
>>> safe, pending some WebMediaPlayerMS expert insight.
>>>
>>
>> danakj's got it: the lock is more about protecting current_frame_used_
>> than it
>> is the bits inside current_frame_ ... but I agree the code could be a
>> little
>> clearer
>>
>> wjia: anything else to add about WebMediaPlayerMS and current_frame_used_
>> /
>> current_frame_ / current_frame_lock_?
>>
>>
> On lifetime management of current_frame_, the lock_ is not needed in
> PutCurrentFrame(). However, the lock_ is indeed used to protect the
> integrity of current_frame_ since its timestamp is set in a separate
> operation. So it's not redundant if you consider not only current_frame_'s
> lifetime, but also its integrity. Without the lock_, the client could get
> wrong TS of current_frame_.
>

Ok thanks! So it sounds like we'll be okay to delete the VideoFrame outside
of PutCurrentFrame, as this will only happen if current_frame_ is already
pointing somewhere else.

[danakj, 2013-03-25 23:28:21]

This CL just deletes code now. James PTAL.

[danakj, 2013-03-25 23:28:50]

oh, sherkus as well for media/ please.

[jamesr, 2013-03-25 23:31:01]

lgtm!!!

[scherkus (not reviewing), 2013-03-26 02:52:01]

lgtm x1000

[commit-bot: I haz the power, 2013-03-26 15:00:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12902002/61001

[commit-bot: I haz the power, 2013-03-26 17:16:04]

Change committed as 190706