Introduce media::RendererFactory interface.

Introduce media::RendererFactory interface.

This class is in charge of creating media::Renderer to be used by the meida
pipeline.

Benefits of this CL:
- We have a central place for media::Renderer creation.
- WebMediaPlayerImpl is much cleaner.
- Creation of MojoRendererImpl is delayed from construction time to load time.

BUG=416570
TEST=Existing tests pass.

Committed: https://crrev.com/6fa3562066174193f79777ed9c6e589a9ca4c434
Cr-Commit-Position: refs/heads/master@{#307808}

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

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

[xhwang, 2014-12-08 18:09:28]

dalecurtis: Please review everything.
scherkus: Please skim the RendererFactory API and naming.

[DaleCurtis, 2014-12-08 22:12:09]

Can you elaborate on your choice of an inheritance pattern versus having a
single RendererFactory which has an EnableExternalRenderer(callback) type
mechanism? It seems even the mojo factory could be created through such a
mechanism; reducing the number of classes and implementations necessary here.

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
File media/filters/default_renderer_factory.cc (right):

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.cc:21: // TODO(xhwang): We are abusing
CreateMediaSourceErrorEvent() in a lot of places
+1 ... though MSE abuses it to report things that aren't errors too :|

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.cc:45: AudioRendererSink*
audio_renderer_sink) {
Probably the sink could be created in the RenderFrame too and dropped from the
WebMediaPlayerParams if you like; there may be some typing issues around
AudioRendererSink vs AudioSourceProvider though...

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.cc:46: LOG(ERROR) << __FUNCTION__;
DVLOG() ?

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.h:35: const
scoped_refptr<base::SingleThreadTaskRunner>&)>
Does this need a parameter? It looks like you could just bind it in at creation
time.

[xhwang, 2014-12-08 22:54:40]

On 2014/12/08 22:12:09, DaleCurtis wrote:
> Can you elaborate on your choice of an inheritance pattern versus having a
> single RendererFactory which has an EnableExternalRenderer(callback) type
> mechanism? It seems even the mojo factory could be created through such a
> mechanism; reducing the number of classes and implementations necessary here.
 
(comments only)

Actually that's what I tried first. It has some issues:
1. That makes dependencies more complicated. For example, HTMLViewer will depend
on DefaultRendererFactory which will then depend on our decoders, e.g.
FFmpegVideoDecoder. But HTMLViewer doesn't need these decoders at all.
2. As part of Spork, different platforms can choose to provide their own
RendererFactory implementation. Most of them don't need a fallback to
media::RendererImpl. Hence, using a default RendererFactory with an
EnableExternalRendererCB doesn't fit well. For example, in the future, we'll
want to be able to provide different media::Renderer in the mojo service as
well, i.e. MojoRendererService will also take a RendererFactory. This factory
will be different for different service providers. For example, if we host this
service in a mojo app, we may use media::RendererImpl with some customized
decoders (using PlatformRendererConfig). If we service the host in the browser
process, we may want to get the RendererFactory from an embedder, e.g.
ContentBrowserClient. See this for more thoughts on this:
https://code.google.com/p/chromium/issues/detail?id=431776#c5

[xhwang, 2014-12-08 22:58:40]

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

https://codereview.chromium.org/783003002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1785:
base::Bind(&ContentRendererClient::CreateMediaRenderer,
Now I feel it makes more sense to change
ContentRendererClient::CreateMediaRenderer() to
ContentRendererClient::GetMediaRendererFactory() as well.

[xhwang, 2014-12-08 23:27:10]

comments addressed

[xhwang, 2014-12-08 23:44:09]

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
File media/filters/default_renderer_factory.cc (right):

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.cc:21: // TODO(xhwang): We are abusing
CreateMediaSourceErrorEvent() in a lot of places
On 2014/12/08 22:12:09, DaleCurtis wrote:
> +1 ... though MSE abuses it to report things that aren't errors too :|

Acknowledged.

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.cc:45: AudioRendererSink*
audio_renderer_sink) {
On 2014/12/08 22:12:09, DaleCurtis wrote:
> Probably the sink could be created in the RenderFrame too and dropped from the
> WebMediaPlayerParams if you like; there may be some typing issues around
> AudioRendererSink vs AudioSourceProvider though...

Actually I like to pass the AudioRendererSink in so that the media::Renderer can
use it to render audio. But we may do this in init time, as we does for
paint_cb. I'd keep it as is and see how it goes...

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.cc:46: LOG(ERROR) << __FUNCTION__;
On 2014/12/08 22:12:09, DaleCurtis wrote:
> DVLOG() ?

oops, debug code that I forgot to remove :) removed.

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/40001/media/filters/default_re...
media/filters/default_renderer_factory.h:35: const
scoped_refptr<base::SingleThreadTaskRunner>&)>
On 2014/12/08 22:12:09, DaleCurtis wrote:
> Does this need a parameter? It looks like you could just bind it in at
creation
> time.

Done.

[DaleCurtis, 2014-12-09 00:37:18]

I like it. lgtm

[xhwang, 2014-12-09 03:55:09]

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

[xhwang, 2014-12-09 03:56:22]

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

[xhwang, 2014-12-09 03:57:13]

jamesr: Please OWNERS review
mojo/services/html_viewer/webmediaplayer_factory.cc.

jam: Please OWNERS review content/*

[xhwang, 2014-12-09 04:14:26]

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

[xhwang, 2014-12-09 04:14:27]

gunsch: Please also take a look at the change in
content/public/renderer/content_renderer_client.h.

[jamesr, 2014-12-09 04:25:01]

mojo/services/html_viewer lgtm

https://codereview.chromium.org/783003002/diff/100001/mojo/services/html_view...
File mojo/services/html_viewer/webmediaplayer_factory.cc (right):

https://codereview.chromium.org/783003002/diff/100001/mojo/services/html_view...
mojo/services/html_viewer/webmediaplayer_factory.cc:76: media_log,
GetMediaThreadTaskRunner(), compositor_task_runner_, NULL);
nullptr

[gunsch, 2014-12-09 04:26:09]

lgtm from Chromecast side, I think the new combination of
ContentRendererClient/MediaRendererFactory is cleaner than before

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
content/public/renderer/content_renderer_client.h:261: virtual
scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
naming nit: I think "Get..." usually implies a single instance, returning a raw
pointer, and ContentRendererClient maintaining ownership; since this is used to
create a new factory with every WMPI (with caller taking ownership), I think
"Create..." is more semantic.

https://codereview.chromium.org/783003002/diff/100001/media/filters/default_r...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/100001/media/filters/default_r...
media/filters/default_renderer_factory.h:20: class MEDIA_EXPORT
DefaultRendererFactory : public RendererFactory {
naming nit: aren't specific implementations of interfaces usually named with the
qualifier afterwards? e.g. RendererFactoryDefault rather than
DefaultRendererFactory. (keeps the related files together)

[jam, 2014-12-09 17:14:19]

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
content/public/renderer/content_renderer_client.h:261: virtual
scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
On 2014/12/09 04:26:09, gunsch wrote:
> naming nit: I think "Get..." usually implies a single instance, returning a
raw
> pointer, and ContentRendererClient maintaining ownership; since this is used
to
> create a new factory with every WMPI (with caller taking ownership), I think
> "Create..." is more semantic.

if this is switched to a factory now, why do we need to create a new factory
each time? seems like the factory should be long lived.

[xhwang, 2014-12-09 17:48:33]

comments addressed

[xhwang, 2014-12-09 18:05:52]

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
content/public/renderer/content_renderer_client.h:261: virtual
scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
On 2014/12/09 04:26:09, gunsch wrote:
> naming nit: I think "Get..." usually implies a single instance, returning a
raw
> pointer, and ContentRendererClient maintaining ownership; since this is used
to
> create a new factory with every WMPI (with caller taking ownership), I think
> "Create..." is more semantic.

Done.

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
content/public/renderer/content_renderer_client.h:261: virtual
scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
On 2014/12/09 17:14:19, jam wrote:
> On 2014/12/09 04:26:09, gunsch wrote:
> > naming nit: I think "Get..." usually implies a single instance, returning a
> raw
> > pointer, and ContentRendererClient maintaining ownership; since this is used
> to
> > create a new factory with every WMPI (with caller taking ownership), I think
> > "Create..." is more semantic.
> 
> if this is switched to a factory now, why do we need to create a new factory
> each time? seems like the factory should be long lived.

I felt using scoped_ptr makes the lifetime/ownership easier to follow. 

For example, if we have a long lived factory, then we need to make sure the
factory is destroyed after |render_frame| is destructed. I don't see an easy way
to do this now. (Unless we add a RenderFrameDeleted() call in this interface?)

Also, we have other places a RendererFactory could be created (e.g. in
RenderFrameImpl). In that case, if we pass along a raw pointer (instead of a
scoped_ptr), we also need to manage the lifetime of that RendererFactory
instance so that it will outlive the consumer of the RendererFactory.

Another option is to use scoped_refptr, which will solve all the issues above.
But in media code we like to avoid scoped_refptr unless necessary, because
scoped_refptr makes it even harder to track the lifetime of an object.

Last, typically the RendererFactory implementation is a pretty small class. So
allocating one for each call shouldn't be a big problem performance-wise...

Let me know what you think of it. Thanks!

https://codereview.chromium.org/783003002/diff/100001/media/filters/default_r...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/100001/media/filters/default_r...
media/filters/default_renderer_factory.h:20: class MEDIA_EXPORT
DefaultRendererFactory : public RendererFactory {
On 2014/12/09 04:26:09, gunsch wrote:
> naming nit: aren't specific implementations of interfaces usually named with
the
> qualifier afterwards? e.g. RendererFactoryDefault rather than
> DefaultRendererFactory. (keeps the related files together)

Hmm, I don't think we are following this model in media/ code. For example, we
have FFmpegVideoDecoder and VpxVideoDecoder...

https://codereview.chromium.org/783003002/diff/100001/mojo/services/html_view...
File mojo/services/html_viewer/webmediaplayer_factory.cc (right):

https://codereview.chromium.org/783003002/diff/100001/mojo/services/html_view...
mojo/services/html_viewer/webmediaplayer_factory.cc:76: media_log,
GetMediaThreadTaskRunner(), compositor_task_runner_, NULL);
On 2014/12/09 04:25:00, jamesr wrote:
> nullptr

Done.

[gunsch, 2014-12-09 18:08:29]

https://codereview.chromium.org/783003002/diff/100001/media/filters/default_r...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/100001/media/filters/default_r...
media/filters/default_renderer_factory.h:20: class MEDIA_EXPORT
DefaultRendererFactory : public RendererFactory {
On 2014/12/09 18:05:52, xhwang wrote:
> On 2014/12/09 04:26:09, gunsch wrote:
> > naming nit: aren't specific implementations of interfaces usually named with
> the
> > qualifier afterwards? e.g. RendererFactoryDefault rather than
> > DefaultRendererFactory. (keeps the related files together)
> 
> Hmm, I don't think we are following this model in media/ code. For example, we
> have FFmpegVideoDecoder and VpxVideoDecoder...

Ah yeah, I guess that's not really consistent codebase-wide. Seems to be more
for platform things (Thing, ThingAndroid, ThingLinux, etc.)

[scherkus (not reviewing), 2014-12-09 22:44:28]

lgtm w/ some nits & q's

https://codereview.chromium.org/783003002/diff/120001/content/public/renderer...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/783003002/diff/120001/content/public/renderer...
content/public/renderer/content_renderer_client.cc:174: return
scoped_ptr<media::RendererFactory>();
nullptr?

https://codereview.chromium.org/783003002/diff/120001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/783003002/diff/120001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:911: media_task_runner_,
audio_source_provider_.get()),
formatting looks strange?

https://codereview.chromium.org/783003002/diff/120001/media/filters/default_r...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/120001/media/filters/default_r...
media/filters/default_renderer_factory.h:26: ~DefaultRendererFactory() final;
do we do final now?

https://codereview.chromium.org/783003002/diff/120001/media/mojo/services/moj...
File media/mojo/services/mojo_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/120001/media/mojo/services/moj...
media/mojo/services/mojo_renderer_factory.h:18: ~MojoRendererFactory() final;
ditto for final

[xhwang, 2014-12-09 23:11:21]

comments addressed

[xhwang, 2014-12-09 23:12:04]

https://codereview.chromium.org/783003002/diff/120001/content/public/renderer...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/783003002/diff/120001/content/public/renderer...
content/public/renderer/content_renderer_client.cc:174: return
scoped_ptr<media::RendererFactory>();
On 2014/12/09 22:44:28, scherkus wrote:
> nullptr?

Done.

https://codereview.chromium.org/783003002/diff/120001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/783003002/diff/120001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:911: media_task_runner_,
audio_source_provider_.get()),
On 2014/12/09 22:44:28, scherkus wrote:
> formatting looks strange?

Agreed! Sadly this is the taste of clang format. Fixed manually.

https://codereview.chromium.org/783003002/diff/120001/media/filters/default_r...
File media/filters/default_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/120001/media/filters/default_r...
media/filters/default_renderer_factory.h:26: ~DefaultRendererFactory() final;
On 2014/12/09 22:44:28, scherkus wrote:
> do we do final now?

We do! https://chromium-cpp.appspot.com/

https://codereview.chromium.org/783003002/diff/120001/media/mojo/services/moj...
File media/mojo/services/mojo_renderer_factory.h (right):

https://codereview.chromium.org/783003002/diff/120001/media/mojo/services/moj...
media/mojo/services/mojo_renderer_factory.h:18: ~MojoRendererFactory() final;
On 2014/12/09 22:44:28, scherkus wrote:
> ditto for final

ditto

[xhwang, 2014-12-10 04:42:40]

jam: kindly ping...

[jam, 2014-12-10 20:14:46]

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
content/public/renderer/content_renderer_client.h:261: virtual
scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
On 2014/12/09 18:05:51, xhwang wrote:
> On 2014/12/09 17:14:19, jam wrote:
> > On 2014/12/09 04:26:09, gunsch wrote:
> > > naming nit: I think "Get..." usually implies a single instance, returning
a
> > raw
> > > pointer, and ContentRendererClient maintaining ownership; since this is
used
> > to
> > > create a new factory with every WMPI (with caller taking ownership), I
think
> > > "Create..." is more semantic.
> > 
> > if this is switched to a factory now, why do we need to create a new factory
> > each time? seems like the factory should be long lived.
> 
> I felt using scoped_ptr makes the lifetime/ownership easier to follow. 
> 
> For example, if we have a long lived factory, then we need to make sure the
> factory is destroyed after |render_frame| is destructed. I don't see an easy
way
> to do this now. (Unless we add a RenderFrameDeleted() call in this interface?)

Is this factory per render frame? or is there one factory per renderer process?
It seems the latter? if so, then we don't need to worry about lifetime.

> 
> Also, we have other places a RendererFactory could be created (e.g. in
> RenderFrameImpl). In that case, if we pass along a raw pointer (instead of a
> scoped_ptr), we also need to manage the lifetime of that RendererFactory
> instance so that it will outlive the consumer of the RendererFactory.

well in general, factories in the renderer process live for the lifetime of the
process so raw pointer is fine.

> 
> Another option is to use scoped_refptr, which will solve all the issues above.
> But in media code we like to avoid scoped_refptr unless necessary, because
> scoped_refptr makes it even harder to track the lifetime of an object.
> 
> Last, typically the RendererFactory implementation is a pretty small class. So
> allocating one for each call shouldn't be a big problem performance-wise...
> 
> Let me know what you think of it. Thanks!

[xhwang, 2014-12-10 20:22:55]

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
content/public/renderer/content_renderer_client.h:261: virtual
scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
On 2014/12/10 20:14:46, jam wrote:
> On 2014/12/09 18:05:51, xhwang wrote:
> > On 2014/12/09 17:14:19, jam wrote:
> > > On 2014/12/09 04:26:09, gunsch wrote:
> > > > naming nit: I think "Get..." usually implies a single instance,
returning
> a
> > > raw
> > > > pointer, and ContentRendererClient maintaining ownership; since this is
> used
> > > to
> > > > create a new factory with every WMPI (with caller taking ownership), I
> think
> > > > "Create..." is more semantic.
> > > 
> > > if this is switched to a factory now, why do we need to create a new
factory
> > > each time? seems like the factory should be long lived.
> > 
> > I felt using scoped_ptr makes the lifetime/ownership easier to follow. 
> > 
> > For example, if we have a long lived factory, then we need to make sure the
> > factory is destroyed after |render_frame| is destructed. I don't see an easy
> way
> > to do this now. (Unless we add a RenderFrameDeleted() call in this
interface?)
> 
> Is this factory per render frame? or is there one factory per renderer
process?
> It seems the latter? if so, then we don't need to worry about lifetime.

For now, the factory is per render frame. We may be able to get rid of it in the
future, but not now.
> 
> > 
> > Also, we have other places a RendererFactory could be created (e.g. in
> > RenderFrameImpl). In that case, if we pass along a raw pointer (instead of a
> > scoped_ptr), we also need to manage the lifetime of that RendererFactory
> > instance so that it will outlive the consumer of the RendererFactory.
> 
> well in general, factories in the renderer process live for the lifetime of
the
> process so raw pointer is fine.

ditto
> 
> > 
> > Another option is to use scoped_refptr, which will solve all the issues
above.
> > But in media code we like to avoid scoped_refptr unless necessary, because
> > scoped_refptr makes it even harder to track the lifetime of an object.
> > 
> > Last, typically the RendererFactory implementation is a pretty small class.
So
> > allocating one for each call shouldn't be a big problem performance-wise...
> > 
> > Let me know what you think of it. Thanks!
>

[jam, 2014-12-10 21:54:50]

On 2014/12/10 20:22:55, xhwang wrote:
>
https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
> File content/public/renderer/content_renderer_client.h (right):
> 
>
https://codereview.chromium.org/783003002/diff/100001/content/public/renderer...
> content/public/renderer/content_renderer_client.h:261: virtual
> scoped_ptr<media::RendererFactory> GetMediaRendererFactory(
> On 2014/12/10 20:14:46, jam wrote:
> > On 2014/12/09 18:05:51, xhwang wrote:
> > > On 2014/12/09 17:14:19, jam wrote:
> > > > On 2014/12/09 04:26:09, gunsch wrote:
> > > > > naming nit: I think "Get..." usually implies a single instance,
> returning
> > a
> > > > raw
> > > > > pointer, and ContentRendererClient maintaining ownership; since this
is
> > used
> > > > to
> > > > > create a new factory with every WMPI (with caller taking ownership), I
> > think
> > > > > "Create..." is more semantic.
> > > > 
> > > > if this is switched to a factory now, why do we need to create a new
> factory
> > > > each time? seems like the factory should be long lived.
> > > 
> > > I felt using scoped_ptr makes the lifetime/ownership easier to follow. 
> > > 
> > > For example, if we have a long lived factory, then we need to make sure
the
> > > factory is destroyed after |render_frame| is destructed. I don't see an
easy
> > way
> > > to do this now. (Unless we add a RenderFrameDeleted() call in this
> interface?)
> > 
> > Is this factory per render frame? or is there one factory per renderer
> process?
> > It seems the latter? if so, then we don't need to worry about lifetime.
> 
> For now, the factory is per render frame. We may be able to get rid of it in
the
> future, but not now.

the factory could be global and the factory method could take a renderframe
pointer?
do you have code uploaded somewhere which shows how this will be used? it's
always hard to review cls with new APIs but no consumers.

> > 
> > > 
> > > Also, we have other places a RendererFactory could be created (e.g. in
> > > RenderFrameImpl). In that case, if we pass along a raw pointer (instead of
a
> > > scoped_ptr), we also need to manage the lifetime of that RendererFactory
> > > instance so that it will outlive the consumer of the RendererFactory.
> > 
> > well in general, factories in the renderer process live for the lifetime of
> the
> > process so raw pointer is fine.
> 
> ditto
> > 
> > > 
> > > Another option is to use scoped_refptr, which will solve all the issues
> above.
> > > But in media code we like to avoid scoped_refptr unless necessary, because
> > > scoped_refptr makes it even harder to track the lifetime of an object.
> > > 
> > > Last, typically the RendererFactory implementation is a pretty small
class.
> So
> > > allocating one for each call shouldn't be a big problem
performance-wise...
> > > 
> > > Let me know what you think of it. Thanks!
> >

[xhwang, 2014-12-10 22:05:11]

> the factory could be global and the factory method could take a renderframe
> pointer?
> do you have code uploaded somewhere which shows how this will be used? it's
> always hard to review cls with new APIs but no consumers.

This is currently used by chromecast. The reason we need this is because the
BrowserCdmManager needs the render frame ID to identify a CDM:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

gunsch: Do you have code that shows how your specific Renderer is created?

[gunsch, 2014-12-10 22:51:32]

On 2014/12/10 22:05:11, xhwang wrote:
> > the factory could be global and the factory method could take a renderframe
> > pointer?
> > do you have code uploaded somewhere which shows how this will be used? it's
> > always hard to review cls with new APIs but no consumers.
> 
> This is currently used by chromecast. The reason we need this is because the
> BrowserCdmManager needs the render frame ID to identify a CDM:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> 
> gunsch: Do you have code that shows how your specific Renderer is created?

Two reasons:

1) We currently assume a single RenderFrame and use CONTROL IPC messages instead
of ROUTED ones. We'd like to move to ROUTED messages but haven't had time. Once
we do, it would likely take a similar structure to RendererCdmManager, e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
and need RenderFrame* reference for the same reason

2) To propagate the SetCdm call
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/rendere...)
from renderer side to browser side and then to get the CDM
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...),
as xhwang@ pointed out, requires at the very least the render_frame_id
associated with the given WebMediaPlayer.

[jam, 2014-12-10 23:20:33]

On 2014/12/10 22:51:32, gunsch wrote:
> On 2014/12/10 22:05:11, xhwang wrote:
> > > the factory could be global and the factory method could take a
renderframe
> > > pointer?
> > > do you have code uploaded somewhere which shows how this will be used?
it's
> > > always hard to review cls with new APIs but no consumers.
> > 
> > This is currently used by chromecast. The reason we need this is because the
> > BrowserCdmManager needs the render frame ID to identify a CDM:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> > 
> > gunsch: Do you have code that shows how your specific Renderer is created?
> 
> Two reasons:
> 
> 1) We currently assume a single RenderFrame and use CONTROL IPC messages
instead
> of ROUTED ones. We'd like to move to ROUTED messages but haven't had time.
Once
> we do, it would likely take a similar structure to RendererCdmManager, e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> and need RenderFrame* reference for the same reason
> 
> 2) To propagate the SetCdm call
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/rendere...)
> from renderer side to browser side and then to get the CDM
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...),
> as xhwang@ pointed out, requires at the very least the render_frame_id
> associated with the given WebMediaPlayer.

i see, so if there was one factory, it would be awkward for media code which
doesn't know about this info to pass it up to the factory. lgtm then

[xhwang, 2014-12-10 23:22:59]

On 2014/12/10 23:20:33, jam wrote:
> On 2014/12/10 22:51:32, gunsch wrote:
> > On 2014/12/10 22:05:11, xhwang wrote:
> > > > the factory could be global and the factory method could take a
> renderframe
> > > > pointer?
> > > > do you have code uploaded somewhere which shows how this will be used?
> it's
> > > > always hard to review cls with new APIs but no consumers.
> > > 
> > > This is currently used by chromecast. The reason we need this is because
the
> > > BrowserCdmManager needs the render frame ID to identify a CDM:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> > > 
> > > gunsch: Do you have code that shows how your specific Renderer is created?
> > 
> > Two reasons:
> > 
> > 1) We currently assume a single RenderFrame and use CONTROL IPC messages
> instead
> > of ROUTED ones. We'd like to move to ROUTED messages but haven't had time.
> Once
> > we do, it would likely take a similar structure to RendererCdmManager, e.g.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> > and need RenderFrame* reference for the same reason
> > 
> > 2) To propagate the SetCdm call
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/rendere...)
> > from renderer side to browser side and then to get the CDM
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...),
> > as xhwang@ pointed out, requires at the very least the render_frame_id
> > associated with the given WebMediaPlayer.
> 
> i see, so if there was one factory, it would be awkward for media code which
> doesn't know about this info to pass it up to the factory. lgtm then

Thanks for the review. In the future I plan to switch the
RenderCdmManager/BrowserCdmManager to mojo, and hopefully we'll be
able to get rid of this render frame ID. Then we can just have a per process
factory and use a raw pointer here.

[xhwang, 2014-12-10 23:23:09]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-12-10 23:24:55]

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

[commit-bot: I haz the power, 2014-12-11 00:44:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-12-11 00:46:00]

Patchset 8 (id:??) landed as
https://crrev.com/6fa3562066174193f79777ed9c6e589a9ca4c434
Cr-Commit-Position: refs/heads/master@{#307808}