Added support for YUV videos to the software compositor.

Added support for YUV videos to the software compositor.

Those Layout Tests pass with this patch (minus filtering differences):

platform/chromium/virtual/softwarecompositing/geometry/video-fixed-scrolling.html
platform/chromium/virtual/softwarecompositing/geometry/video-opacity-overlay.html
platform/chromium/virtual/softwarecompositing/layers-inside-overflow-scroll.html
platform/chromium/virtual/softwarecompositing/overflow/scroll-ancestor-update.html
platform/chromium/virtual/softwarecompositing/self-painting-layers.html
platform/chromium/virtual/softwarecompositing/reflections/load-video-in-reflection.html
platform/chromium/virtual/softwarecompositing/video-page-visibility.html                                                                                                                                            
platform/chromium/virtual/softwarecompositing/visibility/visibility-simple-video-layer.html



BUG=150016
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164871

[slavi, 2012-10-24 17:51:09]

The patch fails on the try bots currently because it depends on
http://src.chromium.org/viewvc/chrome?view=rev&revision=163829 and the LKGR is
still 163826.

I'll retry later.

[enne (OOO), 2012-10-24 17:56:48]

You can do: git cl try --revision 163829

[danakj, 2012-10-24 18:01:07]

https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
File cc/video_frame_draw_quad.h (right):

https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
cc/video_frame_draw_quad.h:18: class VideoFrameDrawQuad : public DrawQuad {
Any chance of reusing the YUVVideoDrawQuad?

https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
cc/video_frame_draw_quad.h:22: media::VideoFrame* frame() const { return frame_;
}
No pointers in quads please. These need to be serializable.

[slavi, 2012-10-24 18:06:47]

On 2012/10/24 18:01:07, danakj wrote:
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> File cc/video_frame_draw_quad.h (right):
> 
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> cc/video_frame_draw_quad.h:18: class VideoFrameDrawQuad : public DrawQuad {
> Any chance of reusing the YUVVideoDrawQuad?
> 
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> cc/video_frame_draw_quad.h:22: media::VideoFrame* frame() const { return
frame_;
> }
> No pointers in quads please. These need to be serializable.

Part of the reason for VideoFrameDrawQuad and not YUVVideoDrawQuad is to avoid a
memcpy when rendering video. I wanted to transport the media::VideoFrame itself
directly to the software renderer, not a copy of it.

Maybe we should have a discussion offline how to serialize media::VideoFrame-s?

[danakj, 2012-10-24 18:08:08]

On 2012/10/24 18:06:47, slavi wrote:
> On 2012/10/24 18:01:07, danakj wrote:
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > File cc/video_frame_draw_quad.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > cc/video_frame_draw_quad.h:18: class VideoFrameDrawQuad : public DrawQuad {
> > Any chance of reusing the YUVVideoDrawQuad?
> > 
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > cc/video_frame_draw_quad.h:22: media::VideoFrame* frame() const { return
> frame_;
> > }
> > No pointers in quads please. These need to be serializable.
> 
> Part of the reason for VideoFrameDrawQuad and not YUVVideoDrawQuad is to avoid
a
> memcpy when rendering video. I wanted to transport the media::VideoFrame
itself
> directly to the software renderer, not a copy of it.
> 
> Maybe we should have a discussion offline how to serialize
media::VideoFrame-s?

Sure, can you use a ResourceId from ResourceProvider? We should be using that
for all bitmaps (software) and textures (hardware) to allow us to pass them
across the wire.

[slavi, 2012-10-24 18:14:50]

On 2012/10/24 18:08:08, danakj wrote:
> On 2012/10/24 18:06:47, slavi wrote:
> > On 2012/10/24 18:01:07, danakj wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > > File cc/video_frame_draw_quad.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > > cc/video_frame_draw_quad.h:18: class VideoFrameDrawQuad : public DrawQuad
{
> > > Any chance of reusing the YUVVideoDrawQuad?
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > > cc/video_frame_draw_quad.h:22: media::VideoFrame* frame() const { return
> > frame_;
> > > }
> > > No pointers in quads please. These need to be serializable.
> > 
> > Part of the reason for VideoFrameDrawQuad and not YUVVideoDrawQuad is to
avoid
> a
> > memcpy when rendering video. I wanted to transport the media::VideoFrame
> itself
> > directly to the software renderer, not a copy of it.
> > 
> > Maybe we should have a discussion offline how to serialize
> media::VideoFrame-s?
> 
> Sure, can you use a ResourceId from ResourceProvider? We should be using that
> for all bitmaps (software) and textures (hardware) to allow us to pass them
> across the wire.

I can and I did at some point. But that requires an additional memcpy:

 YUV (media::VideoFrame) -> RGB SkBitmap (or three one channel SkBitmaps for Y,
U, V) -> SkCanvas. 

Now there's a fast path from

 YUV (media::VideoFrame) -> SkCanvas

The memcpy is necessary for the GL compositor since it needs the data down on
the GPU. I want to avoid that in the software compositor.

[danakj, 2012-10-24 18:20:27]

On 2012/10/24 18:14:50, slavi wrote:
> On 2012/10/24 18:08:08, danakj wrote:
> > On 2012/10/24 18:06:47, slavi wrote:
> > > On 2012/10/24 18:01:07, danakj wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > > > File cc/video_frame_draw_quad.h (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > > > cc/video_frame_draw_quad.h:18: class VideoFrameDrawQuad : public
DrawQuad
> {
> > > > Any chance of reusing the YUVVideoDrawQuad?
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw...
> > > > cc/video_frame_draw_quad.h:22: media::VideoFrame* frame() const { return
> > > frame_;
> > > > }
> > > > No pointers in quads please. These need to be serializable.
> > > 
> > > Part of the reason for VideoFrameDrawQuad and not YUVVideoDrawQuad is to
> avoid
> > a
> > > memcpy when rendering video. I wanted to transport the media::VideoFrame
> > itself
> > > directly to the software renderer, not a copy of it.
> > > 
> > > Maybe we should have a discussion offline how to serialize
> > media::VideoFrame-s?
> > 
> > Sure, can you use a ResourceId from ResourceProvider? We should be using
that
> > for all bitmaps (software) and textures (hardware) to allow us to pass them
> > across the wire.
> 
> I can and I did at some point. But that requires an additional memcpy:
> 
>  YUV (media::VideoFrame) -> RGB SkBitmap (or three one channel SkBitmaps for
Y,
> U, V) -> SkCanvas. 
> 
> Now there's a fast path from
> 
>  YUV (media::VideoFrame) -> SkCanvas
> 
> The memcpy is necessary for the GL compositor since it needs the data down on
> the GPU. I want to avoid that in the software compositor.

I think in order to avoid that the media code will have to draw into something
that the ResourceProvider can turn into a ResourceId without a copy - ie shared
memory. Without that we can't do zero-copy.

ResourceProvider has methods to take external textures and turn them into
ResourceIds (createResourceFromExternalTexture). We should make use of a similar
software-mode method for this scenario.

[slavi, 2012-10-24 19:56:41]

> I think in order to avoid that the media code will have to draw into something
> that the ResourceProvider can turn into a ResourceId without a copy - ie
shared
> memory. Without that we can't do zero-copy.
> 
> ResourceProvider has methods to take external textures and turn them into
> ResourceIds (createResourceFromExternalTexture). We should make use of a
similar
> software-mode method for this scenario.

What do you think of adding VideoFrame resource type and
ResourceProvider::createFromVideoFrame? Then VideoFrameDrawQuad can be kept POD
(or maybe we can reuse one of the already existing DrawQuads) and thus wire
transferable.

Then I can tackle in a separate CL the ubercompositor scenario when the
media::VideoFrame is using a shared mem as a backing store, VideoFrames creation
and lifetime.

[danakj, 2012-10-24 20:08:29]

On 2012/10/24 19:56:41, slavi wrote:
> What do you think of adding VideoFrame resource type and
> ResourceProvider::createFromVideoFrame? Then VideoFrameDrawQuad can be kept
POD
> (or maybe we can reuse one of the already existing DrawQuads) and thus wire
> transferable.
> 
> Then I can tackle in a separate CL the ubercompositor scenario when the
> media::VideoFrame is using a shared mem as a backing store, VideoFrames
creation
> and lifetime.

@piman what do you think?

[piman, 2012-10-24 22:48:41]

+aelias.
At a high level, being able to create a resource from existing data makes sense,
but I'm very doubtful that the solution here is to have a third type of resource
as opposed to a different format or something. E.g. can't we make this use 3
single-channel resources like in the GL+software decode case?

[slavi, 2012-10-24 23:04:05]

On 2012/10/24 22:48:41, piman wrote:
> +aelias.
> At a high level, being able to create a resource from existing data makes
sense,
> but I'm very doubtful that the solution here is to have a third type of
resource
> as opposed to a different format or something. E.g. can't we make this use 3
> single-channel resources like in the GL+software decode case?

Yes we can, I did that in an earlier prototype of the code. But that's an extra
copy that I'm trying to avoid. Namely,

 YUV (media::VideoFrame) -> RGB SkBitmap (or three one channel SkBitmaps for Y,
U, V) -> SkCanvas. 

is what you and Dana suggested, and I'm trying to do

 YUV (media::VideoFrame) -> SkCanvas

The GL codepath has to do that extra copy to upload the data to the GPU. I'm
hoping to avoid that for the software path.

Copying follows the current design and is trivial to implement. Making it zero
copy goes against the grain and will add some complexity.

[piman, 2012-10-24 23:06:03]

On Wed, Oct 24, 2012 at 4:04 PM, <skaslev@chromium.org> wrote:

> On 2012/10/24 22:48:41, piman wrote:
>
>> +aelias.
>> At a high level, being able to create a resource from existing data makes
>>
> sense,
>
>> but I'm very doubtful that the solution here is to have a third type of
>>
> resource
>
>> as opposed to a different format or something. E.g. can't we make this
>> use 3
>> single-channel resources like in the GL+software decode case?
>>
>
> Yes we can, I did that in an earlier prototype of the code. But that's an
> extra
> copy that I'm trying to avoid. Namely,
>
>
>  YUV (media::VideoFrame) -> RGB SkBitmap (or three one channel SkBitmaps
> for Y,
> U, V) -> SkCanvas.
>

Why can't we have the SkBitmaps take ownership of the VideoFrame buffers,
instead of copying?

Antoine


>
> is what you and Dana suggested, and I'm trying to do
>
>
>  YUV (media::VideoFrame) -> SkCanvas
>
> The GL codepath has to do that extra copy to upload the data to the GPU.
> I'm
> hoping to avoid that for the software path.
>
> Copying follows the current design and is trivial to implement. Making it
> zero
> copy goes against the grain and will add some complexity.
>
>
http://codereview.chromium.**org/11274017/<http://codereview.chromium.org/112...
>

[aelias_OOO_until_Jul13, 2012-10-24 23:10:45]

I agree we should try to keep resource types to a minimum.  The resource
provider mainly is there to manage lifetimes and cross-process transfer, and for
that it doesn't matter whether data is YUV or not.

We could add another format and make the software read/write locks assert that
the resource is in RGBA8 format.

[Ami GONE FROM CHROMIUM, 2012-10-24 23:25:54]

FYI

On Wed, Oct 24, 2012 at 4:05 PM, Antoine Labour <piman@chromium.org> wrote:

> Why can't we have the SkBitmaps take ownership of the VideoFrame buffers,
> instead of copying?
>

VideoFrame isn't prepared to relinquish its buffers, at least right now.
(VF's get reused as paints can come in more frequently/faster than frames
are decoded, and so on)

[danakj, 2012-10-24 23:29:34]

On Wed, Oct 24, 2012 at 7:25 PM, Ami Fischman <fischman@chromium.org> wrote:
> FYI
>
> On Wed, Oct 24, 2012 at 4:05 PM, Antoine Labour <piman@chromium.org> wrote:
>>
>> Why can't we have the SkBitmaps take ownership of the VideoFrame buffers,
>> instead of copying?
>
>
> VideoFrame isn't prepared to relinquish its buffers, at least right now.
> (VF's get reused as paints can come in more frequently/faster than frames
> are decoded, and so on)

I don't think we can get around a copy right now then. These have to
be able to live in another process and be drawn from for multiple
frames there until a new frame arrives.

Ideally, I think, the provider would decode into shared memory, and
whenever the current frame was to be shipped off to the compositor, it
would start decoding into a new shared memory buffer (reusing them as
released from the compositor).

[slavi, 2012-10-24 23:32:28]

> Why can't we have the SkBitmaps take ownership of the VideoFrame buffers,
> instead of copying?

I'm not sure if that's possible. VideoFrames are ref counted and I imagine that
the decoder might reuse them. 

The interface WebVideoFrameProvider has is 

virtual WebVideoFrame* getCurrentFrame()
virtual void putCurrentFrame(WebVideoFrame*)     // WebVideoFrame is just a
media::VideoFrame wrapper

with no guarantee if the frame will be deleted or recycled after
putCurrentFrame.



Since the majority of the reviewers are mindful of the complexity would come by
making video zero-copy in software, I suggest reverting to my prototype
implementation where  media::VideoFrame -> RGB SkBitmap -> SkCanvas. And
tackling how the performance can be improved in a later CL.

[danakj, 2012-10-24 23:33:41]

On Wed, Oct 24, 2012 at 7:29 PM, Dana Jansens <danakj@chromium.org> wrote:
> On Wed, Oct 24, 2012 at 7:25 PM, Ami Fischman <fischman@chromium.org> wrote:
>> FYI
>>
>> On Wed, Oct 24, 2012 at 4:05 PM, Antoine Labour <piman@chromium.org> wrote:
>>>
>>> Why can't we have the SkBitmaps take ownership of the VideoFrame buffers,
>>> instead of copying?
>>
>>
>> VideoFrame isn't prepared to relinquish its buffers, at least right now.
>> (VF's get reused as paints can come in more frequently/faster than frames
>> are decoded, and so on)
>
> I don't think we can get around a copy right now then. These have to
> be able to live in another process and be drawn from for multiple
> frames there until a new frame arrives.
>
> Ideally, I think, the provider would decode into shared memory, and
> whenever the current frame was to be shipped off to the compositor, it
> would start decoding into a new shared memory buffer (reusing them as
> released from the compositor).

Also, we're going to need a similar system to avoid a copy for the
hardware decode path, the current video methods aren't compatible with
ubercomp. So if you're using that as a base point, it's a sadly busted
one we're going to have to change soon :( The WebVideoFrameProvider
interface is going to have to change for ubercomp.

[slavi, 2012-10-25 20:27:28]

PTAL

I moved the YUV -> RGB conversion to VideoLayerImpl and reuse TextureDrawQuad to
transfer the data to the software compositor.

Since it involves an extra copy this is suboptimal. I'll follow up on this issue
when we start addressing how to do zero-copy video in the ubercompositor
setting. As Dana said, it seems that this will be an issue also for the GL
compositor.

[slavi, 2012-10-26 16:51:35]

Added Ami Fischman for review of webkit/media changes.

[Ami GONE FROM CHROMIUM, 2012-10-26 22:45:09]

Only looked at webkit/media

http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
File webkit/media/webvideoframe_impl.h (right):

http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:18: WebVideoFrame* web_video_frame);
optional: Personally I find the previous version more visually useful (as it
matches the comment).

http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:36: inline media::VideoFrame*
WebVideoFrameImpl::toVideoFrame(
Why this thunk?

[slavi, 2012-10-26 23:46:23]

http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
File webkit/media/webvideoframe_impl.h (right):

http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:18: WebVideoFrame* web_video_frame);
On 2012/10/26 22:45:09, Ami Fischman wrote:
> optional: Personally I find the previous version more visually useful (as it
> matches the comment).

Done.

http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:36: inline media::VideoFrame*
WebVideoFrameImpl::toVideoFrame(
On 2012/10/26 22:45:09, Ami Fischman wrote:
> Why this thunk?

Ok, this is subtle.

The problem is that I want to be able to unwrap WebKit::WebVideoFrame to a
concrete media::VideoFrame in the cc/ module. I can't do this by calling
WebVideoFrameImpl::toVideoFrame directly since WebVideoFrameImpl lives in
webkit/ on which cc/ cannot depend. 

So, I'm passing it a callback from
webkit/compositor_bindings/web_video_layer_impl.cc down to cc/. 

That almost works but it breaks shared lib builds because
WebVideoFrameImpl::toVideoFrame is not marked as EXPORTed and cc/ is trying to
call it from another shared lib.

One way it fix it is to mark WebVideoFrameImpl::toVideoFrame as EXPORTed but I
didn't do it because webkit/ doesn't export anything besides chromium WebKit API
interfaces.

Or, you can just mark it as |inline| and then I get a copy of it in cc/. Subtle
but elegant and works on all platforms and compilers.

[jamesr, 2012-10-26 23:50:25]

On 2012/10/26 23:46:23, slavi wrote:
>
http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
> File webkit/media/webvideoframe_impl.h (right):
> 
>
http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
> webkit/media/webvideoframe_impl.h:18: WebVideoFrame* web_video_frame);
> On 2012/10/26 22:45:09, Ami Fischman wrote:
> > optional: Personally I find the previous version more visually useful (as it
> > matches the comment).
> 
> Done.
> 
>
http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_...
> webkit/media/webvideoframe_impl.h:36: inline media::VideoFrame*
> WebVideoFrameImpl::toVideoFrame(
> On 2012/10/26 22:45:09, Ami Fischman wrote:
> > Why this thunk?
> 
> Ok, this is subtle.
> 
> The problem is that I want to be able to unwrap WebKit::WebVideoFrame to a
> concrete media::VideoFrame in the cc/ module. I can't do this by calling
> WebVideoFrameImpl::toVideoFrame directly since WebVideoFrameImpl lives in
> webkit/ on which cc/ cannot depend. 

Why can't you downcast from somewhere inside of webkit/compositor_bindings/,
perhaps by wrapping the WebVideoFrameProvider in an adapter class there and
having the cc::VideoLayerImpl class only ever see media::VideoFrame*s?

[slavi, 2012-10-26 23:57:13]

> Why can't you downcast from somewhere inside of webkit/compositor_bindings/,
> perhaps by wrapping the WebVideoFrameProvider in an adapter class there and
> having the cc::VideoLayerImpl class only ever see media::VideoFrame*s?

I actually did that first (as I told you offline). But it ended up way more
complicated than it looks at first.

First you need an interface, that lives in cc/ConcreteVideoProvider.

Then you implement it in webkit/compositor_bindings/ConcreteVideoProvider.h[cc]
and then you wrap WebVideoProvider inside of ConcreteVideoProvider instance in
webkit/compositor_bindings/web_video_layer_impl.cc.

Compared to just passing a function to do the unwrapping, it's way more
complicated and more code sloc.

[slavi, 2012-10-26 23:58:44]

On 2012/10/26 23:57:13, slavi wrote:
> > Why can't you downcast from somewhere inside of webkit/compositor_bindings/,
> > perhaps by wrapping the WebVideoFrameProvider in an adapter class there and
> > having the cc::VideoLayerImpl class only ever see media::VideoFrame*s?
> 
> I actually did that first (as I told you offline). But it ended up way more
> complicated than it looks at first.
> 
> First you need an interface, that lives in cc/ConcreteVideoProvider.
> 
> Then you implement it in
webkit/compositor_bindings/ConcreteVideoProvider.h[cc]
> and then you wrap WebVideoProvider inside of ConcreteVideoProvider instance in
> webkit/compositor_bindings/web_video_layer_impl.cc.
> 
> Compared to just passing a function to do the unwrapping, it's way more
> complicated and more code sloc.

And you still need WebVideoFrameImpl::toVideoFrame inline or exported in order
to make it to work for shared lib builds.

[Ami GONE FROM CHROMIUM, 2012-10-27 03:46:01]

I don't think it's ok to avoid componentization by copy/pasting (which is
what you're having the preprocessor do for you here).  |inline| is a red
herring here.
I think EXPORTing is a fine solution, since it semantically maps to that
which you're trying to achieve.

I'm actually surprised that base::Bind() isn't saving you here.
ajwong: do you have thoughts on making Bind be able to curry up local
symbols to be passed between dynamic libs?

[skaslev, 2012-10-27 04:27:39]

The reason base::Bind doesn't save me is because everything in
webkit/compositor_bindings (where base::Bind is called) lives in a
different shared lib from webkit/media (where toVideoFrame is actually
defined).

Adding EXPORT will need some gyp massaging and an extra header to
webkit/media for the definition of WEBKIT_MEDIA_EXPORT. I wanted to avoid
that because WebVideoFrame is becoming an useless media::VideoFrame wrapper
that goes from Chrome to WebKit and back again when it gets unwrapped. In
fact, none of WebVideoFrame interface and WebVideoFrameImpl (besides
toVideoFrame) is used anymore. I was planning to find a way to kill it in
the near future.


On Fri, Oct 26, 2012 at 8:45 PM, Ami Fischman <fischman@chromium.org> wrote:

> I don't think it's ok to avoid componentization by copy/pasting (which is
> what you're having the preprocessor do for you here).  |inline| is a red
> herring here.
> I think EXPORTing is a fine solution, since it semantically maps to that
> which you're trying to achieve.
>
> I'm actually surprised that base::Bind() isn't saving you here.
> ajwong: do you have thoughts on making Bind be able to curry up local
> symbols to be passed between dynamic libs?
>

[slavi, 2012-10-27 07:14:10]

> > I don't think it's ok to avoid componentization by copy/pasting (which is
> > what you're having the preprocessor do for you here).  |inline| is a red
> > herring here.
> > I think EXPORTing is a fine solution, since it semantically maps to that
> > which you're trying to achieve.

@fischman

After some more thought, EXPORT doesn't feel right. 

First, having WebVideoFrameImpl::toVideoFrame as the only exported function out
of webkit/media smells fishy.

Second, the core of the problem is that cc/ cannot depend on webkit/. By
EXPORT-ing, cc/ is still depending on webkit/media, namely, it's using it's only
EXPORTed function from the shared lib.

I do actually want a copy of the function to break the dependency, hence inline.

I suggest adding a comment explaining why the function needs to be inline.

Do you still feel strongly about EXPORT?

[Ami GONE FROM CHROMIUM, 2012-10-28 04:23:08]

If you can kill WebVideoFrame in the near future, and add a TODO explaining
that the inline is a temporary measure pending deletion of WVF then LGTM.

[slavi, 2012-10-29 16:53:31]

On 2012/10/28 04:23:08, Ami Fischman wrote:
> If you can kill WebVideoFrame in the near future, and add a TODO explaining
> that the inline is a temporary measure pending deletion of WVF then LGTM.

Done. PTAL.

[Ami GONE FROM CHROMIUM, 2012-10-29 16:57:25]

http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_...
File webkit/media/webvideoframe_impl.h (right):

http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:37: // WebKit::WebVideoFrame and
WebVideoFrameImpl which are currently unused.
I'm not sure why you say they are unused.
They are used by WebVideoFrameProvider which is used by
WebMediaPlayerClientImpl.
Is your plan to GTFO WMPCI out of webkit and into chromium to avoid the need for
WVF?

[Ami GONE FROM CHROMIUM, 2012-10-29 17:10:17]

http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_...
File webkit/media/webvideoframe_impl.h (right):

http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:37: // WebKit::WebVideoFrame and
WebVideoFrameImpl which are currently unused.
On 2012/10/29 16:57:25, Ami Fischman wrote:
> I'm not sure why you say they are unused.
> They are used by WebVideoFrameProvider which is used by
> WebMediaPlayerClientImpl.
> Is your plan to GTFO WMPCI out of webkit and into chromium to avoid the need
for
> WVF?

Clarified in IM that "currently unused" refers to WVFI's non-static methods: as
of this CL nobody uses them anymore and then will be deleted in a follow-up CL.

LGTM++

[jamesr, 2012-10-29 21:32:08]

The goal here for cc is to not depend on any WebKit headers, types, libraries,
or anything.  Getting there means that cc::VideoLayerImpl is going to have to be
implemented on top of media:: types only.  Looking at WebMediaPlayerClientImpl,
it's only using WebKit types to set up the layer<->provider association but
otherwise stays pretty much out of the way.  I think the most direct migration
path forward will look something like:

1.) Code up cc:: interface like VideoFrameProvider that provides
media::VideoFrame instead of WebVideoFrames, port cc::VideoLayerImpl to use that
2.) Implement this cc::VideoFrameProvider interface on top of a
WebVideoFrameProvider by unwrapping the WebVideoFrame inside of
webkit/compositor_bindings
3.) Switch around the provider pointer setup in WebMediaPlayerClientImpl to
instead ask the chromium side (webkit/media/webmediaplayer_impl.cc) to give a
provider pointer to cc directly that doesn't go through the WebVideoFrame
interface
4.) Remove WebVideoFrame wrapper in webkit/compositor_bindings/, remove
WebVideoLayer completely

This patch kind of moves a bit sideways - it doesn't really get us closer to the
end goal.  I think if you were to just go ahead and implement (1) it'd be more
typing and more types in the short term, but you wouldn't need this odd
unwrapper closure and it'd be moving us closer to the end state.  That said,
this does enable new functionality.

LGTM if you want to go with this way and clean it up later, but I think in the
end you'll find it less work overall to just write the wrappers for the full
provider interface.

[commit-bot: I haz the power, 2012-10-30 04:17:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11274017/41004

[commit-bot: I haz the power, 2012-10-30 06:45:51]

Change committed as 164871