Pass default renderer factory into CreateMediaRendererFactory

Pass default renderer factory into CreateMediaRendererFactory

This makes renderer creation more flexible by allowing
implementer of CreateMediaRendererFactory to choose which
renderer to use - the default one or the custom one. One
example where this might be useful is for Chromecast, where
we want to choose renderer based on the types of actual
input content streams, e.g. we will use CMA media renderer
for media types that are supported by it (H264, AAC, etc)
and use the default media renderer by media types/codecs
not supported by CMA (FLAC, Opus, etc).

BUG=457959

[servolk, 2015-02-12 01:05:31]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, erickung@chromium.org, gunsch@chromium.org,
lcwu@chromium.org, scherkus@chromium.org, xhwang@chromium.org

[gunsch, 2015-02-12 01:19:35]

can you point to a crbug with any discussion on this?

src/chromecast/ code is simple and lgtm though

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
content/public/renderer/content_renderer_client.cc:183: return factory.Pass();
I think this can just be:

return default_factory.PassAs<media::RendererFactory>();

[servolk, 2015-02-12 02:20:10]

On 2015/02/12 01:19:35, gunsch wrote:
> can you point to a crbug with any discussion on this?
> 
> src/chromecast/ code is simple and lgtm though
> 
>
https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
> File content/public/renderer/content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
> content/public/renderer/content_renderer_client.cc:183: return factory.Pass();
> I think this can just be:
> 
> return default_factory.PassAs<media::RendererFactory>();

Added bug info

[servolk, 2015-02-12 02:20:26]

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
content/public/renderer/content_renderer_client.cc:183: return factory.Pass();
On 2015/02/12 01:19:34, gunsch wrote:
> I think this can just be:
> 
> return default_factory.PassAs<media::RendererFactory>();

Hmm, I can't find scoped_ptr::PassAs method. The only reference I could find is
in some comments for WebRtc's scoped_ptr
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...).
PassAs is not found anywhere under base/
Is that something new that hasn't made it to base/ from webrtc yet or is it
something deprecated?

[gunsch, 2015-02-12 03:18:51]

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
content/public/renderer/content_renderer_client.cc:183: return factory.Pass();
On 2015/02/12 02:20:26, servolk wrote:
> On 2015/02/12 01:19:34, gunsch wrote:
> > I think this can just be:
> > 
> > return default_factory.PassAs<media::RendererFactory>();
> 
> Hmm, I can't find scoped_ptr::PassAs method. The only reference I could find
is
> in some comments for WebRtc's scoped_ptr
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...).
> PassAs is not found anywhere under base/
> Is that something new that hasn't made it to base/ from webrtc yet or is it
> something deprecated?

Oh, I think you can just use Pass now without the downcast step:
https://codereview.chromium.org/655143002

[DaleCurtis, 2015-02-12 19:16:50]

Why can't your content client just instantiate the default factory and return
that?

[servolk, 2015-02-17 18:48:49]

On 2015/02/12 19:16:50, DaleCurtis wrote:
> Why can't your content client just instantiate the default factory and return
> that?

Default factory constructor requires media log, gpu factories and audio hardware
config - we don't have access to those from the content client. So it's easier
to instantiate the default factory from render_frame_impl where we have access
to all those things and pass an instance of the default factory to the
ContentRendererClient::CreateMediaRendererFactory.

[servolk, 2015-02-17 18:54:49]

New patchsets have been uploaded after l-g-t-m from gunsch@chromium.org

[servolk, 2015-02-17 18:55:05]

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/883353012/diff/1/content/public/renderer/cont...
content/public/renderer/content_renderer_client.cc:183: return factory.Pass();
On 2015/02/12 03:18:51, gunsch wrote:
> On 2015/02/12 02:20:26, servolk wrote:
> > On 2015/02/12 01:19:34, gunsch wrote:
> > > I think this can just be:
> > > 
> > > return default_factory.PassAs<media::RendererFactory>();
> > 
> > Hmm, I can't find scoped_ptr::PassAs method. The only reference I could find
> is
> > in some comments for WebRtc's scoped_ptr
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...).
> > PassAs is not found anywhere under base/
> > Is that something new that hasn't made it to base/ from webrtc yet or is it
> > something deprecated?
> 
> Oh, I think you can just use Pass now without the downcast step:
> https://codereview.chromium.org/655143002

Done.

[DaleCurtis, 2015-02-17 19:48:16]

https://codereview.chromium.org/883353012/diff/20001/chromecast/renderer/cast...
File chromecast/renderer/cast_content_renderer_client.cc (right):

https://codereview.chromium.org/883353012/diff/20001/chromecast/renderer/cast...
chromecast/renderer/cast_content_renderer_client.cc:149: return
scoped_ptr<::media::RendererFactory>(
So in some future world you'll pass the default factory into this one and it'll
vend default or cma based renderers based on some criteria?

RendererFactory::CreateRenderer() seemingly doesn't give you enough context to
make that decision. Can you elaborate on what this future code might look like?

[servolk, 2015-02-17 20:00:36]

On 2015/02/17 19:48:16, DaleCurtis wrote:
>
https://codereview.chromium.org/883353012/diff/20001/chromecast/renderer/cast...
> File chromecast/renderer/cast_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/883353012/diff/20001/chromecast/renderer/cast...
> chromecast/renderer/cast_content_renderer_client.cc:149: return
> scoped_ptr<::media::RendererFactory>(
> So in some future world you'll pass the default factory into this one and
it'll
> vend default or cma based renderers based on some criteria?
> 
> RendererFactory::CreateRenderer() seemingly doesn't give you enough context to
> make that decision. Can you elaborate on what this future code might look
like?

In the future I'm going to pass the default_factory to CmaMediaRendererFactory
which will take ownership and keep default_factory alive. When
CmaMediaRendererFactory is asked to create a new renderer it will create an
instance of CMA renderer and an instance of default media renderer (using the
default_factory) and will create a new MediaRenderer proxy object which will
look at the demuxer stream types/codecs upon initialization and will decide
whether we are actually going to use the CMA renderer or the default one
(something like this
https://eureka-internal-review.googlesource.com/#/c/27181/). If it was easier to
instantiate either an instance of default renderer or an instance of default
renderer factory, we could avoid passing them down to the custom factory. But
both require additional input parameters not easily accessible in the content
clients.

[DaleCurtis, 2015-02-17 20:54:38]

Would it make more sense to expose the information you need to create the
default factory to embedders? It seems like at least the media log would be
useful to you. On Chromecast is the GPU factory always empty?

[gunsch, 2015-02-17 20:59:52]

On 2015/02/17 20:54:38, DaleCurtis wrote:
> Would it make more sense to expose the information you need to create the
> default factory to embedders? It seems like at least the media log would be
> useful to you. On Chromecast is the GPU factory always empty?

One related note is that Chromecast doesn't currently depend on media/filters,
only media/base. I gather it's considered not reasonable for embedders to depend
on media/filters, is it reasonable to also consider DefaultRendererFactory part
of the media/base API and that embedders should, in general, be able to create
it?

[DaleCurtis, 2015-02-17 22:06:10]

Hmm, I hadn't heard of anyone saying you shouldn't depend on media/filters. I
think given the way we've structured the render config, it's not unreasonable to
depend on. Do you have any quoted context you can provide for that statement?

[servolk, 2015-02-19 01:00:14]

On 2015/02/17 20:54:38, DaleCurtis wrote:
> Would it make more sense to expose the information you need to create the
> default factory to embedders? It seems like at least the media log would be
> useful to you. On Chromecast is the GPU factory always empty?

Yeah, I guess media log could be useful to embedders in general (although we
typically rely more on regular logging in Chromecast at the moment).
One thing that I would like to have changed around RendererFactory and
DefaultRendererFactory is having consistent interface for creating both, i.e.
both DefaultRendererFactory and the custom RendererFactory constructors ideally
should have matching signatures and the same set of input parameters (how about
we create a MediaRendererFactoryFactory that would facilitate creation of
renderer factories in a consistent manner? j/k).
After looking at the code some more, I think what we could do here is modify the
signature of the renderer factory to expect an input media_log parameter, in
addition to RenderFrame, and the DefaultRendererFactory could obtain the
GpuFactories and audio config directly from the RenderThreadImpl::current(),
instead of passing those explicitly. As far as I understand the
RenderThreadImpl::current() is like a singleton - there's only one per renderer
process. I'll try and see if that works, but let me know if you have better
ideas.

[servolk, 2015-02-19 01:04:43]

On 2015/02/17 20:59:52, gunsch wrote:
> On 2015/02/17 20:54:38, DaleCurtis wrote:
> > Would it make more sense to expose the information you need to create the
> > default factory to embedders? It seems like at least the media log would be
> > useful to you. On Chromecast is the GPU factory always empty?
> 
> One related note is that Chromecast doesn't currently depend on media/filters,
> only media/base. I gather it's considered not reasonable for embedders to
depend
> on media/filters, is it reasonable to also consider DefaultRendererFactory
part
> of the media/base API and that embedders should, in general, be able to create
> it?

Since we depend on media/ anyway via media/base/, is it gonna make any
difference if we started using media/filters/? It sounds to me like the
dependency is already there, it's not like media/base/ and media/filters/ are
built into separate libraries. I guess we could move the default renderer
factory into media/base/ if we really want to avoid explicitly including
media/filters/ but that doesn't really change anything in my opinion.

[DaleCurtis, 2015-02-19 01:34:30]

I don't have a problem with using media/filters if it's just to construct a
custom decoder list or something similar; though I wonder if there might be a
more efficient mechanism. We exposed a RendererConfig type pattern for the
Ensemble folk, which we could share in mainline.

[gunsch, 2015-02-19 01:43:08]

On 2015/02/19 01:34:30, DaleCurtis wrote:
> I don't have a problem with using media/filters if it's just to construct a
> custom decoder list or something similar; though I wonder if there might be a
> more efficient mechanism. We exposed a RendererConfig type pattern for the
> Ensemble folk, which we could share in mainline.

Some quick searching didn't turn anything up in writing, but past work with
damienv@ / xhwang@ seems to recall to me that our top-level principles involved
chromecast not depending on media/filters. But if that doesn't bother y'all, I
don't have a personal objection. 

In general, it does seem light-weight dependency at best:
https://code.google.com/p/chromium/codesearch#search/&q=media/filters%20-file...

[DaleCurtis, 2015-02-19 02:23:04]

I think as a general statement, not depending on media/filters probably makes
sense; there's a lot of assorted stuff in there.

Here's a half-baked idea:
1. RenderConfig'ize the DefaultRendererFactory so that the renderers it
generates only get the decoders you actually want. (Note we already have a TODO
to do this in the default factory).
2. Move DefaultRendererFactory into a new media/renderers directory which can be
DEPS'd.

This would preserve the default factory creation in the frame impl, but allow
you guys to construct normal renderers when necessary.

The reason I dislike the current approach is:
1. You're not actually going to want all the decoders our default factory
returns.
2. It's weird to ask for a factory and give one at the same time...

I'll give this some more thought tomorrow.

[servolk, 2015-02-19 04:01:33]

On 2015/02/19 02:23:04, DaleCurtis wrote:
> I think as a general statement, not depending on media/filters probably makes
> sense; there's a lot of assorted stuff in there.
> 
> Here's a half-baked idea:
> 1. RenderConfig'ize the DefaultRendererFactory so that the renderers it
> generates only get the decoders you actually want. (Note we already have a
TODO
> to do this in the default factory).
> 2. Move DefaultRendererFactory into a new media/renderers directory which can
be
> DEPS'd.
> 
> This would preserve the default factory creation in the frame impl, but allow
> you guys to construct normal renderers when necessary.
> 
> The reason I dislike the current approach is:
> 1. You're not actually going to want all the decoders our default factory
> returns.
> 2. It's weird to ask for a factory and give one at the same time...
> 
> I'll give this some more thought tomorrow.

I guess we could try that as well, but first let me ask a few questions to
better understand your idea, I'm not sure I fully get it.
1. Why are you saying we are not actually going to want all the decoders? I
think that's actually what we do. Ideally we want cast_shell running on
Chromecast to be exactly the same as Chrome in terms of supported features and
codecs, to simplify development and avoid platform fragmentation. It's just that
Chromecast has some specific limitations (limited CPU power due to hardware
constraints), so we have to do some things differently. e.g. we can support
software video decoding, our CPU is too slow for that, so we need to use
hardware decoding, and since unfortunately our hardware platform doesn't support
the usual GPU APIs like VAAPI we have to use the custom hardware decoding APIs
that our hardware vendor provides, which makes the whole CMA thing necessary at
the moment. But if, going forward, we could support regular GPU video decoding
code path (and we might be able to do that with next versions of our hardware
platform, although probably post-v3), that would be ideal.
So again, what we want is playback through CMA renderer for the media types that
our hardware decoder supports (H.264, VP8, AAC, etc) and we want to fall back on
the default Chrome media renderer for everything else. This would allow us to
support media types supported by the default media renderer through software
decoders - FLAC, Opus, ALAC, PCM A-LAW, etc. So we probably want exactly the
same decoders as the default media renderer has.
2. I agree that passing default factory into the CreateRendererFactory method is
weird, but that was only necessary because we can't create the default factory
ourselves at the moment. If we did what I've described in the previous message
(make factory creation signature uniform for the default and custom factories)
then we could instantiate the DefaultRendererFactory inside CMA renderer factory
and wouldn't need to pass the default factory into CreateRendererFactory
anymore.
3. To be honest it's not clear to me what problem would RendererConfig solve for
us. The TODO at
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/defa...
talks about selecting decoders, but given what I've explained in #1 above we
probably want the same set of decoders as the default renderer.
4. Creating media/renderers and moving all the renderer and renderer factory
code there from media/filters/ sounds good like a good idea to me. Renderers are
more like media sinks and less like media filters/tranforms.

[DaleCurtis, 2015-02-19 19:39:41]

Interesting. I assumed you guys wouldn't want any GPU decoders and wouldn't want
say the libvpx video decoder either. If that's not the case, I agree
RenderConfig doesn't make any sense for you.

I don't think we can adapt the renderer factory signature to take a RenderFrame
since that's a content/ construct and the default factory lives in media/

How about the following then:
- Adapt the content/public interface to pass along a MediaLog (you don't need
the GpuFactories today, so those can be skipped for the moment).
- Move all renderers into media/renderers and then allow media/renderers to be
DEPS'd. You can then create the default factory internal to your code.

[servolk, 2015-02-20 01:51:57]

On 2015/02/19 19:39:41, DaleCurtis wrote:
> Interesting. I assumed you guys wouldn't want any GPU decoders and wouldn't
want
> say the libvpx video decoder either. If that's not the case, I agree
> RenderConfig doesn't make any sense for you.
> 
> I don't think we can adapt the renderer factory signature to take a
RenderFrame
> since that's a content/ construct and the default factory lives in media/
> 
> How about the following then:
> - Adapt the content/public interface to pass along a MediaLog (you don't need
> the GpuFactories today, so those can be skipped for the moment).
> - Move all renderers into media/renderers and then allow media/renderers to be
> DEPS'd. You can then create the default factory internal to your code.

Ok, sounds like a plan.
I've created two separate CLs to get things moving:
https://codereview.chromium.org/941633004/
https://codereview.chromium.org/943903002/