Add content API CreateVideoEncodeAccelerator.

Add content API CreateVideoEncodeAccelerator.

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

[hshi1, 2014-01-11 02:16:52]

Alpha - this is not meant to be landed but can you take a look at the header and
confirm if this is what you're looking for?

We may have a hard time to land this due to the DEPS check (see bug report).

[Ami GONE FROM CHROMIUM, 2014-01-11 04:08:22]

drive-by

On 2014/01/11 02:16:52, hshi1 wrote:
> Alpha - this is not meant to be landed but can you take a look at the header
and
> confirm if this is what you're looking for?
> 
> We may have a hard time to land this due to the DEPS check (see bug report).

In go/rgxtk (google-internal only, sorry) jam@ clarifies that
content/public/renderer can depend on media, and that it's only that there
hasn't been a need for it so far that prevents media from already being there. 
I think you should be fine with this.

[hshi1, 2014-01-13 18:51:14]

+fischman (OWNERS of dependencies added to DEPS: +media/video). Thanks!

[Ami GONE FROM CHROMIUM, 2014-01-13 19:42:51]

LGTM from a media perspective.

[hshi1, 2014-01-13 19:44:37]

+piman@ PTAL - Need one more OWNERS bit for content/public. Thanks!

[piman, 2014-01-13 20:32:50]

+jam

Can you describe why you need this in the public content/ API, that is, what are
the scenarios that you're thinking to use this for, from the Chrome side.

We have to look very carefully at the API we add to content/public, because we
have to support them in a stable way. You will note that we currently don't
depend on media/ from the public API, and adding this dependency could be a
scary thought.

Depending on the use case, it may be better served by doing more inside of
content.

[hshi1, 2014-01-13 20:36:14]

On 2014/01/13 20:32:50, piman wrote:
> +jam
> 
> Can you describe why you need this in the public content/ API, that is, what
are
> the scenarios that you're thinking to use this for, from the Chrome side.
> 
> We have to look very carefully at the API we add to content/public, because we
> have to support them in a stable way. You will note that we currently don't
> depend on media/ from the public API, and adding this dependency could be a
> scary thought.
> 
> Depending on the use case, it may be better served by doing more inside of
> content.

The API is requested by hclam@ from the Cast team. The cast code is under
media/cast/ and runs in the renderer process, but it needs an API to obtain a
VideoEncodeAccelerator instance for the current renderer.

[Alpha Left Google, 2014-01-13 20:38:06]

> The API is requested by hclam@ from the Cast team. The cast code is under
> media/cast/ and runs in the renderer process, but it needs an API to obtain a
> VideoEncodeAccelerator instance for the current renderer.

This API is needed by Cast Streaming Extensions API to perform hardware video
encoding in Chrome. The API obtains video frame from another public content API
named MediaStreamVideoSink.

The API looks good.

[jam, 2014-01-13 20:41:40]

On 2014/01/13 20:36:14, hshi1 wrote:
> On 2014/01/13 20:32:50, piman wrote:
> > +jam
> > 
> > Can you describe why you need this in the public content/ API, that is, what
> are
> > the scenarios that you're thinking to use this for, from the Chrome side.
> > 
> > We have to look very carefully at the API we add to content/public, because
we
> > have to support them in a stable way.

Nit: the content api isn't stable. we do review changes to the public api
carefully to ensure there are no layering violations, features aren't
implemented across modules (i.e. content and chrome) where it could be
implemented inside just once, avoiding duplication in the public api etc.

> > You will note that we currently don't
> > depend on media/ from the public API, and adding this dependency could be a
> > scary thought.

btw the deps rule for content/public just reflected what we needed so far. if we
do need to add something lower-level than content in there, that's fine with me
(we had a private thread about this)

> > 
> > Depending on the use case, it may be better served by doing more inside of
> > content.
> 
> The API is requested by hclam@ from the Cast team. The cast code is under
> media/cast/ and runs in the renderer process, but it needs an API to obtain a
> VideoEncodeAccelerator instance for the current renderer.

can you point us at at the code which actually calls this? it's often hard to
review changes to the public API without seeing how they'll be used.

[hshi1, 2014-01-13 22:01:57]

On 2014/01/13 20:41:40, jam wrote:
> On 2014/01/13 20:36:14, hshi1 wrote:
> > On 2014/01/13 20:32:50, piman wrote:
> > > +jam
> > > 
> > > Can you describe why you need this in the public content/ API, that is,
what
> > are
> > > the scenarios that you're thinking to use this for, from the Chrome side.
> > > 
> > > We have to look very carefully at the API we add to content/public,
because
> we
> > > have to support them in a stable way.
> 
> Nit: the content api isn't stable. we do review changes to the public api
> carefully to ensure there are no layering violations, features aren't
> implemented across modules (i.e. content and chrome) where it could be
> implemented inside just once, avoiding duplication in the public api etc.
> 
> > > You will note that we currently don't
> > > depend on media/ from the public API, and adding this dependency could be
a
> > > scary thought.
> 
> btw the deps rule for content/public just reflected what we needed so far. if
we
> do need to add something lower-level than content in there, that's fine with
me
> (we had a private thread about this)
> 
> > > 
> > > Depending on the use case, it may be better served by doing more inside of
> > > content.
> > 
> > The API is requested by hclam@ from the Cast team. The cast code is under
> > media/cast/ and runs in the renderer process, but it needs an API to obtain
a
> > VideoEncodeAccelerator instance for the current renderer.
> 
> can you point us at at the code which actually calls this? it's often hard to
> review changes to the public API without seeing how they'll be used.

The cast code which actually calls this still is in development and hasn't
landed which is why we need to land the API first. But hclam@ can probably
provide a pointer to the work in progress.

[hshi1, 2014-01-14 17:54:10]

Alpha: ping - could you please rpoint us at the code that calls this? Thanks.

[Alpha Left Google, 2014-01-14 20:16:54]

On 2014/01/14 17:54:10, hshi1 wrote:
> Alpha: ping - could you please rpoint us at the code that calls this? Thanks.

We're going to pass in VideoEncodeAccelerator at this line:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me...

and the definition is here:

https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_se...

A VideoEncodeAccelerator in renderer is implemented by using IPCs in content.
Hence a public content API is needed to create such object.

[hshi1, 2014-01-15 22:10:45]

jam/piman - ping. Did Alpha answer your questions? Thanks.

[jam, 2014-01-16 16:44:52]

lgtm

[commit-bot: I haz the power, 2014-01-16 17:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/98183007/190005

[commit-bot: I haz the power, 2014-01-17 00:49:10]

Change committed as 245397