Remove RenderThreadImpl dependencies from WebMediaPlayerImpl.

Remove RenderThreadImpl dependencies from WebMediaPlayerImpl.

This change replaces RenderThreadImpl calls in WebMediaPlayerImpl with
calls on the WebMediaPlayerParams passed into the constructor. This
removes another content/ depencency so that this code can be moved to
media/blink.

The EncryptedMediaSupport creation was moved to RenderThreadImpl and
passed via WebMediaPlayerParams to avoid linking problems that would
occur if WebMediaPlayerImpl was in media/blink.

BUG=408338
Committed: https://crrev.com/755d12d77ef044472b56a11b4ae3f5ffad30bb09
Cr-Commit-Position: refs/heads/master@{#292741}

[acolwell GONE FROM CHROMIUM, 2014-08-28 17:23:33]

acolwell@chromium.org changed reviewers:
+ jamesr@chromium.org, scherkus@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-08-28 17:23:33]

jamesr: OWNERS for content/renderer/render_frame_impl.cc changes
scherkus: Everything else.

[scherkus (not reviewing), 2014-08-28 18:36:12]

few questions 'n stuff

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/encrypted_media_player_support.h (left):

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support.h:25: // method
must always return a valid pointer.
is the plan to move this to media/ as well?

if so ... I'm curious how we plan to support the EME methods. Will we have a
stub implementation of this, or return NULL, or ???

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_params.h (right):

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_params.h:29: // Parameters may be null.
is this still true?

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_params.h:75: return
encrypted_media_player_support_.Pass();
Having this be the only method that actually passes ownership seems odd,
especially since it looks like WMPI can create a EMPS with a client pointer. It
looks like it's the main cause of changing WMPP to a scoped_ptr<>

I was wondering whether it makes sense to either: 
 a) require media_blink embedders to provide an implementation of
EncryptedMediaPlayerSupport::Create() that gets resolved at link-time
 b) change WMPP to accept a EMPS-creating callback/function pointer and expose a
CreateEMPS() method

[acolwell GONE FROM CHROMIUM, 2014-08-28 19:07:28]

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/encrypted_media_player_support.h (left):

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support.h:25: // method
must always return a valid pointer.
On 2014/08/28 18:36:12, scherkus wrote:
> is the plan to move this to media/ as well?
Yes.

> 
> if so ... I'm curious how we plan to support the EME methods. Will we have a
> stub implementation of this, or return NULL, or ???
The plan is to have a stub implementation so that we don't have to have null
checks everywhere.

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_params.h (right):

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_params.h:29: // Parameters may be null.
On 2014/08/28 18:36:12, scherkus wrote:
> is this still true?
No. Updated comment to indicate which ones can be null.

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_params.h:75: return
encrypted_media_player_support_.Pass();
On 2014/08/28 18:36:12, scherkus wrote:
> Having this be the only method that actually passes ownership seems odd,
> especially since it looks like WMPI can create a EMPS with a client pointer.
It
> looks like it's the main cause of changing WMPP to a scoped_ptr<>

Yeah. Not super happy about this.

> 
> I was wondering whether it makes sense to either: 
>  a) require media_blink embedders to provide an implementation of
> EncryptedMediaPlayerSupport::Create() that gets resolved at link-time

This is what the original code did, but it causes linking problems because it
means media/blink depends on content/, which then depends on media/blink. :/

>  b) change WMPP to accept a EMPS-creating callback/function pointer and expose
a
> CreateEMPS() method

I think this is a reasonable compromise, but changes this code params object
from essentially being inert to containing behavior.

[jamesr, 2014-08-28 19:12:03]

I don't think I know enough about this to provide much useful input, but here's
a stamp for r_f_i which looks fine: lgtm.  Feel free to ask if you need help
with linking or whatnot.

https://codereview.chromium.org/517003002/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/517003002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.h:67: const AudioHardwareConfig* const
hardware_params);
nit: seems like this can't be null, const ref?

[scherkus (not reviewing), 2014-08-28 19:12:15]

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_params.h (right):

https://codereview.chromium.org/517003002/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_params.h:75: return
encrypted_media_player_support_.Pass();
On 2014/08/28 19:07:28, acolwell wrote:
> On 2014/08/28 18:36:12, scherkus wrote:
> > Having this be the only method that actually passes ownership seems odd,
> > especially since it looks like WMPI can create a EMPS with a client pointer.
> It
> > looks like it's the main cause of changing WMPP to a scoped_ptr<>
> 
> Yeah. Not super happy about this.
> 
> > 
> > I was wondering whether it makes sense to either: 
> >  a) require media_blink embedders to provide an implementation of
> > EncryptedMediaPlayerSupport::Create() that gets resolved at link-time
> 
> This is what the original code did, but it causes linking problems because it
> means media/blink depends on content/, which then depends on media/blink. :/
> 
> >  b) change WMPP to accept a EMPS-creating callback/function pointer and
expose
> a
> > CreateEMPS() method
> 
> I think this is a reasonable compromise, but changes this code params object
> from essentially being inert to containing behavior.

Yeah ... I'm fine leaving this roughly as they are now as WMPP will likely
evolve/split into a more proper embedder interface over time.

For example, most of these parameters are static for a process' lifetime. Only
AudioRendererSink, MediaLog, and EMPS are created per-WMPI.

[acolwell GONE FROM CHROMIUM, 2014-08-28 20:55:51]

scherkus: PTAL

https://codereview.chromium.org/517003002/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/517003002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.h:67: const AudioHardwareConfig* const
hardware_params);
On 2014/08/28 19:12:03, jamesr wrote:
> nit: seems like this can't be null, const ref?

Done.

[DaleCurtis, 2014-08-28 21:01:06]

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

[DaleCurtis, 2014-08-28 21:01:06]

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

https://codereview.chromium.org/517003002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1576:
render_thread->GetAudioRendererMixerManager()->CreateInput(
Note, this will create and start an AudioOutputDevice thread.  If no player is
forthcoming after a few seconds, it will pause the sink, but the thread will
remain.

https://codereview.chromium.org/517003002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1578:
*render_thread->GetAudioHardwareConfig(),
Note, this is a synchronous IPC to the browser.

[DaleCurtis, 2014-08-28 21:02:39]

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

https://codereview.chromium.org/517003002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1576:
render_thread->GetAudioRendererMixerManager()->CreateInput(
On 2014/08/28 21:01:06, DaleCurtis wrote:
> Note, this will create and start an AudioOutputDevice thread.  If no player is
> forthcoming after a few seconds, it will pause the sink, but the thread will
> remain.

Durr, disregard that, it's not started until ARI calls Start() for the first
time.

[scherkus (not reviewing), 2014-08-28 21:04:04]

lgtm w/ nit

https://codereview.chromium.org/517003002/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/517003002/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:173:
params.CreateEncryptedMediaPlayerSupport(client).Pass()),
I *think* you can drop .Pass() if the function is returning a scoped_ptr<>

[acolwell GONE FROM CHROMIUM, 2014-08-28 21:15:05]

https://codereview.chromium.org/517003002/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/517003002/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:173:
params.CreateEncryptedMediaPlayerSupport(client).Pass()),
On 2014/08/28 21:04:04, scherkus wrote:
> I *think* you can drop .Pass() if the function is returning a scoped_ptr<>

Done.

[acolwell GONE FROM CHROMIUM, 2014-08-28 21:15:25]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-28 21:16:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/517003002/60001

[acolwell GONE FROM CHROMIUM, 2014-08-28 21:59:56]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-28 22:00:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/517003002/80001

[commit-bot: I haz the power, 2014-08-28 23:24:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-28 23:29:04]

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

[commit-bot: I haz the power, 2014-08-28 23:29:05]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[acolwell GONE FROM CHROMIUM, 2014-08-29 01:25:24]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-29 01:26:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/517003002/80001

[commit-bot: I haz the power, 2014-08-29 01:35:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-29 01:49:14]

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

[commit-bot: I haz the power, 2014-08-29 01:49:15]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[acolwell GONE FROM CHROMIUM, 2014-08-29 21:14:52]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-30 01:09:59]

Committed patchset #6 (id:100001) as 4955068fa9b32e203f922492cafbe2bb20bd771c

[commit-bot: I haz the power, 2014-09-10 03:12:35]

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