media: Support MojoRendererService in the browser process.

media: Support MojoRendererService in the browser process.

On some platforms, we run part of the media pipeline in the browser process. For example, on ChromeCast and on Android,  audio/video buffers are decoded in the browser process. Currently, there are some ad-hoc IPC mechanism implemented for these platforms to support hosting  the decoders in the browser process. This CL aims at providing a common mojo based path to support this scenario.

This CL makes it possible to host a mojo::MeidaRenderer service in the browser process. By default, it's using media::RendererImpl and some default audio/video decoders. In later CLs, I'll update this so that we can support embedder specific media::Renderer implementation in MojoRendererService.

Note that media playback is NOT working yet due to DataPipe is not working out-of-process.

Test steps:
1, Change "enable_media_mojo_renderer" to true in media/media_options.gni.
2, Build a GN build of Chrome.
3, Play <video> in Chrome.

BUG=431776
TEST=See above test steps.

Committed: https://crrev.com/e6117333d61fe759bcecd8e6cab927236ce3da79
Cr-Commit-Position: refs/heads/master@{#313463}

[xhwang, 2014-12-15 21:32:30]

https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1193:
GetServiceRegistry()->AddService<mojo::MediaRenderer>(
Currently we don't want to provide this service by default on all platforms,
e.g. on Desktop. Maybe we should have a flag or ifdef for this.

https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1819:
base::Unretained(GetServiceRegistry()))));
ditto, we need a way (flag or ifdef) to figure out when to choose
DefaultRendererFactory, and when to choose MojoRendererFactory.

https://codereview.chromium.org/712463004/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/712463004/diff/20001/media/media.gyp#newcode1072
media/media.gyp:1072: # GN version: //media/mojo/interfaces
Sigh.... These are brought back because content/browser and content/renderer are
still using gyp.

[xhwang, 2014-12-15 21:48:57]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org, jamesr@chromium.org

[xhwang, 2014-12-15 21:48:57]

PTAL

This CL is not complete, but is ready for partial-review/discussion.

One part that's missing is the selection between browser based media Renderer
(MojoRendererFactory) and the default media Renderer (DefaultRendererFactory).
In HTMLViewer we are using a flag (--enable-mojo-media-renderer) for this. Maybe
we can do the same for Chromium, at least for now.

Also, I tested this and it's not fully working, I got an error (DCHECK) that a
data pipe consumer handle passed over to the browser process is invalid. I
"guess" this is because DataPipe doesn't work out-of-process yet.

[17706:17740:1213/220523:ERROR:transport_data.cc(171)] Failed to serialize
handle to remote message pipe
......
[17665:17665:1213/220524:FATAL:mojo_demuxer_stream_adapter.cc(74)] Check failed:
pipe.is_valid().

Let me know what your opinions are. Thanks!

[DaleCurtis, 2014-12-15 23:21:17]

https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1193:
GetServiceRegistry()->AddService<mojo::MediaRenderer>(
On 2014/12/15 21:32:30, xhwang wrote:
> Currently we don't want to provide this service by default on all platforms,
> e.g. on Desktop. Maybe we should have a flag or ifdef for this.

I think an EXTERNAL_MEDIA_RENDERER define would work well. Alternatively if
GetServiceRegistry() is safe to call from other places, you can call it from a
more media appropriate call site. Maybe long term the service registry should do
something like PathService where each module can register their own services
locally and have them globally initialized automatically.

[xhwang, 2014-12-16 00:23:17]

On 2014/12/15 23:21:17, DaleCurtis wrote:
>
https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
> content/browser/frame_host/render_frame_host_impl.cc:1193:
> GetServiceRegistry()->AddService<mojo::MediaRenderer>(
> On 2014/12/15 21:32:30, xhwang wrote:
> > Currently we don't want to provide this service by default on all platforms,
> > e.g. on Desktop. Maybe we should have a flag or ifdef for this.
> 
> I think an EXTERNAL_MEDIA_RENDERER define would work well. Alternatively if
> GetServiceRegistry() is safe to call from other places, you can call it from a
> more media appropriate call site. Maybe long term the service registry should
do
> something like PathService where each module can register their own services
> locally and have them globally initialized automatically.

The plan is to:
1. Use RendererFactory in MojoRendererService so that we don't always use
media::RendererImpl in MojoRendererService.
2. Have something like a ContentBrowserClient::GetRendererFactory() method so
that embedders like ChromeCast can provide their own RendererFactory.
3. If GetRendererFactory() returns null, connecting to the mojo::MediaRenderer
service will fail.

But I am still not sure whether we should always try to connect to
mojo::MediaRenderer service. On desktop this will always fail, and we might just
be wasting time/resources for most of our users. That's where I want to
introduce some flag, or macro as Dale suggested. But previously jamesr@ doesn't
like macros in the HTMLViewer, so I wonder what's the best option here....
+jamesr

[jamesr, 2014-12-16 00:25:15]

> But I am still not sure whether we should always try to connect to
mojo::MediaRenderer service.

connect from where?

[xhwang, 2014-12-16 00:34:22]

On 2014/12/16 00:25:15, jamesr wrote:
> > But I am still not sure whether we should always try to connect to
> mojo::MediaRenderer service.
> 
> connect from where?

Connect from the render process (RenderFrameImpl::createMediaPlayer):
https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_...

Basically the media pipeline in the render process needs an media renderer. This
could be provided by the media pipeline by default (which runs in the render
process). This is the case on Desktop. But it could also be provided by a mojo
MediaRenderer service in the browser process, this will be the case for
ChromeCast/Android in the future. We need some way to choose which way to go:
1. Fallback upon error: always try to connect to mojo service first; upon
failure, fallback to default media::Renderer. I am worrying that doing this on
desktop for every media playback might have some performance cost. But it's just
my wild guess.
2. Run time flag: If --enable-mojo-media-renderer is specified, use
MojoMediaRenderer, otherwise, use default media::Renderer. This is what we are
doing in HTMLViewer.
3. Static macro as Dale suggested.

WDYT?

[jamesr, 2014-12-16 00:42:15]

Where is the code that has to make this decision?

[jamesr, 2014-12-16 00:43:22]

Code in //content will never link into html_viewer and vice versa, so it seems
like if you make the decision about which way you connect there then this is not
a problem.

[xhwang, 2014-12-16 00:49:47]

On 2014/12/16 00:43:22, jamesr wrote:
> Code in //content will never link into html_viewer and vice versa, so it seems
> like if you make the decision about which way you connect there then this is
not
> a problem.

I mentioned html_viewer only as an example. This code is in Chromium and has
nothing to do with html_viewer.

I need to make the decision right here:
https://codereview.chromium.org/712463004/diff/20001/content/renderer/render_...

As you can see in the current CL, MojoRendererFactory (l.1817) always overrides
DefaultRendererFactory (l.1812). This is not right/complete. I need a way to
decide which one to use on a given platform.

[jamesr, 2014-12-16 00:53:44]

That code's already full to the gills of platform-specific #ifdefs. Whether that
bit of code is configured with #ifdefs vs other mechanisms is up to the relevant
OWNERS (which I'm not one of).

[xhwang, 2014-12-16 04:09:54]

On 2014/12/16 00:53:44, jamesr wrote:
> That code's already full to the gills of platform-specific #ifdefs. Whether
that
> bit of code is configured with #ifdefs vs other mechanisms is up to the
relevant
> OWNERS (which I'm not one of).

That makes sense. I guess I'll start with #ifdefs and see what the OWNERS feel
about it. Thanks!

[xhwang, 2014-12-16 07:09:57]

rebase only

[qsr, 2014-12-16 08:59:22]

On 2014/12/16 00:23:17, xhwang wrote:
> On 2014/12/15 23:21:17, DaleCurtis wrote:
> >
>
https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
> > File content/browser/frame_host/render_frame_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/712463004/diff/20001/content/browser/frame_ho...
> > content/browser/frame_host/render_frame_host_impl.cc:1193:
> > GetServiceRegistry()->AddService<mojo::MediaRenderer>(
> > On 2014/12/15 21:32:30, xhwang wrote:
> > > Currently we don't want to provide this service by default on all
platforms,
> > > e.g. on Desktop. Maybe we should have a flag or ifdef for this.
> > 
> > I think an EXTERNAL_MEDIA_RENDERER define would work well. Alternatively if
> > GetServiceRegistry() is safe to call from other places, you can call it from
a
> > more media appropriate call site. Maybe long term the service registry
should
> do
> > something like PathService where each module can register their own services
> > locally and have them globally initialized automatically.
> 
> The plan is to:
> 1. Use RendererFactory in MojoRendererService so that we don't always use
> media::RendererImpl in MojoRendererService.
> 2. Have something like a ContentBrowserClient::GetRendererFactory() method so
> that embedders like ChromeCast can provide their own RendererFactory.

 For providing mojo service from the embedders, we already have:
virtual void OverrideRenderProcessMojoServices(ServiceRegistry* registry)
in public/browser/content_browser_client.h. Will not this work for you?

> 3. If GetRendererFactory() returns null, connecting to the mojo::MediaRenderer
> service will fail.
> 
> But I am still not sure whether we should always try to connect to
> mojo::MediaRenderer service. On desktop this will always fail, and we might
just
> be wasting time/resources for most of our users. That's where I want to
> introduce some flag, or macro as Dale suggested. But previously jamesr@
doesn't
> like macros in the HTMLViewer, so I wonder what's the best option here....
> +jamesr

[DaleCurtis, 2014-12-16 20:59:20]

Since content depends on media you should be able to accomplish your define with
an all_dependent_configs / direct_dependent_config setup instead of adding them
to build/.  Run "gn help all_dependent_configs" for the GN version. Look for
examples in media.gyp for direct_dependent_config.

https://codereview.chromium.org/712463004/diff/80001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/712463004/diff/80001/build/config/BUILD.gn#ne...
build/config/BUILD.gn:57: defines += [ "ENABLE_MOJO_MEDIA_RENDERER" ]
Just use =1 now then :)

[xhwang, 2014-12-17 18:06:05]

> > The plan is to:
> > 1. Use RendererFactory in MojoRendererService so that we don't always use
> > media::RendererImpl in MojoRendererService.
> > 2. Have something like a ContentBrowserClient::GetRendererFactory() method
so
> > that embedders like ChromeCast can provide their own RendererFactory.
> 
>  For providing mojo service from the embedders, we already have:
> virtual void OverrideRenderProcessMojoServices(ServiceRegistry* registry)
> in public/browser/content_browser_client.h. Will not this work for you?

That'd work, and an embedder can use this to override the mojo MediaRenderer
service entirely. But for most embedders, it'll be easier to provide a C++
implementation than to provide a mojo service. Also, we could have multiple
embedders using this (e.g. Android and ChromeCast), putting mojo service hosting
code together and hide it from embedders can help reduce code duplication.

[xhwang, 2014-12-17 19:28:22]

On 2014/12/16 20:59:20, DaleCurtis wrote:
> Since content depends on media you should be able to accomplish your define
with
> an all_dependent_configs / direct_dependent_config setup instead of adding
them
> to build/.  Run "gn help all_dependent_configs" for the GN version. Look for
> examples in media.gyp for direct_dependent_config.

Hmm, currently content_browser and content_renderer are still using gypi instead
of GN. So they are not depending on any media GN target yet. So I guess I can't
do this at this moment? Or do you want me to do something similar in gyp as
well?
 
> https://codereview.chromium.org/712463004/diff/80001/build/config/BUILD.gn
> File build/config/BUILD.gn (right):
> 
>
https://codereview.chromium.org/712463004/diff/80001/build/config/BUILD.gn#ne...
> build/config/BUILD.gn:57: defines += [ "ENABLE_MOJO_MEDIA_RENDERER" ]
> Just use =1 now then :)

I wasn't sure because it's "should PROBABLY be", and we have a lot of these in
that file....

[DaleCurtis, 2014-12-17 19:30:04]

On 2014/12/17 19:28:22, xhwang wrote:
> On 2014/12/16 20:59:20, DaleCurtis wrote:
> > Since content depends on media you should be able to accomplish your define
> with
> > an all_dependent_configs / direct_dependent_config setup instead of adding
> them
> > to build/.  Run "gn help all_dependent_configs" for the GN version. Look for
> > examples in media.gyp for direct_dependent_config.
> 
> Hmm, currently content_browser and content_renderer are still using gypi
instead
> of GN. So they are not depending on any media GN target yet. So I guess I
can't
> do this at this moment? Or do you want me to do something similar in gyp as
> well?

Yes, it's something you can do with both GN and GYP. "all_dependent_configs" is
the GN version, while "direct_dependent_config" is the GYP version. 

>  
> > https://codereview.chromium.org/712463004/diff/80001/build/config/BUILD.gn
> > File build/config/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/712463004/diff/80001/build/config/BUILD.gn#ne...
> > build/config/BUILD.gn:57: defines += [ "ENABLE_MOJO_MEDIA_RENDERER" ]
> > Just use =1 now then :)
> 
> I wasn't sure because it's "should PROBABLY be", and we have a lot of these in
> that file....

[xhwang, 2014-12-17 22:54:36]

Patchset #6 (id:100001) has been deleted

[xhwang, 2014-12-17 22:59:13]

> > Hmm, currently content_browser and content_renderer are still using gypi
> instead
> > of GN. So they are not depending on any media GN target yet. So I guess I
> can't
> > do this at this moment? Or do you want me to do something similar in gyp as
> > well?
> 
> Yes, it's something you can do with both GN and GYP. "all_dependent_configs"
is
> the GN version, while "direct_dependent_config" is the GYP version. 

I guess you mean direct_dependent_settings. Let me try that.

At the same time, please hold off reviewing this CL for now while I am
experimenting
the gyp/gn magics.

[xhwang, 2014-12-18 17:08:27]

It turned out I CAN do this in GN! So I dropped the gyp changes which is painful
to make it clean. Now we only support the mojo::MediaRenderer service in the
browser process in GN build, which should be fine for our development. If we
ever have an embedder requiring a gyp build, we can always add gyp support later
(or ask the embedder to switch to GN ;)

PTAnotherL!

[jamesr, 2014-12-18 17:49:55]

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:15: config("media_mojo_dependent_config") {
nit: i would name this after what it does, not how it's used.

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:16: defines = []
nit: you don't have to have this, you can instead just say "defines = [ ... ] "
inside the if block

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:52: all_dependent_configs = [
":media_mojo_dependent_config" ]
all_dependent_configs is bad - you'll set this define in lots of targets that
have nothing to do with this.  use public_configs and and, where necessary,
forward_dependent_configs_from to forward this as needed

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:79: all_dependent_configs = [
":media_mojo_dependent_config" ]
ditto

[DaleCurtis, 2014-12-18 19:25:11]

https://codereview.chromium.org/712463004/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/712463004/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:488: class MediaRendererServiceProvider
Is this something we want to live in RenderFrameImpl?

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:52: all_dependent_configs = [
":media_mojo_dependent_config" ]
On 2014/12/18 17:49:54, jamesr wrote:
> all_dependent_configs is bad - you'll set this define in lots of targets that
> have nothing to do with this.  use public_configs and and, where necessary,
> forward_dependent_configs_from to forward this as needed

Can you elaborate on how this will pollute non-dependent targets?

[jamesr, 2014-12-18 19:26:18]

Non-directly-dependent targets.  Targets 50 build edges downstream that never
see any of your headers will still have this define.

[xhwang, 2014-12-18 21:22:10]

comments addressed

[xhwang, 2014-12-18 21:22:38]

Thanks for the comments! Please take a look again.

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:15: config("media_mojo_dependent_config") {
On 2014/12/18 17:49:54, jamesr wrote:
> nit: i would name this after what it does, not how it's used.

Done.

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:16: defines = []
On 2014/12/18 17:49:54, jamesr wrote:
> nit: you don't have to have this, you can instead just say "defines = [ ... ]
"
> inside the if block

Done.

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:52: all_dependent_configs = [
":media_mojo_dependent_config" ]
On 2014/12/18 17:49:54, jamesr wrote:
> all_dependent_configs is bad - you'll set this define in lots of targets that
> have nothing to do with this.  use public_configs and and, where necessary,
> forward_dependent_configs_from to forward this as needed

Good point! Thanks!

https://codereview.chromium.org/712463004/diff/120001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:79: all_dependent_configs = [
":media_mojo_dependent_config" ]
On 2014/12/18 17:49:54, jamesr wrote:
> ditto

Done.

[jamesr, 2014-12-18 21:25:19]

GN files lgtm

[DaleCurtis, 2014-12-18 21:34:02]

https://codereview.chromium.org/712463004/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/712463004/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:488: class MediaRendererServiceProvider
On 2014/12/18 19:25:11, DaleCurtis_OOO_Until_Jan5 wrote:
> Is this something we want to live in RenderFrameImpl?

Don't see this one resolved or discussed.

[DaleCurtis, 2014-12-18 22:26:53]

dalecurtis@chromium.org changed reviewers:
+ ddorwin@chromium.org

[DaleCurtis, 2014-12-18 22:26:53]

Heading OOO, so this lgtm % resolution of the RenderFrameImpl comments. I defer
to ddorwin@ if radical changes are necessary.

[xhwang, 2014-12-18 22:48:06]

comments addressed

[xhwang, 2014-12-18 22:48:30]

DaleCurtis: PTAnotherL, thanks!

https://codereview.chromium.org/712463004/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/712463004/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:488: class MediaRendererServiceProvider
On 2014/12/18 21:34:02, DaleCurtis_OOO_Until_Jan5 wrote:
> On 2014/12/18 19:25:11, DaleCurtis_OOO_Until_Jan5 wrote:
> > Is this something we want to live in RenderFrameImpl?
> 
> Don't see this one resolved or discussed.

Sorry, I missed this one. Moved out to content/renderer/media.

[xhwang, 2014-12-18 22:55:46]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org

[xhwang, 2014-12-18 22:55:47]

jam: Please OWNERS review content/ changes. Thanks!

[xhwang, 2015-01-02 19:38:53]

rebase only

[xhwang, 2015-01-05 18:19:48]

On 2015/01/02 19:38:53, xhwang wrote:
> rebase only

jam: kindly ping...

[xhwang, 2015-01-08 17:53:25]

xhwang@chromium.org changed reviewers:
+ creis@chromium.org

[xhwang, 2015-01-08 17:53:26]

It seems jam@ isn't available.

creis: Could you please OWNERS review changes in content/ (excluding
content/renderer/media/*) ?

[Charlie Reis, 2015-01-08 20:55:08]

creis@chromium.org changed reviewers:
+ darin@chromium.org

[Charlie Reis, 2015-01-08 20:55:09]

[+darin]

Hmm, I probably don't have the context here, but the overall direction concerns
me.  I wouldn't think that we want to encourage renderer services to run in the
browser process, where they aren't sandboxed.  I'm aware that it's done for some
features on some platforms for practical reasons, but it's not clear to me that
we want to encourage that further.

I'll defer to Darin's review here, since he's probably a better reviewer for the
Mojo aspects anyway.

[xhwang, 2015-01-08 21:02:10]

On 2015/01/08 20:55:09, Charlie Reis wrote:
> [+darin]
> 
> Hmm, I probably don't have the context here, but the overall direction
concerns
> me.  I wouldn't think that we want to encourage renderer services to run in
the
> browser process, where they aren't sandboxed.  I'm aware that it's done for
some
> features on some platforms for practical reasons, but it's not clear to me
that
> we want to encourage that further.
> 
> I'll defer to Darin's review here, since he's probably a better reviewer for
the
> Mojo aspects anyway.

As described in the CL description, we are already running media
decoding/rendering code in the browser process on ChromeCast and Android, via
IPC. This work is only trying to replace the IPC part with mojo so that we can
clean up a lot of ugly code.

I agree that running these code in the browser process is concerning, but the
future direction of the architecture of ChromeCast and Chrome for Android seems
to be beyond the scope of this work.

[jam, 2015-01-27 17:00:21]

I have similar questions as Charlie, but if this is behind a flag, and is used
just for media stuff, lgtm.

[xhwang, 2015-01-28 05:33:46]

rebase only

[xhwang, 2015-01-28 05:37:30]

On 2015/01/27 17:00:21, jam wrote:
> I have similar questions as Charlie, but if this is behind a flag, and is used
> just for media stuff, lgtm.

thanks for the review!

[xhwang, 2015-01-28 07:52:37]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-01-28 07:53:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/712463004/220001

[commit-bot: I haz the power, 2015-01-28 08:51:47]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-01-28 08:52:43]

Patchset 11 (id:??) landed as
https://crrev.com/e6117333d61fe759bcecd8e6cab927236ce3da79
Cr-Commit-Position: refs/heads/master@{#313463}