Move VideoFrameProvider to, and remove all usage of WebVideoFrame from cc/

Move VideoFrameProvider to, and remove all usage of WebVideoFrame from cc/

BUG=123444
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175413

[Ami GONE FROM CHROMIUM, 2013-01-03 21:59:06]

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
File webkit/media/webvideoframe_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
webkit/media/webvideoframe_impl.h:14: class WebVideoFrameImpl : public
WebKit::WebVideoFrame {
AFAICT at this point this class serves the sole purpose of being able to do a
static_cast<> instead of a C-style cast in WMPI; is that true?

Once this lands, can WebKit::WebVideoFrame be gutted/deleted and replaced with
an opaque pointer?

I'm kinda surprised this works with SW compositing, TBH.  Does webkit painting
really not make any calls to the frame anymore?

[Ami GONE FROM CHROMIUM, 2013-01-03 21:59:54]

> Once this lands, can WebKit::WebVideoFrame be gutted/deleted and replaced with
> an opaque pointer?

lol as I was typing this Dana filed http://webk.it/106037 :)

[sheu, 2013-01-03 22:00:27]

There's a couple files where you've started the Chromium code-stylization of
various cc/ bits, and other files where no changes have been made.  Does that
mean we're starting to do this now?

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_to_ccvideo_frame_provider.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_to_ccvideo_frame_provider.h:10: #include
"third_party/WebKit/Source/Platform/chromium/public/WebVideoFrameProvider.h"
Forward-declaration instead of header?

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_video_layer_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_video_layer_impl.h:10: #include
"webkit/compositor_bindings/web_to_ccvideo_frame_provider.h"
As a forward-declaration instead of a header include maybe?

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
File webkit/media/webvideoframe_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
webkit/media/webvideoframe_impl.h:9: #include "media/base/video_frame.h"
You could just drop the header now and forward-declare

[danakj, 2013-01-03 22:02:59]

+skaslev

[danakj, 2013-01-03 22:44:54]

PTAL

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_to_ccvideo_frame_provider.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_to_ccvideo_frame_provider.h:10: #include
"third_party/WebKit/Source/Platform/chromium/public/WebVideoFrameProvider.h"
On 2013/01/03 22:00:27, sheu wrote:
> Forward-declaration instead of header?

Done.

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_video_layer_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_video_layer_impl.h:10: #include
"webkit/compositor_bindings/web_to_ccvideo_frame_provider.h"
On 2013/01/03 22:00:27, sheu wrote:
> As a forward-declaration instead of a header include maybe?

Done.

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
File webkit/media/webvideoframe_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
webkit/media/webvideoframe_impl.h:9: #include "media/base/video_frame.h"
On 2013/01/03 22:00:27, sheu wrote:
> You could just drop the header now and forward-declare

Done.

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
webkit/media/webvideoframe_impl.h:14: class WebVideoFrameImpl : public
WebKit::WebVideoFrame {
On 2013/01/03 21:59:07, Ami Fischman wrote:
> AFAICT at this point this class serves the sole purpose of being able to do a
> static_cast<> instead of a C-style cast in WMPI; is that true?
> 
> Once this lands, can WebKit::WebVideoFrame be gutted/deleted and replaced with
> an opaque pointer?
> 
> I'm kinda surprised this works with SW compositing, TBH.  Does webkit painting
> really not make any calls to the frame anymore?

Yep, as you said: https://bugs.webkit.org/show_bug.cgi?id=106037

Next step will be to remove WebVideoLayer entirely, and then maybe the other
WebVideo* classes can just go away, I'm not sure.

[Ami GONE FROM CHROMIUM, 2013-01-03 22:49:31]

(FTR, my comments were drive-by)

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
File webkit/media/webvideoframe_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
webkit/media/webvideoframe_impl.h:14: class WebVideoFrameImpl : public
WebKit::WebVideoFrame {
On 2013/01/03 22:44:54, danakj wrote:
> On 2013/01/03 21:59:07, Ami Fischman wrote:
> > AFAICT at this point this class serves the sole purpose of being able to do
a
> > static_cast<> instead of a C-style cast in WMPI; is that true?
> > 
> > Once this lands, can WebKit::WebVideoFrame be gutted/deleted and replaced
with
> > an opaque pointer?
> > 
> > I'm kinda surprised this works with SW compositing, TBH.  Does webkit
painting
> > really not make any calls to the frame anymore?
> 
> Yep, as you said: https://bugs.webkit.org/show_bug.cgi?id=106037
> 
> Next step will be to remove WebVideoLayer entirely, and then maybe the other
> WebVideo* classes can just go away, I'm not sure.

I take it you tested this (gutting WVF) and it works with the meaningful subset
of the cross-product of:
HW/SW decode
accelerated/non-accelerated compositing
threaded/not compositing
canvas2d/webgl grabs of video frames
?

[danakj, 2013-01-03 22:53:57]

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
File webkit/media/webvideoframe_impl.h (right):

https://codereview.chromium.org/11754003/diff/2001/webkit/media/webvideoframe...
webkit/media/webvideoframe_impl.h:14: class WebVideoFrameImpl : public
WebKit::WebVideoFrame {
On 2013/01/03 22:49:32, Ami Fischman wrote:
> On 2013/01/03 22:44:54, danakj wrote:
> > On 2013/01/03 21:59:07, Ami Fischman wrote:
> > > AFAICT at this point this class serves the sole purpose of being able to
do
> a
> > > static_cast<> instead of a C-style cast in WMPI; is that true?
> > > 
> > > Once this lands, can WebKit::WebVideoFrame be gutted/deleted and replaced
> with
> > > an opaque pointer?
> > > 
> > > I'm kinda surprised this works with SW compositing, TBH.  Does webkit
> painting
> > > really not make any calls to the frame anymore?
> > 
> > Yep, as you said: https://bugs.webkit.org/show_bug.cgi?id=106037
> > 
> > Next step will be to remove WebVideoLayer entirely, and then maybe the other
> > WebVideo* classes can just go away, I'm not sure.
> 
> I take it you tested this (gutting WVF) and it works with the meaningful
subset
> of the cross-product of:
> HW/SW decode
> accelerated/non-accelerated compositing
> threaded/not compositing
> canvas2d/webgl grabs of video frames
> ?

I tested with:
- youtube html5 video
- html5doctor.com/demos/video-canvas-magic/demo2.html
- the media test matrix (webm, ogv, mp4)

The avi videos on the test matrix aren't working for me, with or without this
patch.

[jamesr, 2013-01-03 23:35:55]

All of the WebKit::WebVideo* can go away eventually, but it'll require
constructing + owning a cc::VideoLayer in the webkit/media/ implementations of
the WebKit::WebMediaPlayer interface (i.e. webkit/media/webmediaplayer_impl,
webkit/media/android/webmediaplayerandroid_foo) and providing a WebLayer* to the
WebMediaPlayerClient so WebMediaPlayerClientImpl can insert that into the
compositing tree at the right place.

https://codereview.chromium.org/11754003/diff/9001/cc/video_frame_provider.h
File cc/video_frame_provider.h (right):

https://codereview.chromium.org/11754003/diff/9001/cc/video_frame_provider.h#...
cc/video_frame_provider.h:49: // This function releases the lock on the video
frame. It should always be
nit: newline above this to separate the previous function from this comment
block

https://codereview.chromium.org/11754003/diff/9001/cc/video_layer.h
File cc/video_layer.h (right):

https://codereview.chromium.org/11754003/diff/9001/cc/video_layer.h#newcode29
cc/video_layer.h:29: VideoLayer(VideoFrameProvider*);
explicit

https://codereview.chromium.org/11754003/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/surface_texture_transport_client_android.h
(right):

https://codereview.chromium.org/11754003/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/surface_texture_transport_client_android.h:10:
#include "cc/VideoFrameProvider.h"
should be video_frame_provider.h, no?

https://codereview.chromium.org/11754003/diff/9001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_to_ccvideo_frame_provider.cc (right):

https://codereview.chromium.org/11754003/diff/9001/webkit/compositor_bindings...
webkit/compositor_bindings/web_to_ccvideo_frame_provider.cc:38: //
WebToCCVideoFrameProvider::Client implementation.
this is the WebVideoFrameProvider::Client implementation, no?

[Ami GONE FROM CHROMIUM, 2013-01-03 23:42:53]

>
> I tested with:
> - youtube html5 video
> -
html5doctor.com/demos/video-**canvas-magic/demo2.html<http://html5doctor.com/demos/video-canvas-magic/demo2.html>
> - the media test matrix (webm, ogv, mp4)
> The avi videos on the test matrix aren't working for me, with or without
> this patch.
>

AVI not working is expected (we only build relevant demuxers/codecs for
cros, I think), and irrelevant.
To be more explicit, please test the list above works with each of
--enable-threaded-compositing and --disable-threaded-compositing, as well
as with and without --disable-accelerated-video-decode (on win7 (to test HW
decode)) and with/without --disable-accelerated-compositing.

[danakj, 2013-01-04 03:40:59]

On Thu, Jan 3, 2013 at 6:42 PM, Ami Fischman <fischman@chromium.org> wrote:

>  I tested with:
>> - youtube html5 video
>> -
html5doctor.com/demos/video-**canvas-magic/demo2.html<http://html5doctor.com/demos/video-canvas-magic/demo2.html>
>> - the media test matrix (webm, ogv, mp4)
>> The avi videos on the test matrix aren't working for me, with or without
>> this patch.
>>
>
> AVI not working is expected (we only build relevant demuxers/codecs for
> cros, I think), and irrelevant.
> To be more explicit, please test the list above works with each of
> --enable-threaded-compositing and --disable-threaded-compositing, as well
> as with and without --disable-accelerated-video-decode (on win7 (to test HW
> decode)) and with/without --disable-accelerated-compositing.
>

In the absence of a Win7 machine is there something I can do? (Why don't we
have a try bot with tests for this? Or do we?)

Considering this is just moving plumbing, it seems like if any frame
structure arrives at all, the values in the structures shouldn't really
change. I had some problems with null pointers in the adapter, but the
tests I've done so far caught them.

I tried with/out compositing mode and threading for the above tests that I
listed.

[danakj, 2013-01-04 14:01:28]

https://codereview.chromium.org/11754003/diff/9001/cc/video_frame_provider.h
File cc/video_frame_provider.h (right):

https://codereview.chromium.org/11754003/diff/9001/cc/video_frame_provider.h#...
cc/video_frame_provider.h:49: // This function releases the lock on the video
frame. It should always be
On 2013/01/03 23:35:55, jamesr wrote:
> nit: newline above this to separate the previous function from this comment
> block

Done.

https://codereview.chromium.org/11754003/diff/9001/cc/video_layer.h
File cc/video_layer.h (right):

https://codereview.chromium.org/11754003/diff/9001/cc/video_layer.h#newcode29
cc/video_layer.h:29: VideoLayer(VideoFrameProvider*);
On 2013/01/03 23:35:55, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/11754003/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/surface_texture_transport_client_android.h
(right):

https://codereview.chromium.org/11754003/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/surface_texture_transport_client_android.h:10:
#include "cc/VideoFrameProvider.h"
On 2013/01/03 23:35:55, jamesr wrote:
> should be video_frame_provider.h, no?

er.. yeh.

https://codereview.chromium.org/11754003/diff/9001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_to_ccvideo_frame_provider.cc (right):

https://codereview.chromium.org/11754003/diff/9001/webkit/compositor_bindings...
webkit/compositor_bindings/web_to_ccvideo_frame_provider.cc:38: //
WebToCCVideoFrameProvider::Client implementation.
On 2013/01/03 23:35:55, jamesr wrote:
> this is the WebVideoFrameProvider::Client implementation, no?

Done.

[scherkus (not reviewing), 2013-01-04 16:59:35]

lgtm with a single, solitary μ-nit

re: testing hw decode path -- wouldn't a stumpy/lumpy/lucas/etc suffice? (side
note: the automated test coverage here isn't quite as automated as it could be!)

https://codereview.chromium.org/11754003/diff/13017/cc/test/fake_video_frame_...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/11754003/diff/13017/cc/test/fake_video_frame_...
cc/test/fake_video_frame_provider.h:14: class FakeVideoFrameProvider: public
VideoFrameProvider {
nit: spaces around :

[Ami GONE FROM CHROMIUM, 2013-01-04 17:27:11]

On Thu, Jan 3, 2013 at 7:40 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Jan 3, 2013 at 6:42 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>>  I tested with:
>>> - youtube html5 video
>>> -
html5doctor.com/demos/video-**canvas-magic/demo2.html<http://html5doctor.com/demos/video-canvas-magic/demo2.html>
>>> - the media test matrix (webm, ogv, mp4)
>>> The avi videos on the test matrix aren't working for me, with or without
>>> this patch.
>>>
>>
>> AVI not working is expected (we only build relevant demuxers/codecs for
>> cros, I think), and irrelevant.
>> To be more explicit, please test the list above works with each of
>> --enable-threaded-compositing and --disable-threaded-compositing, as well
>> as with and without --disable-accelerated-video-decode (on win7 (to test HW
>> decode)) and with/without --disable-accelerated-compositing.
>>
>
> In the absence of a Win7 machine is there something I can do?
>

You can test on an x86 cros >=lumpy, or on lucas/snow.  Or ask someone else
to patch in your CL to their working cros setup if you don't have one (I
think sheu@ might have an easy-to-go setup; I'd offer mine but I've been
out of the game for months).
Or if you're super-adventurous you can set up vaapi on your linux desktop
and do a chromeos=1 build on that linux desktop and run using VAVDA, but
that's a huge PITA and fraught with yak-shaving peril.

(Why don't we have a try bot with tests for this? Or do we?)
>

There is no bot coverage for HW decode on x86, since x86 platforms only
have h264 HW decoders (>=win7 & >=lumpy) and the bots build chromium not
Chrome, so no h264 for them.
There *is* a bit of coverage for HW-decoded playback on the arm cros
waterfall, but that triggers hours/days after you land a chrome commit,
sadly.

Considering this is just moving plumbing, it seems like if any frame
> structure arrives at all, the values in the structures shouldn't really
> change. I had some problems with null pointers in the adapter, but the
> tests I've done so far caught them.
>

Yeah, my concern is not that you messed up something small that's slipping
by.  My concern is that you're eliding a code-path that's only used by edge
cases, and you're not seeing the edge cases because of the mainstream setup
you have (accelerated compositing, sw decode).

I tried with/out compositing mode and threading for the above tests that I
> listed.
>

Great.

[jamesr1, 2013-01-04 18:00:31]

On Fri, Jan 4, 2013 at 9:27 AM, Ami Fischman <fischman@chromium.org> wrote:

>
>
>
> On Thu, Jan 3, 2013 at 7:40 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Thu, Jan 3, 2013 at 6:42 PM, Ami Fischman <fischman@chromium.org>wrote:
>>
>>>  I tested with:
>>>> - youtube html5 video
>>>> -
html5doctor.com/demos/video-**canvas-magic/demo2.html<http://html5doctor.com/demos/video-canvas-magic/demo2.html>
>>>> - the media test matrix (webm, ogv, mp4)
>>>> The avi videos on the test matrix aren't working for me, with or without
>>>> this patch.
>>>>
>>>
>>> AVI not working is expected (we only build relevant demuxers/codecs for
>>> cros, I think), and irrelevant.
>>> To be more explicit, please test the list above works with each of
>>> --enable-threaded-compositing and --disable-threaded-compositing, as well
>>> as with and without --disable-accelerated-video-decode (on win7 (to test HW
>>> decode)) and with/without --disable-accelerated-compositing.
>>>
>>
>> In the absence of a Win7 machine is there something I can do?
>>
>
> You can test on an x86 cros >=lumpy, or on lucas/snow.  Or ask someone
> else to patch in your CL to their working cros setup if you don't have one
> (I think sheu@ might have an easy-to-go setup; I'd offer mine but I've
> been out of the game for months).
> Or if you're super-adventurous you can set up vaapi on your linux desktop
> and do a chromeos=1 build on that linux desktop and run using VAVDA, but
> that's a huge PITA and fraught with yak-shaving peril.
>
> (Why don't we have a try bot with tests for this? Or do we?)
>>
>
> There is no bot coverage for HW decode on x86, since x86 platforms only
> have h264 HW decoders (>=win7 & >=lumpy) and the bots build chromium not
> Chrome, so no h264 for them.
> There *is* a bit of coverage for HW-decoded playback on the arm cros
> waterfall, but that triggers hours/days after you land a chrome commit,
> sadly.
>
> Considering this is just moving plumbing, it seems like if any frame
>> structure arrives at all, the values in the structures shouldn't really
>> change. I had some problems with null pointers in the adapter, but the
>> tests I've done so far caught them.
>>
>
> Yeah, my concern is not that you messed up something small that's slipping
> by.  My concern is that you're eliding a code-path that's only used by edge
> cases, and you're not seeing the edge cases because of the mainstream setup
> you have (accelerated compositing, sw decode).
>

This seems like an unreasonable burden for a refactor.  The code Dana's
changing is all easily unit testable without relying on physical hardware,
so if there are bits that you think have insufficient test coverage you
should add that and not require that everyone get access to specific
hardware.

- James

>
> I tried with/out compositing mode and threading for the above tests that I
>> listed.
>>
>
> Great.
>

[Ami GONE FROM CHROMIUM, 2013-01-04 18:13:05]

>
> The code Dana's changing is all easily unit testable without relying on
> physical hardware
>

Hey, you're right!  That's because the interactions I was thinking about
are actually in SkCanvasVideoRenderer (which deals in media::VideoFrame)
level, not the WMPI (which deals with WebKit::WebVideoFrame) level, which
is what's being changed here.

Dana: ignore my request to test the canvas2d+HW-decode situation (which
reduces my testing requests to what you've already done).

[danakj, 2013-01-04 18:24:49]

On Fri, Jan 4, 2013 at 1:13 PM, Ami Fischman <fischman@chromium.org> wrote:

>  The code Dana's changing is all easily unit testable without relying on
>> physical hardware
>>
>
> Hey, you're right!  That's because the interactions I was thinking about
> are actually in SkCanvasVideoRenderer (which deals in media::VideoFrame)
> level, not the WMPI (which deals with WebKit::WebVideoFrame) level, which
> is what's being changed here.
>
> Dana: ignore my request to test the canvas2d+HW-decode situation (which
> reduces my testing requests to what you've already done).
>

Ok thanks!

I'll wait for a jamesr review as well.

[jamesr, 2013-01-04 18:39:40]

lgtm and woohoo!

https://codereview.chromium.org/11754003/diff/13017/webkit/compositor_binding...
File webkit/compositor_bindings/web_to_ccvideo_frame_provider.cc (right):

https://codereview.chromium.org/11754003/diff/13017/webkit/compositor_binding...
webkit/compositor_bindings/web_to_ccvideo_frame_provider.cc:34:
ClientAdapter(cc::VideoFrameProvider::Client* cc_client)
explicit

https://codereview.chromium.org/11754003/diff/13017/webkit/compositor_binding...
File webkit/compositor_bindings/web_video_layer_impl.cc (right):

https://codereview.chromium.org/11754003/diff/13017/webkit/compositor_binding...
webkit/compositor_bindings/web_video_layer_impl.cc:17: , m_layer(new
WebLayerImpl(cc::VideoLayer::create(m_providerAdapter.get())))
chromium style is to put this comma at the end of the previous line

[danakj, 2013-01-04 19:47:34]

https://codereview.chromium.org/11754003/diff/13017/webkit/compositor_binding...
File webkit/compositor_bindings/web_video_layer_impl.cc (right):

https://codereview.chromium.org/11754003/diff/13017/webkit/compositor_binding...
webkit/compositor_bindings/web_video_layer_impl.cc:17: , m_layer(new
WebLayerImpl(cc::VideoLayer::create(m_providerAdapter.get())))
On 2013/01/04 18:39:40, jamesr wrote:
> chromium style is to put this comma at the end of the previous line

None of this file is chromium style yet :/

[commit-bot: I haz the power, 2013-01-04 19:48:42]

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

[commit-bot: I haz the power, 2013-01-04 19:49:03]

Presubmit check for 11754003-20001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host

Presubmit checks took 2.3s to calculate.

[danakj, 2013-01-04 19:50:30]

+aelias for content/

[aelias_OOO_until_Jul13, 2013-01-04 20:19:07]

lgtm for content/.../*android*

[commit-bot: I haz the power, 2013-01-04 22:57:55]

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

[commit-bot: I haz the power, 2013-01-05 00:23:22]

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

[commit-bot: I haz the power, 2013-01-05 02:12:34]

Retried try job too often on linux_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-07 19:57:27]

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

[commit-bot: I haz the power, 2013-01-07 22:17:58]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-07 23:03:05]

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

[commit-bot: I haz the power, 2013-01-07 23:19:20]

Change committed as 175413