media: Add MediaOzonePlatform support

media: Add MediaOzonePlatform support

This CL adds MediaOzonePlatform, a new Ozone component for abstracting media
and related. In particular it adds CreateVideoDecodeAccelerator method so Ozone
implementations can use that for abstracting video decoding acceleration (i.e.
GPU based video decoding). Eventually MediaOzonePlatform will be used as well
for many windowing systems abstractions related to media, e.g. video encode,
audio, etc. We will add support for these others as needed.

Media platform is called and created on demand whenever a video playback is set
to play. Different than ui::OzonePlatform methods, the one added in here is 
non-pure virtual so the implementations can decide themselves whether a video 
implementation is actually wanted, based on its hardware capabilities.

This work is aimed at internal implementations like Ozone-DRI, Ozone-test but
also for externals like Ozone-Wayland. Different targets like Chromium Browser,
Chrome OS, ChromeCast and others can now take advantage of it.

BUG=380884
TEST=manually on Ozone-Wayland, using:

 $ export GYP_DEFINES='chromeos=0 use_ozone=1 proprietary_codecs=1 ffmpeg_branding=Chrome'
 $ ninja -j16 -C out/Release content

and to run:

 ~/git/chromium/src/out/Debug$ ./chrome --no-sandbox --user-data-dir=/tmp/chrome --ignore-gpu-blacklist ../../third_party/WebKit/PerformanceTests/resources/bear-1280x720.mp4  --vmodule=*/ozone/ui*=3 --v=0

additionally, in Release builds you can start chrome with --disable-accelerated-video-decode to compare the results.

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

[vignatti (out of this project), 2014-05-05 22:26:54]

rjkroege@ this is what I had on my mind for the Ozone media abstraction... I
think at this point my biggest concern is with the content layer being added to
ozone stuff. I'm not sure, but I don't think we want anything like that?

[vignatti (out of this project), 2014-05-05 22:35:14]

+fischman@, +scherkus@ can you please take a look on the media side? I think
eventually we'll want to Ozone-abstract other media components as well, but for
now we can start with video decoding which specially is what our Ozone
implementation cares the most.

[Ami GONE FROM CHROMIUM, 2014-05-05 22:40:42]

AFAICT content/common/gpu/media/ asks ui/ozone for a factory which is satisfied
using one defined in content/common/gpu/media/, which sounds like a circular
dependency to me.

Given that the factory lives in c/c/g/m/ why go through ui/ozone at all?

[Ami GONE FROM CHROMIUM, 2014-05-05 22:43:00]

BTW what's the relationship between this and
https://codereview.chromium.org/240113009/ ?


On Mon, May 5, 2014 at 3:40 PM, <fischman@chromium.org> wrote:

> AFAICT content/common/gpu/media/ asks ui/ozone for a factory which is
> satisfied
> using one defined in content/common/gpu/media/, which sounds like a
> circular
> dependency to me.
>
> Given that the factory lives in c/c/g/m/ why go through ui/ozone at all?
>
>
> https://codereview.chromium.org/269673005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vignatti (out of this project), 2014-05-05 23:03:02]

On 2014/05/05 22:40:42, Ami Fischman wrote:
> AFAICT content/common/gpu/media/ asks ui/ozone for a factory which is
satisfied
> using one defined in content/common/gpu/media/, which sounds like a circular
> dependency to me.
> 
> Given that the factory lives in c/c/g/m/ why go through ui/ozone at all?

because in ui/ozone it's where happens all the Ozone platforms initialization
(check ui/ozone/ozone_platform.cc)... I agree, probably it's not the optimal
place to be.

[vignatti (out of this project), 2014-05-05 23:07:54]

On 2014/05/05 22:43:00, Ami Fischman wrote:
> BTW what's the relationship between this and
> https://codereview.chromium.org/240113009/ ?

my CL avoids exactly the exposition of the underlying system details provided on
240113009. I'm actually working now with qing.zhang@ and educating him about
Ozone and its benefits. In any case, I put some description on my CL that might
help you to understand a bit more the motivation of this work.

[Ami GONE FROM CHROMIUM, 2014-05-05 23:29:24]

perhaps the root of my confusion is being unclear about how concrete you intend
content::VideoDecodeFactoryOzone to ever get.
Is it your intention that each Ozone platform provide a subclass(?) of
content::VideoDecodeFactoryOzone?  Do you intend to check in any such subclasses
into chromium?  Where do you plan them to live?

[vignatti (out of this project), 2014-05-06 15:00:10]

On 2014/05/05 23:29:24, Ami Fischman wrote:
> perhaps the root of my confusion is being unclear about how concrete you
intend
> content::VideoDecodeFactoryOzone to ever get.
> Is it your intention that each Ozone platform provide a subclass(?) of
> content::VideoDecodeFactoryOzone?  Do you intend to check in any such
subclasses
> into chromium?  Where do you plan them to live?

ah yes, I forgot to explain that. Sorry.

Each implementation decides whether to subclass c::VideoDecodeFactoryOzone or
not. If a implementation doesn't subclass it then it's fine cause
c::VideoDecodeFactoryOzone is non-pure virtual. That's basically how the others
Ozone APIs are working at the moment as well.

I don't want to confuse you more with code but I hope it helps to show the WIP
in my Ozone-Wayland implementation:
https://github.com/tiagovignatti/ozone-wayland/commit/8f3131634f6df45d807276c...

[vignatti (out of this project), 2014-05-06 15:01:43]

+dnicoara@, +spang

[spang, 2014-05-06 15:38:42]

One way to fix up this kind of dependency cycle is:

- Create a new media/types component that defines the platform-dependent
interfaces.

- ui/ozone can depend on media/types

- media/ & content/ must depend on ui/ozone

In this case media/types would include things like video_decode_accelerator.h
and any other media headers it references.

You can see an example of this pattern in ui/display + ui/display/types.

[dshwang, 2014-05-06 17:07:45]

Thank you. ozone needs hw video.

https://codereview.chromium.org/269673005/diff/1/content/common/gpu/media/ozo...
File content/common/gpu/media/ozone_video_decode_accelerator.cc (right):

https://codereview.chromium.org/269673005/diff/1/content/common/gpu/media/ozo...
content/common/gpu/media/ozone_video_decode_accelerator.cc:25: return
VideoDecodeFactoryOzone::GetInstance()->Initialize(profile, client);
I'm not sure you can use singleton pattern, because afaik multiple
OzoneVideoDecodeAccelerator instances are created for every html video.
So ozone-wayland or ozone-dri should register factory function, and this class
should create an impl instance using the factory function. The change perhaps
resolves cross-dependency automatically.

[vignatti (out of this project), 2014-05-06 20:33:00]

On 2014/05/06 15:38:42, spang wrote:
> One way to fix up this kind of dependency cycle is:
> 
> - Create a new media/types component that defines the platform-dependent
> interfaces.
> 
> - ui/ozone can depend on media/types
> 
> - media/ & content/ must depend on ui/ozone
> 
> In this case media/types would include things like video_decode_accelerator.h
> and any other media headers it references.
> 
> You can see an example of this pattern in ui/display + ui/display/types.

ah nice. I've studied the changes you made and that makes sense for me.
fischman@ and other media OWNERS, based on spang's comment, this is an analogous
on what I'll be working next: https://codereview.chromium.org/230763004

Please let me know if you have any concerns on that kind of split. Thank you.

[Ami GONE FROM CHROMIUM, 2014-05-07 00:42:18]

On Tue, May 6, 2014 at 8:38 AM, <spang@chromium.org> wrote:

> One way to fix up this kind of dependency cycle is:
>
> - Create a new media/types component that defines the platform-dependent
> interfaces.
>

What would those interfaces be in this case?

- ui/ozone can depend on media/types
>

I have no problem with this.


> - media/ & content/ must depend on ui/ozone
>

Do you really mean media/ above?  That still seems cyclical unless
media/types can be made to not depend on anything else in media and nothing
in media/types takes advantage of the inclusion of ui/ozone in media/DEPS.

In this case media/types would include things like
> video_decode_accelerator.h
>

v_d_a.h is already in media/video, not in ui/ or content/


> and any other media headers it references.
>

I think I need the concrete expansion of this clause to understand this
proposal.


To put my question in a different way: what details do the ozone impls
_share_, which makes it necessary to have an ozone-specific VDA impl
(conditionally overridden by an ozone platform impl)?
(the naive alternative would be to ignore than e.g. OzoneDri and
OzoneWayland are both Ozone-flavored, and instead just teach GVDA to
instantiate e.g. OzoneDriVideoDecodeAccelerator and
OzoneWaylandVideoDecodeAccelerator; but presumably these could share enough
code that it would be nice to avoid copy/pasta; why can that sharing not be
achieved using objects local to content/common/gpu/media ?)


> https://codereview.chromium.org/269673005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-05-07 01:58:22]

On 2014/05/07 00:42:18, Ami Fischman wrote:
> On Tue, May 6, 2014 at 8:38 AM, <mailto:spang@chromium.org> wrote:
> 
> > One way to fix up this kind of dependency cycle is:
> >
> > - Create a new media/types component that defines the platform-dependent
> > interfaces.
> >
> 
> What would those interfaces be in this case?
> 
> - ui/ozone can depend on media/types
> >
> 
> I have no problem with this.
> 
> 
> > - media/ & content/ must depend on ui/ozone
> >
> 
> Do you really mean media/ above?  That still seems cyclical unless
> media/types can be made to not depend on anything else in media and nothing
> in media/types takes advantage of the inclusion of ui/ozone in media/DEPS.
> 
> In this case media/types would include things like
> > video_decode_accelerator.h
> >
> 
> v_d_a.h is already in media/video, not in ui/ or content/
> 
> 
> > and any other media headers it references.
> >
> 
> I think I need the concrete expansion of this clause to understand this
> proposal.
> 
> 
> To put my question in a different way: what details do the ozone impls
> _share_, which makes it necessary to have an ozone-specific VDA impl
> (conditionally overridden by an ozone platform impl)?
> (the naive alternative would be to ignore than e.g. OzoneDri and
> OzoneWayland are both Ozone-flavored, and instead just teach GVDA to
> instantiate e.g. OzoneDriVideoDecodeAccelerator and
> OzoneWaylandVideoDecodeAccelerator; but presumably these could share enough
> code that it would be nice to avoid copy/pasta; why can that sharing not be
> achieved using objects local to content/common/gpu/media ?)

The issue isn't really about code sharing between ozone subplatforms. I think
OzoneVideoDecodeAccelerator can be removed. It looks like we should just send
the failure reply if there's no concrete subclass provided by the platform.

One of the goals of ozone is to enable out-of-tree ports of chromium (to various
windowing systems or hardware). So we can't do

#elif defined(USE_WAYLAND)
  video_decode_accelerator_.reset(new
WaylandVideoDecodeAccelerator(make_context_current_));
#else

If we want acceleration under ozone, it has to look like this instead:

#elif defined(USE_OZONE)
 
video_decode_accelerator_.reset(ui::OzonePlatform::GetInstance()->CreateVideoDecodeAccelerator());
#else

The concrete type of VideoDecodeAccelerator varies depending on which platforms
were compiled into the binary, and on the value of the --ozone-platform flag.

In order to implement OzonePlatform::CreateVideoDecodeAccelerator(), we need
access to the base class. That's why creating a types  component is helpful -
there's no way to get access to the base class otherwise.

> > https://codereview.chromium.org/269673005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-07 02:09:07]

Is it desired to build multiple ozones into a single binary?
Can the approach taken by c/c/g/m/v4l2device be used?
On May 6, 2014 6:58 PM, <spang@chromium.org> wrote:

> On 2014/05/07 00:42:18, Ami Fischman wrote:
>
>> On Tue, May 6, 2014 at 8:38 AM, <mailto:spang@chromium.org> wrote:
>>
>
>  > One way to fix up this kind of dependency cycle is:
>> >
>> > - Create a new media/types component that defines the platform-dependent
>> > interfaces.
>> >
>>
>
>  What would those interfaces be in this case?
>>
>
>  - ui/ozone can depend on media/types
>> >
>>
>
>  I have no problem with this.
>>
>
>
>  > - media/ & content/ must depend on ui/ozone
>> >
>>
>
>  Do you really mean media/ above?  That still seems cyclical unless
>> media/types can be made to not depend on anything else in media and
>> nothing
>> in media/types takes advantage of the inclusion of ui/ozone in media/DEPS.
>>
>
>  In this case media/types would include things like
>> > video_decode_accelerator.h
>> >
>>
>
>  v_d_a.h is already in media/video, not in ui/ or content/
>>
>
>
>  > and any other media headers it references.
>> >
>>
>
>  I think I need the concrete expansion of this clause to understand this
>> proposal.
>>
>
>
>  To put my question in a different way: what details do the ozone impls
>> _share_, which makes it necessary to have an ozone-specific VDA impl
>> (conditionally overridden by an ozone platform impl)?
>> (the naive alternative would be to ignore than e.g. OzoneDri and
>> OzoneWayland are both Ozone-flavored, and instead just teach GVDA to
>> instantiate e.g. OzoneDriVideoDecodeAccelerator and
>> OzoneWaylandVideoDecodeAccelerator; but presumably these could share
>> enough
>> code that it would be nice to avoid copy/pasta; why can that sharing not
>> be
>> achieved using objects local to content/common/gpu/media ?)
>>
>
> The issue isn't really about code sharing between ozone subplatforms. I
> think
> OzoneVideoDecodeAccelerator can be removed. It looks like we should just
> send
> the failure reply if there's no concrete subclass provided by the platform.
>
> One of the goals of ozone is to enable out-of-tree ports of chromium (to
> various
> windowing systems or hardware). So we can't do
>
> #elif defined(USE_WAYLAND)
>   video_decode_accelerator_.reset(new
> WaylandVideoDecodeAccelerator(make_context_current_));
> #else
>
> If we want acceleration under ozone, it has to look like this instead:
>
> #elif defined(USE_OZONE)
>
> video_decode_accelerator_.reset(ui::OzonePlatform::GetInstance()->
> CreateVideoDecodeAccelerator());
> #else
>
> The concrete type of VideoDecodeAccelerator varies depending on which
> platforms
> were compiled into the binary, and on the value of the --ozone-platform
> flag.
>
> In order to implement OzonePlatform::CreateVideoDecodeAccelerator(), we
> need
> access to the base class. That's why creating a types  component is
> helpful -
> there's no way to get access to the base class otherwise.
>
>  > https://codereview.chromium.org/269673005/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/269673005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-07 02:50:09]

On Tue, May 6, 2014 at 6:58 PM, <spang@chromium.org> wrote:

> On 2014/05/07 00:42:18, Ami Fischman wrote:
>
>  On Tue, May 6, 2014 at 8:38 AM, <mailto:spang@chromium.org> wrote:
>>
>
>  > One way to fix up this kind of dependency cycle is:
>> >
>> > - Create a new media/types component that defines the platform-dependent
>> > interfaces.
>> >
>>
>
>  What would those interfaces be in this case?
>>
>
>  - ui/ozone can depend on media/types
>> >
>>
>
>  I have no problem with this.
>>
>
>
>  > - media/ & content/ must depend on ui/ozone
>> >
>>
>
>  Do you really mean media/ above?  That still seems cyclical unless
>> media/types can be made to not depend on anything else in media and
>> nothing
>> in media/types takes advantage of the inclusion of ui/ozone in media/DEPS.
>>
>
>  In this case media/types would include things like
>> > video_decode_accelerator.h
>> >
>>
>
>  v_d_a.h is already in media/video, not in ui/ or content/
>>
>
>
>  > and any other media headers it references.
>> >
>>
>
>  I think I need the concrete expansion of this clause to understand this
>> proposal.
>>
>
>
>  To put my question in a different way: what details do the ozone impls
>> _share_, which makes it necessary to have an ozone-specific VDA impl
>> (conditionally overridden by an ozone platform impl)?
>> (the naive alternative would be to ignore than e.g. OzoneDri and
>> OzoneWayland are both Ozone-flavored, and instead just teach GVDA to
>> instantiate e.g. OzoneDriVideoDecodeAccelerator and
>> OzoneWaylandVideoDecodeAccelerator; but presumably these could share
>> enough
>> code that it would be nice to avoid copy/pasta; why can that sharing not
>> be
>> achieved using objects local to content/common/gpu/media ?)
>>
>
> The issue isn't really about code sharing between ozone subplatforms. I
> think
> OzoneVideoDecodeAccelerator can be removed. It looks like we should just
> send
> the failure reply if there's no concrete subclass provided by the platform.
>
> One of the goals of ozone is to enable out-of-tree ports of chromium (to
> various
> windowing systems or hardware). So we can't do
>
> #elif defined(USE_WAYLAND)
>   video_decode_accelerator_.reset(new
> WaylandVideoDecodeAccelerator(make_context_current_));
> #else
>
> If we want acceleration under ozone, it has to look like this instead:
>
> #elif defined(USE_OZONE)
>
> video_decode_accelerator_.reset(ui::OzonePlatform::GetInstance()->
> CreateVideoDecodeAccelerator());
> #else
>
> The concrete type of VideoDecodeAccelerator varies depending on which
> platforms
> were compiled into the binary, and on the value of the --ozone-platform
> flag.
>
> In order to implement OzonePlatform::CreateVideoDecodeAccelerator(), we
> need
> access to the base class. That's why creating a types  component is
> helpful -
> there's no way to get access to the base class otherwise.
>

Drive-by side note:
I would like to move most of content/common/gpu/client to a lower level,
e.g. gpu/ipc, so that I can use it from ppapi/proxy which can't depend on
content and needs to compile on NaCl. The end goal being to have pepper
talk directly to the GPU process without going through the renderer.
To achieve this, I would need the media parts of content/common/gpu/client
(GVDAHost/GVEAHost) to compile in NaCl, if possible without dragging all of
media/

A more nebulous project is to have those in mojo as well, which also can't
depend on content/. The needs are similar-ish.

So if those interfaces (and their dependencies) can be separated out in a
self-contained component, it would be really useful for those 2 things as
well.


>
>  > https://codereview.chromium.org/269673005/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/269673005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qing.zhang, 2014-05-07 03:01:31]

Looks the "(USE_OZONE)" should parallel with "(USE_X11)" in this CL.But since
the OZONE is abstract graphic layer and X11 parallel with wayland or dri, So
shall we re-design VDA impl as below:

OZONE |---> X11
OZONE |---> WAYLAND
OZONE |---> DRM
OZONE |---> DRI
OZONE |---> 'new'

to replace CL did as below which little bit confused :

Original |---> X11

OZONE |---> WAYLAND
OZONE |---> DRM
OZONE |---> DRI
OZONE |---> 'new'

[spang, 2014-05-07 16:15:13]

On 2014/05/07 02:09:07, Ami Fischman wrote:
> Is it desired to build multiple ozones into a single binary?

So far we have maintained the ability to delay binding of the platform until
runtime, and so we do already allow multiple platforms to be built into one
binary.

> Can the approach taken by c/c/g/m/v4l2device be used?

It looks like v4l2_video_device tries each implementation in sequence. So a port
would need to patch this file to add its implementation to the list. That's a
problem.

> On May 6, 2014 6:58 PM, <mailto:spang@chromium.org> wrote:
> 
> > On 2014/05/07 00:42:18, Ami Fischman wrote:
> >
> >> On Tue, May 6, 2014 at 8:38 AM, <mailto:spang@chromium.org> wrote:
> >>
> >
> >  > One way to fix up this kind of dependency cycle is:
> >> >
> >> > - Create a new media/types component that defines the platform-dependent
> >> > interfaces.
> >> >
> >>
> >
> >  What would those interfaces be in this case?
> >>
> >
> >  - ui/ozone can depend on media/types
> >> >
> >>
> >
> >  I have no problem with this.
> >>
> >
> >
> >  > - media/ & content/ must depend on ui/ozone
> >> >
> >>
> >
> >  Do you really mean media/ above?  That still seems cyclical unless
> >> media/types can be made to not depend on anything else in media and
> >> nothing
> >> in media/types takes advantage of the inclusion of ui/ozone in media/DEPS.
> >>
> >
> >  In this case media/types would include things like
> >> > video_decode_accelerator.h
> >> >
> >>
> >
> >  v_d_a.h is already in media/video, not in ui/ or content/
> >>
> >
> >
> >  > and any other media headers it references.
> >> >
> >>
> >
> >  I think I need the concrete expansion of this clause to understand this
> >> proposal.
> >>
> >
> >
> >  To put my question in a different way: what details do the ozone impls
> >> _share_, which makes it necessary to have an ozone-specific VDA impl
> >> (conditionally overridden by an ozone platform impl)?
> >> (the naive alternative would be to ignore than e.g. OzoneDri and
> >> OzoneWayland are both Ozone-flavored, and instead just teach GVDA to
> >> instantiate e.g. OzoneDriVideoDecodeAccelerator and
> >> OzoneWaylandVideoDecodeAccelerator; but presumably these could share
> >> enough
> >> code that it would be nice to avoid copy/pasta; why can that sharing not
> >> be
> >> achieved using objects local to content/common/gpu/media ?)
> >>
> >
> > The issue isn't really about code sharing between ozone subplatforms. I
> > think
> > OzoneVideoDecodeAccelerator can be removed. It looks like we should just
> > send
> > the failure reply if there's no concrete subclass provided by the platform.
> >
> > One of the goals of ozone is to enable out-of-tree ports of chromium (to
> > various
> > windowing systems or hardware). So we can't do
> >
> > #elif defined(USE_WAYLAND)
> >   video_decode_accelerator_.reset(new
> > WaylandVideoDecodeAccelerator(make_context_current_));
> > #else
> >
> > If we want acceleration under ozone, it has to look like this instead:
> >
> > #elif defined(USE_OZONE)
> >
> > video_decode_accelerator_.reset(ui::OzonePlatform::GetInstance()->
> > CreateVideoDecodeAccelerator());
> > #else
> >
> > The concrete type of VideoDecodeAccelerator varies depending on which
> > platforms
> > were compiled into the binary, and on the value of the --ozone-platform
> > flag.
> >
> > In order to implement OzonePlatform::CreateVideoDecodeAccelerator(), we
> > need
> > access to the base class. That's why creating a types  component is
> > helpful -
> > there's no way to get access to the base class otherwise.
> >
> >  > https://codereview.chromium.org/269673005/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> > https://codereview.chromium.org/269673005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-07 16:54:23]

>
> Can the approach taken by c/c/g/m/v4l2device be used?
>
> It looks like v4l2_video_device tries each implementation in sequence. So
> a port
> would need to patch this file to add its implementation to the list.
> That's a
> problem.


ISTM there are various ways to get around this with solutions that don't
escape content/; e.g.:
- dlopen/dlsym/RTLD_NEXT to invoke all linked-in factories (only one of
which will be a no-op, presumably)
- command-line flag or another scheme to name the desired platform coupled
with dlsym

I'm not fundamentally opposed to a scheme that is driven by ui/ozone, but
I'd really like to understand more the requirements that will drive on
media/.
(perhaps obviously, I would like to avoid having lasting unnatural
contortions in media/)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-07 16:55:09]

On Tue, May 6, 2014 at 8:01 PM, <qing.zhang@intel.com> wrote:

> Looks the "(USE_OZONE)" should parallel with "(USE_X11)" in this CL.But
> since
> the OZONE is abstract graphic layer and X11 parallel with wayland or dri,
> So
> shall we re-design VDA impl as below:
>

I'm sorry, I don't follow.  Can you say more about what you're proposing?
(note that there are multiple X11-using VDA impls already)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-07 16:59:08]

On Tue, May 6, 2014 at 7:49 PM, Antoine Labour <piman@google.com> wrote:
>
> Drive-by side note:
> I would like to move most of content/common/gpu/client to a lower level,
> e.g. gpu/ipc, so that I can use it from ppapi/proxy which can't depend on
> content and needs to compile on NaCl. The end goal being to have pepper
> talk directly to the GPU process without going through the renderer.
> To achieve this, I would need the media parts of content/common/gpu/client
> (GVDAHost/GVEAHost) to compile in NaCl, if possible without dragging all of
> media/
>

I suspect GV[ED]AHost only require media/base and media/video (which in
turn depend on ui/gfx and maybe some other baseish ui/).


> So if those interfaces (and their dependencies) can be separated out in a
> self-contained component, it would be really useful for those 2 things as
> well.
>

@scherkus: is there an effort this could be coalesced into?  (is anyone
else asking to depend on some public API of media/?)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-05-07 17:20:02]

On 2014/05/07 16:54:23, Ami Fischman wrote:
> >
> > Can the approach taken by c/c/g/m/v4l2device be used?
> >
> > It looks like v4l2_video_device tries each implementation in sequence. So
> > a port
> > would need to patch this file to add its implementation to the list.
> > That's a
> > problem.
> 
> 
> ISTM there are various ways to get around this with solutions that don't
> escape content/; e.g.:
> - dlopen/dlsym/RTLD_NEXT to invoke all linked-in factories (only one of
> which will be a no-op, presumably)
> - command-line flag or another scheme to name the desired platform coupled
> with dlsym

Ports can't currently #include your header file to subclass this interface. We
need to solve that issue first.

> I'm not fundamentally opposed to a scheme that is driven by ui/ozone, but
> I'd really like to understand more the requirements that will drive on
> media/.
> (perhaps obviously, I would like to avoid having lasting unnatural
> contortions in media/)
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-07 17:25:28]

On Wed, May 7, 2014 at 10:20 AM, <spang@chromium.org> wrote:

> Ports can't currently #include your header file to subclass this
> interface. We
> need to solve that issue first.
>

Is that assuming that ports can only add code under ui/ozone?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-05-07 17:31:17]

On Wed, May 7, 2014 at 1:25 PM, Ami Fischman <fischman@chromium.org> wrote:

>
> On Wed, May 7, 2014 at 10:20 AM, <spang@chromium.org> wrote:
>
>> Ports can't currently #include your header file to subclass this
>> interface. We
>> need to solve that issue first.
>>
>
> Is that assuming that ports can only add code under ui/ozone?
>

That's the only place so far, but it's certainly possible to add more. We
just need to figure out what makes sense.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-07 17:40:37]

So what prevents ports from depending on media/video/*.h?


On Wed, May 7, 2014 at 10:30 AM, Michael Spang <spang@chromium.org> wrote:

>
>
>
> On Wed, May 7, 2014 at 1:25 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>>
>> On Wed, May 7, 2014 at 10:20 AM, <spang@chromium.org> wrote:
>>
>>> Ports can't currently #include your header file to subclass this
>>> interface. We
>>> need to solve that issue first.
>>>
>>
>> Is that assuming that ports can only add code under ui/ozone?
>>
>
> That's the only place so far, but it's certainly possible to add more. We
> just need to figure out what makes sense.
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-05-07 18:13:18]

There's at least one cycle that prevents the straightforward fix:

media -> ui/gl -> ui/ozone -> media

We can try to break that cycle instead, with the following caveat: media
won't be able to initialize ozone (this is where the platform binding
happens). Maybe that's OK.

On Wed, May 7, 2014 at 1:40 PM, Ami Fischman <fischman@chromium.org> wrote:

> So what prevents ports from depending on media/video/*.h?
>
>
> On Wed, May 7, 2014 at 10:30 AM, Michael Spang <spang@chromium.org> wrote:
>
>>
>>
>>
>> On Wed, May 7, 2014 at 1:25 PM, Ami Fischman <fischman@chromium.org>wrote:
>>
>>>
>>> On Wed, May 7, 2014 at 10:20 AM, <spang@chromium.org> wrote:
>>>
>>>> Ports can't currently #include your header file to subclass this
>>>> interface. We
>>>> need to solve that issue first.
>>>>
>>>
>>> Is that assuming that ports can only add code under ui/ozone?
>>>
>>
>> That's the only place so far, but it's certainly possible to add more. We
>> just need to figure out what makes sense.
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vignatti (out of this project), 2014-05-07 22:28:43]

alright guys. I believe I understood your (valuable) comments so now I've
managed to fix the deps issue through breaking up some video headers into
media/types, as suggested by spang@. PTAL in the patch set 2.

[spang, 2014-05-07 22:43:14]

On 2014/05/07 22:28:43, vignatti wrote:
> alright guys. I believe I understood your (valuable) comments so now I've
> managed to fix the deps issue through breaking up some video headers into
> media/types, as suggested by spang@. PTAL in the patch set 2.

Even supposing there's consensus on allowing this split, the current patch
doesn't do it correctly.

Source files in the media_types component can't reference headers the component
doesn't depend on. And it can't depend on media or we have the same problem as
before.

[vignatti (out of this project), 2014-05-07 22:50:33]

On 2014/05/07 22:43:14, spang wrote:
> Even supposing there's consensus on allowing this split, the current patch
> doesn't do it correctly.
> 
> Source files in the media_types component can't reference headers the
component
> doesn't depend on. And it can't depend on media or we have the same problem as
> before.

I see the problem. Basically I have to move
media/video/video_decode_accelerator.h also to media/types, right?

[Ami GONE FROM CHROMIUM, 2014-05-07 22:51:27]

Thanks; the concrete media/types helped me understand better what you're
thinking.

https://codereview.chromium.org/269673005/diff/20001/media/types/video_decode...
File media/types/video_decode_accelerator_impl.h (right):

https://codereview.chromium.org/269673005/diff/20001/media/types/video_decode...
media/types/video_decode_accelerator_impl.h:25: virtual bool
CanDecodeOnIOThread();
This method is the only reason for the existence of this class.  This class was
created to clarify the distinction that this method only makes sense in the GPU
process (not in the renderer process, where there are also implementations of
the VDA interface): https://chromiumcodereview.appspot.com/23125014/#msg12

If this interface is going to live in a not-scoped-to-the-gpu-process directory,
and not have any gpu-specific name attached to it, then this method might as
well be added to video_decode_accelerator.h and this file deleted, and with it
the need for media/types?

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
File ui/ozone/media/video_decode_factory_ozone.cc (right):

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
ui/ozone/media/video_decode_factory_ozone.cc:37: NOTREACHED();
this and all the other NOTREACHED's below don't make sense to me in the world
you've described where platforms are free to implement alternative VDFO's or
not.

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
File ui/ozone/media/video_decode_factory_ozone.h (right):

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
ui/ozone/media/video_decode_factory_ozone.h:18: : public
media::VideoDecodeAcceleratorImpl {
Per another comment earlier in the reviewlog, having a singleton backing VDA is
not going to work except for very specific use-cases (e.g. a TV).  Each <video>
tag will attempt to create an instance of OzoneVDA, which as you have it ATM
will cause them all to trigger the same VDFactoryO and sadness will ensue.

I think you want to make the factory vend newly-minted platform-specific
OzoneVDA impls on demand.

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
ui/ozone/media/video_decode_factory_ozone.h:18: : public
media::VideoDecodeAcceleratorImpl {
I don't think you need the factory to inherit from VDAI even now (and of course
if you take my suggestion above it definitely doesn't need to).

[spang, 2014-05-08 00:02:00]

I tried extracting the VideoDecodeAccelerator interface into a new component as
I suggested and it is not really sufficient, as there's more dependency issues
involving PictureBuffer & the gpu component.

I'll see if I can come up with a less invasive solution.

[vignatti (out of this project), 2014-06-02 19:21:42]

hi! I'm back on this work.

I've updated the patchset here (v3) given that I finally sorted out the Ozone
implementation side, in Ozone-Wayland using vaapi. In v3, all the factories I've
been adding before are gone so the things look simpler. We need to figure out
now how to tackle the issue with the media/types dependencies.

FYI, this is how Ozone-Wayland is looking: 
https://github.com/tiagovignatti/ozone-wayland/commit/8ef70b7870f975977ae53b1...

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
File ui/ozone/media/video_decode_factory_ozone.cc (right):

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
ui/ozone/media/video_decode_factory_ozone.cc:37: NOTREACHED();
On 2014/05/07 22:51:27, Ami leaving Chromium June 9th wrote:
> this and all the other NOTREACHED's below don't make sense to me in the world
> you've described where platforms are free to implement alternative VDFO's or
> not.

Done.

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
File ui/ozone/media/video_decode_factory_ozone.h (right):

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
ui/ozone/media/video_decode_factory_ozone.h:18: : public
media::VideoDecodeAcceleratorImpl {
On 2014/05/07 22:51:27, Ami leaving Chromium June 9th wrote:
> I don't think you need the factory to inherit from VDAI even now (and of
course
> if you take my suggestion above it definitely doesn't need to).

Done.

https://codereview.chromium.org/269673005/diff/20001/ui/ozone/media/video_dec...
ui/ozone/media/video_decode_factory_ozone.h:18: : public
media::VideoDecodeAcceleratorImpl {
On 2014/05/07 22:51:27, Ami leaving Chromium June 9th wrote:
> Per another comment earlier in the reviewlog, having a singleton backing VDA
is
> not going to work except for very specific use-cases (e.g. a TV).  Each
<video>
> tag will attempt to create an instance of OzoneVDA, which as you have it ATM
> will cause them all to trigger the same VDFactoryO and sadness will ensue.
> 
> I think you want to make the factory vend newly-minted platform-specific
> OzoneVDA impls on demand.

Done.

[Ami GONE FROM CHROMIUM, 2014-06-02 19:40:34]

I think you missed some comment(s) I made on PS#2
(https://codereview.chromium.org/269673005/#ps20001) that e.g. mean media/types
should not be necessary.

[vignatti (out of this project), 2014-06-02 20:17:21]

On 2014/06/02 19:40:34, Ami leaving Chromium June 9th wrote:
> I think you missed some comment(s) I made on PS#2
> (https://codereview.chromium.org/269673005/#ps20001) that e.g. mean
media/types
> should not be necessary.

ok, I left intentionally not commented because that's related with the
media/types dependencies problem we need to talk about. That said, even if we
remove entirely VideoDecodeAcceleratorImpl class the VDA implementations will
need to inherit VideoDecodeAccelerator which is where the real problem lays
(with Picture and ui/gfx stuff). Makes sense?

[Ami GONE FROM CHROMIUM, 2014-06-02 20:30:19]

On 2014/06/02 20:17:21, vignatti wrote:
> On 2014/06/02 19:40:34, Ami leaving Chromium June 9th wrote:
> > I think you missed some comment(s) I made on PS#2
> > (https://codereview.chromium.org/269673005/#ps20001) that e.g. mean
> media/types
> > should not be necessary.
> 
> ok, I left intentionally not commented because that's related with the
> media/types dependencies problem we need to talk about. That said, even if we
> remove entirely VideoDecodeAcceleratorImpl class the VDA implementations will
> need to inherit VideoDecodeAccelerator which is where the real problem lays
> (with Picture and ui/gfx stuff). Makes sense?

Sort of.  I guess you're looking for input from spang@ on how to move this
forward?
(if that's not it, please be more explicit)

[spang, 2014-06-03 16:55:14]

On 2014/06/02 20:30:19, Ami leaving Chromium June 9th wrote:
> On 2014/06/02 20:17:21, vignatti wrote:
> > On 2014/06/02 19:40:34, Ami leaving Chromium June 9th wrote:
> > > I think you missed some comment(s) I made on PS#2
> > > (https://codereview.chromium.org/269673005/#ps20001) that e.g. mean
> > media/types
> > > should not be necessary.
> > 
> > ok, I left intentionally not commented because that's related with the
> > media/types dependencies problem we need to talk about. That said, even if
we
> > remove entirely VideoDecodeAcceleratorImpl class the VDA implementations
will
> > need to inherit VideoDecodeAccelerator which is where the real problem lays
> > (with Picture and ui/gfx stuff). Makes sense?
> 
> Sort of.  I guess you're looking for input from spang@ on how to move this
> forward?
> (if that's not it, please be more explicit)

I think we need to think about how to componentize ozone. We have gotten by so
far with a centralized ui/ozone component but this has not been painless and
this is particularly problematic for media & VDA.

[vignatti (out of this project), 2014-06-06 21:43:35]

patch was updated now. It relies on https://codereview.chromium.org/317083003/
that must be applied before. PTAL.

[Ami GONE FROM CHROMIUM, 2014-06-06 22:09:17]

defer to scherkus/spang for future review
(this CL is about ozone & plumbing, not about HW acceleration)

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
File media/ozone/ozone_platform.h (right):

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.h:34: virtual VideoDecodeAccelerator*
CreateVideoDecodeFactoryOzone(
CreateVideoDecodeFactoryOzone is inappropriate considering what this does, which
is more a CreateVideoDecodeAccelerator().

("Ozone" is implied by the containing class, and "Factory" is not a thing
anymore)

[spang, 2014-06-06 22:23:34]

On 2014/06/06 22:09:17, Ami leaving Chromium June 6th wrote:
> defer to scherkus/spang for future review
> (this CL is about ozone & plumbing, not about HW acceleration)
> 
>
https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
> File media/ozone/ozone_platform.h (right):
> 
>
https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
> media/ozone/ozone_platform.h:34: virtual VideoDecodeAccelerator*
> CreateVideoDecodeFactoryOzone(
> CreateVideoDecodeFactoryOzone is inappropriate considering what this does,
which
> is more a CreateVideoDecodeAccelerator().
> 
> ("Ozone" is implied by the containing class, and "Factory" is not a thing
> anymore)

I think we should dispense with the OzonePlatformMedia and just do

   ui::PlatformObject<VideoDecodeAccelerator>::Create()

inside gpu_video_decode_accelerator.cc

If the platform wants to fiddle with some shared state, it can do so internally
to itself.

[vignatti (out of this project), 2014-06-09 17:57:00]

On 2014/06/06 22:23:34, spang wrote:
> On 2014/06/06 22:09:17, Ami leaving Chromium June 6th wrote:
> > defer to scherkus/spang for future review
> > (this CL is about ozone & plumbing, not about HW acceleration)
> > 
> >
>
https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
> > File media/ozone/ozone_platform.h (right):
> > 
> >
>
https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
> > media/ozone/ozone_platform.h:34: virtual VideoDecodeAccelerator*
> > CreateVideoDecodeFactoryOzone(
> > CreateVideoDecodeFactoryOzone is inappropriate considering what this does,
> which
> > is more a CreateVideoDecodeAccelerator().
> > 
> > ("Ozone" is implied by the containing class, and "Factory" is not a thing
> > anymore)
> 
> I think we should dispense with the OzonePlatformMedia and just do
> 
>    ui::PlatformObject<VideoDecodeAccelerator>::Create()
> 
> inside gpu_video_decode_accelerator.cc
> 
> If the platform wants to fiddle with some shared state, it can do so
internally
> to itself.

right, fine.

so what's the plan to pass make_context_current_ to the implementation then?
Should we automatize through the script and pass it to the template constructor?

[spang, 2014-06-09 19:01:58]

https://codereview.chromium.org/269673005/diff/50001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/269673005/diff/50001/media/media.gyp#newcode721
media/media.gyp:721: ['use_ozone==1', {
(question for media/ owners)

should we move this into a new file

  media/ozone/media_ozone.gyp[i]

?

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
File media/ozone/ozone_platform.h (right):

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.h:5: #ifndef MEDIA_OZONE_OZONE_PLATFORM_H_
filename should match class name

And I think OzonePlatformForMedia or MediaOzonePlatform makes more grammatical
sense.

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.h:21: class OZONE_EXPORT OzonePlatformMedia {
wrong export, this is part of media.

[spang, 2014-06-09 19:17:33]

https://codereview.chromium.org/269673005/diff/50001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/269673005/diff/50001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:284:
make_context_current_));
Probably need to check for NULL return here:

if (!video_decode_accelerator_) {
  SendCreateDecoderReply(init_done_msg, false);
  return.
}

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
File media/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.cc:16: OzonePlatformMedia*
CreateOzonePlatformMediaDri() { return NULL; }
You are dereferencing this NULL elsewhere.

For now, could add a StubOzonePlatformMedia.

[vignatti (out of this project), 2014-06-09 20:30:27]

PTAL in patchset 5 now. Thank you.

https://codereview.chromium.org/269673005/diff/50001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/269673005/diff/50001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:284:
make_context_current_));
On 2014/06/09 19:17:32, spang wrote:
> Probably need to check for NULL return here:
> 
> if (!video_decode_accelerator_) {
>   SendCreateDecoderReply(init_done_msg, false);
>   return.
> }

Done.

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
File media/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.cc:16: OzonePlatformMedia*
CreateOzonePlatformMediaDri() { return NULL; }
On 2014/06/09 19:17:32, spang wrote:
> You are dereferencing this NULL elsewhere.
> 
> For now, could add a StubOzonePlatformMedia.

Done.

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
File media/ozone/ozone_platform.h (right):

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.h:5: #ifndef MEDIA_OZONE_OZONE_PLATFORM_H_
On 2014/06/09 19:01:58, spang wrote:
> filename should match class name
> 
> And I think OzonePlatformForMedia or MediaOzonePlatform makes more grammatical
> sense.
> 

Done.

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.h:21: class OZONE_EXPORT OzonePlatformMedia {
On 2014/06/09 19:01:58, spang wrote:
> wrong export, this is part of media.

Done.

https://codereview.chromium.org/269673005/diff/50001/media/ozone/ozone_platfo...
media/ozone/ozone_platform.h:34: virtual VideoDecodeAccelerator*
CreateVideoDecodeFactoryOzone(
On 2014/06/06 22:09:17, Ami GONE FROM CHROMIUM wrote:
> CreateVideoDecodeFactoryOzone is inappropriate considering what this does,
which
> is more a CreateVideoDecodeAccelerator().
> 
> ("Ozone" is implied by the containing class, and "Factory" is not a thing
> anymore)

Done.

[spang, 2014-06-09 20:53:57]

lgtm

https://codereview.chromium.org/269673005/diff/70001/content/common/gpu/media...
File content/common/gpu/media/DEPS (right):

https://codereview.chromium.org/269673005/diff/70001/content/common/gpu/media...
content/common/gpu/media/DEPS:3: "+media/ozone",
I don't think you need this.

https://codereview.chromium.org/269673005/diff/70001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/70001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:40: }
Two more that are off by default:

Gbm
Caca

[vignatti (out of this project), 2014-06-09 21:01:53]

The CQ bit was checked by tiago.vignatti@intel.com

[commit-bot: I haz the power, 2014-06-09 21:04:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/269673005/90001

[vignatti (out of this project), 2014-06-09 22:14:47]

adding xhwang@ for media/ and content/common/gpu/media/

can you take a look? Thank you.

[xhwang, 2014-06-09 23:53:08]

On 2014/06/09 22:14:47, vignatti wrote:
> adding xhwang@ for media/ and content/common/gpu/media/
> 
> can you take a look? Thank you.

Well media/ and content/common/gpu/media is pretty much the whole CL :P

I am not super familiar with this area, so I read about the CL description and 
http://www.chromium.org/developers/design-documents/ozone trying to understand
more
background of this CL. I have a few questions.

1, Are we trying to provide a path for other ozone platforms to provide their
own
video decoder? Are there decoders all GPU based? (since we are changing
c/c/gpu/media)

2, For your comment: "implementations can decide themselves whether a video
implementation
is actually wanted". How will this be implemented? Will
CreateVideoDecodeAccelerator() return
NULL? Or should CreateMediaOzonePlatformXxx just return NULL?

3: Is there a doc about converting media decoding using ozone? I am also
interested in any
plan that converts ChromeOS or ChromeCast to use ozone for video decoding.
Thanks!

[xhwang, 2014-06-09 23:53:34]

And here are some trivial comments.

https://codereview.chromium.org/269673005/diff/90001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/269673005/diff/90001/media/media.gyp#newcode727
media/media.gyp:727: '<(SHARED_INTERMEDIATE_DIR)',
Comment about what this is used for.

https://codereview.chromium.org/269673005/diff/90001/media/media.gyp#newcode737
media/media.gyp:737: 'action_name': 'generate_constructor_list',
Add a comment about what this generates. Since this is in media/, it's not
obvious for people without searching for "generate_constructor_list".

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:29: // internal platforms decide to actually
implement their media specifics
nit: Punctuation, Spelling and Grammar:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:49: }
Are these declared by generate_constructor_list.py?

These creation functions are not used in this CL? Shall we add them later when
it's actually needed/used?

Can we just return NULL on platforms that doesn't support their own media
specifies?

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:76: if (!instance_) {
nit: We like to return early:

if (instance_)
  return;

TRACE...
Create()...

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:82:
ui::PlatformObject<MediaOzonePlatform>::Create();
Is it possible to use base/lazy_instance.h for |instance_|?

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:84: // TODO(spang): Currently need to leak
this object.
Is there a bug tracking this?

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.h (right):

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.h:27: // necessary for media also
nit: period at end of sentence.

[commit-bot: I haz the power, 2014-06-10 09:12:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-06-10 11:20:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 11:20:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[vignatti (out of this project), 2014-06-10 14:11:15]

On 2014/06/09 23:53:08, xhwang wrote:
> I am not super familiar with this area, so I read about the CL description and

> http://www.chromium.org/developers/design-documents/ozone trying to understand
> more
> background of this CL. I have a few questions.
> 
> 1, Are we trying to provide a path for other ozone platforms to provide their
> own
> video decoder? Are there decoders all GPU based? (since we are changing
> c/c/gpu/media)

yes, it's all GPU based. Video decoders rely on some windowing system details
and Ozone was originally made for abstract these kind of things.

> 2, For your comment: "implementations can decide themselves whether a video
> implementation
> is actually wanted". How will this be implemented? Will
> CreateVideoDecodeAccelerator() return
> NULL? Or should CreateMediaOzonePlatformXxx just return NULL?

MediaOzonePlatform would encompass all media implementation hw details,
including but not limited to video decode. Say that in the future some Ozone
implementation wants to implement audio but not video, then
CreateVideoDecodeAccelerator could be simply ignored.


> 3: Is there a doc about converting media decoding using ozone? I am also
> interested in any
> plan that converts ChromeOS or ChromeCast to use ozone for video decoding.
> Thanks!

Maybe one thing we were not clear enough here is that if you are using Ozone
then _all_ your windowing aspects are abstracted by it. You cannot use Aura/X11
and at the same time abstract media through say Ozone-Wayland. That's impossible
probably. That said, if you're interested to convert a given target for Ozone
media then it has to be planned as a whole system being transitioned for it and
not only a subsystem.

Thanks for reviewing! I re-wrote now the CL description based on your questions
here. Take a look please, I hope it's clear now.

[vignatti (out of this project), 2014-06-10 14:50:29]

patchset 7 uploaded. PTAL.

https://codereview.chromium.org/269673005/diff/90001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/269673005/diff/90001/media/media.gyp#newcode727
media/media.gyp:727: '<(SHARED_INTERMEDIATE_DIR)',
On 2014/06/09 23:53:33, xhwang wrote:
> Comment about what this is used for.

Done.

https://codereview.chromium.org/269673005/diff/90001/media/media.gyp#newcode737
media/media.gyp:737: 'action_name': 'generate_constructor_list',
On 2014/06/09 23:53:33, xhwang wrote:
> Add a comment about what this generates. Since this is in media/, it's not
> obvious for people without searching for "generate_constructor_list".

Done.

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:29: // internal platforms decide to actually
implement their media specifics
On 2014/06/09 23:53:34, xhwang wrote:
> nit: Punctuation, Spelling and Grammar:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

Done.

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:49: }
On 2014/06/09 23:53:34, xhwang wrote:
> Are these declared by generate_constructor_list.py?
> 
> These creation functions are not used in this CL? Shall we add them later when
> it's actually needed/used?
> 
> Can we just return NULL on platforms that doesn't support their own media
> specifies?
> 

Yes, these are used in this CL and declared by generate_constructor_list.py. If
just return NULL we would be deferring wrongly, per spang@ comment in
https://codereview.chromium.org/269673005/#msg42@

Done.

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:76: if (!instance_) {
On 2014/06/09 23:53:34, xhwang wrote:
> nit: We like to return early:
> 
> if (instance_)
>   return;
> 
> TRACE...
> Create()...

Done.

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:82:
ui::PlatformObject<MediaOzonePlatform>::Create();
On 2014/06/09 23:53:34, xhwang wrote:
> Is it possible to use base/lazy_instance.h for |instance_|?

I think we could but we preferred the other style just. I'm not sure.

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:84: // TODO(spang): Currently need to leak
this object.
On 2014/06/09 23:53:34, xhwang wrote:
> Is there a bug tracking this?

no, sorry.

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.h (right):

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.h:27: // necessary for media also
On 2014/06/09 23:53:34, xhwang wrote:
> nit: period at end of sentence.

Done.

[spang, 2014-06-10 15:24:23]

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:82:
ui::PlatformObject<MediaOzonePlatform>::Create();
On 2014/06/09 23:53:34, xhwang wrote:
> Is it possible to use base/lazy_instance.h for |instance_|?

No, lazy instance can NOT be used when you do not know the concrete type. It
allocates the storage statically.

[spang, 2014-06-10 15:37:40]

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
File media/ozone/media_ozone_platform.cc (right):

https://codereview.chromium.org/269673005/diff/90001/media/ozone/media_ozone_...
media/ozone/media_ozone_platform.cc:84: // TODO(spang): Currently need to leak
this object.
On 2014/06/10 14:50:29, vignatti wrote:
> On 2014/06/09 23:53:34, xhwang wrote:
> > Is there a bug tracking this?
> 
> no, sorry.

crbug.com/380884

[xhwang, 2014-06-10 15:48:07]

Thanks for the explanations. LGTM

[xhwang, 2014-06-10 15:48:58]

BTW, you have two BUG= line in the CL description. Remove the BUG=none one?

[spang, 2014-06-10 15:51:09]

On 2014/06/10 15:48:58, xhwang wrote:
> BTW, you have two BUG= line in the CL description. Remove the BUG=none one?

my fault. fixed.

[vignatti (out of this project), 2014-06-10 16:09:51]

The CQ bit was checked by tiago.vignatti@intel.com

[commit-bot: I haz the power, 2014-06-10 16:14:25]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/269673005/110001

[commit-bot: I haz the power, 2014-06-11 02:24:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-11 05:01:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 05:01:41]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[vignatti (out of this project), 2014-06-11 13:42:38]

The CQ bit was checked by tiago.vignatti@intel.com

[commit-bot: I haz the power, 2014-06-11 13:44:52]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/269673005/110001

[commit-bot: I haz the power, 2014-06-11 20:34:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-11 20:40:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:40:08]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[vignatti (out of this project), 2014-06-12 02:56:03]

The CQ bit was checked by tiago.vignatti@intel.com

[commit-bot: I haz the power, 2014-06-12 02:57:54]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/269673005/110001

[commit-bot: I haz the power, 2014-06-12 06:47:54]

Change committed as 276552

[mcasas, 2015-05-08 23:08:21]

A revert of this CL (patchset #7 id:110001) has been created in
https://codereview.chromium.org/1127343003/ by mcasas@chromium.org.

The reason for reverting is: Not used at the moment.

Agreed with kalyan.kondapally@intel.com,
tiago.vignatti@intel.com and 
joone.hur@intel.com..