Allow CastContentBrowserClient to customise media pipeline device

Allow CastContentBrowserClient to customise media pipeline device

We need a hook to customise how MediaPipelineDevice is created,
orthogonal to the vendor-specific aspect of creation.  Decided to
keep existing factory method for the vendor customisation so there's
no need to change vendor implementations for now (although this
will all get moved into libcast_media before long).  New hook is
in CastContentBrowserClient and can be substituted as the other
Platform* functions already are.

BUG=

Committed: https://crrev.com/1a4282180660fce54726f648ac465afea227109c
Cr-Commit-Position: refs/heads/master@{#333090}

[halliwell, 2015-06-04 16:51:49]

halliwell@chromium.org changed reviewers:
+ erickung@chromium.org, gunsch@chromium.org

[gunsch, 2015-06-04 20:50:28]

I'm fine having a "platform"-specific way to create a MediaPipelineDevice
instance, but... why CastContentBrowserClient? Seems like a lot of this code is
a) just to plumb it all the way back to CCBC and b) causes some abstraction
leaks (the only non-CMA code that "knows about" the CMA backend is
MediaPipelineHost. Codesearch for "chromecast/media/cma/backend
-file:chromecast/media/cma").

[halliwell, 2015-06-04 21:02:58]

On 2015/06/04 20:50:28, gunsch wrote:
> I'm fine having a "platform"-specific way to create a MediaPipelineDevice
> instance, but... why CastContentBrowserClient? Seems like a lot of this code
is
> a) just to plumb it all the way back to CCBC and b) causes some abstraction
> leaks (the only non-CMA code that "knows about" the CMA backend is
> MediaPipelineHost. Codesearch for "chromecast/media/cma/backend
> -file:chromecast/media/cma").

I did this because it's the only place I could see in our current structure that
can do this (CastContentBrowserClient creates CmaMessageFilterHost, which
creates MediaPipelineHost, which creates MediaPipelineDevice).  The other
approach I could see would be to make the switch by just linking in a different
implementation (as the current CreateMediaPipelineDevice does), I'm not a huge
fan of that technique in general, but do you think that would be preferable?  Or
other suggestions?

[halliwell, 2015-06-04 21:24:28]

On 2015/06/04 21:02:58, halliwell wrote:
> On 2015/06/04 20:50:28, gunsch wrote:
> > I'm fine having a "platform"-specific way to create a MediaPipelineDevice
> > instance, but... why CastContentBrowserClient? Seems like a lot of this code
> is
> > a) just to plumb it all the way back to CCBC and b) causes some abstraction
> > leaks (the only non-CMA code that "knows about" the CMA backend is
> > MediaPipelineHost. Codesearch for "chromecast/media/cma/backend
> > -file:chromecast/media/cma").
> 
> I did this because it's the only place I could see in our current structure
that
> can do this (CastContentBrowserClient creates CmaMessageFilterHost, which
> creates MediaPipelineHost, which creates MediaPipelineDevice).  The other
> approach I could see would be to make the switch by just linking in a
different
> implementation (as the current CreateMediaPipelineDevice does), I'm not a huge
> fan of that technique in general, but do you think that would be preferable? 
Or
> other suggestions?

Also, I'm not convinced CMA backend is so much of an implementation detail going
forward as it has been to date - the top-level entry point will most likely be
moving to our chromecast/public APIs.  And we're adding new components which
will interact with these APIs.

[halliwell, 2015-06-04 21:24:30]

On 2015/06/04 21:02:58, halliwell wrote:
> On 2015/06/04 20:50:28, gunsch wrote:
> > I'm fine having a "platform"-specific way to create a MediaPipelineDevice
> > instance, but... why CastContentBrowserClient? Seems like a lot of this code
> is
> > a) just to plumb it all the way back to CCBC and b) causes some abstraction
> > leaks (the only non-CMA code that "knows about" the CMA backend is
> > MediaPipelineHost. Codesearch for "chromecast/media/cma/backend
> > -file:chromecast/media/cma").
> 
> I did this because it's the only place I could see in our current structure
that
> can do this (CastContentBrowserClient creates CmaMessageFilterHost, which
> creates MediaPipelineHost, which creates MediaPipelineDevice).  The other
> approach I could see would be to make the switch by just linking in a
different
> implementation (as the current CreateMediaPipelineDevice does), I'm not a huge
> fan of that technique in general, but do you think that would be preferable? 
Or
> other suggestions?

Also, I'm not convinced CMA backend is so much of an implementation detail going
forward as it has been to date - the top-level entry point will most likely be
moving to our chromecast/public APIs.  And we're adding new components which
will interact with these APIs.

[gunsch, 2015-06-04 22:57:45]

I'm not the biggest fan of linking different implementations in, but to be fair
that's what CastContentBrowserClient is doing right now, both before and
including your change :)

I think there's primarily only one exception to that pattern, and that's
CastService/ChromecastService. As an aside, I'd enjoy having something similar
for many of these public/internal bits, e.g. some sort of a "CastClient"
interface where the actual instance passed in provided you the implementation,
not the linked-in "platform" method. Then we could just have one link-time
method (e.g. CastService::Create) and everything else could be a subclass of a
base interface. That would probably also alleviate a lot of the issues with the
TOT+internal build being broken when submitting N-sided patches.

(I can dream, right?)

In the meantime, I'll buy your explanation about the CMA backend interfaces
being something that can be more public than they currently are. My comment was
largely a mechanical pushback on the expanding of references to it, but I think
I'm OK with it as a deliberate action.

https://codereview.chromium.org/1168643004/diff/1/chromecast/browser/cast_con...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/1168643004/diff/1/chromecast/browser/cast_con...
chromecast/browser/cast_content_browser_client.h:56: // Factory to create a
MediaPipelineDevice.
Comment should describe what the method does, e.g. "Creates a
MediaPipelineDevice to use for playback. Called once per media player instance."

https://codereview.chromium.org/1168643004/diff/1/chromecast/browser/cast_con...
chromecast/browser/cast_content_browser_client.h:57:
scoped_ptr<media::MediaPipelineDevice> PlatformCreateMediaPipelineDevice(
guard with #if !OS_ANDROID

[gunsch, 2015-06-04 22:57:55]

(oh and lgtm % the above small comments)

[halliwell, 2015-06-05 13:54:36]

On 2015/06/04 22:57:45, gunsch wrote:
> I'm not the biggest fan of linking different implementations in, but to be
fair
> that's what CastContentBrowserClient is doing right now, both before and
> including your change :)
> 
Gosh yes, good point :)  I keep mistaking these Platform* functions for virtuals
because they're grouped together in a class.

> I think there's primarily only one exception to that pattern, and that's
> CastService/ChromecastService. As an aside, I'd enjoy having something similar
> for many of these public/internal bits, e.g. some sort of a "CastClient"
> interface where the actual instance passed in provided you the implementation,
> not the linked-in "platform" method. Then we could just have one link-time
> method (e.g. CastService::Create) and everything else could be a subclass of a
> base interface. That would probably also alleviate a lot of the issues with
the
> TOT+internal build being broken when submitting N-sided patches.
> 

Totally agree, I think this would be a much healthier approach :)

> (I can dream, right?)
> 
> In the meantime, I'll buy your explanation about the CMA backend interfaces
> being something that can be more public than they currently are. My comment
was
> largely a mechanical pushback on the expanding of references to it, but I
think
> I'm OK with it as a deliberate action.
> 
>
https://codereview.chromium.org/1168643004/diff/1/chromecast/browser/cast_con...
> File chromecast/browser/cast_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/1168643004/diff/1/chromecast/browser/cast_con...
> chromecast/browser/cast_content_browser_client.h:56: // Factory to create a
> MediaPipelineDevice.
> Comment should describe what the method does, e.g. "Creates a
> MediaPipelineDevice to use for playback. Called once per media player
instance."
> 
Done.

>
https://codereview.chromium.org/1168643004/diff/1/chromecast/browser/cast_con...
> chromecast/browser/cast_content_browser_client.h:57:
> scoped_ptr<media::MediaPipelineDevice> PlatformCreateMediaPipelineDevice(
> guard with #if !OS_ANDROID

Done.

[halliwell, 2015-06-05 13:55:46]

The CQ bit was checked by halliwell@chromium.org to run a CQ dry run

[halliwell, 2015-06-05 13:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1168643004/#ps20001
(title: "Comment updates + Android exclusion")

[commit-bot: I haz the power, 2015-06-05 13:56:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1168643004/20001

[commit-bot: I haz the power, 2015-06-05 14:48:39]

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

[commit-bot: I haz the power, 2015-06-05 14:48:40]

Dry run: This issue passed the CQ dry run.

[gunsch-google, 2015-06-05 16:16:54]

gunsch@google.com changed reviewers:
+ gunsch@google.com

[gunsch-google, 2015-06-05 16:16:55]

lgtm

[halliwell, 2015-06-05 17:40:26]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2015-06-05 17:41:01]

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

[commit-bot: I haz the power, 2015-06-05 17:53:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-05 18:03:47]

Patchset 2 (id:??) landed as
https://crrev.com/1a4282180660fce54726f648ac465afea227109c
Cr-Commit-Position: refs/heads/master@{#333090}