Add media Remoter mojo interface and RendererWMPDelegate accessor.

Add media Remoter mojo interface and RendererWMPDelegate accessor.

This change introduces the media remoting mojo interface, which handles
control messages and bitstream data flow between a media element in the
render process and the Media Router in the browser process. The
"RemoterFactory" client/service is instantiated lazily when the first
media element is created in a render frame.

Note that, while new code has been added by this change, there are no
new active code paths yet. Later changes will fill in the surrounding
implementation.

[miu, 2016-07-27 02:15:46]

miu@chromium.org changed reviewers:
+ dalecurtis@chromium.org, erickung@chromium.org

[miu, 2016-07-27 02:15:47]

Eric/Dale: PTAL.

[miu, 2016-07-27 02:15:53]

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

[commit-bot: I haz the power, 2016-07-27 02:16:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 02:24:44]

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

[commit-bot: I haz the power, 2016-07-27 02:24:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[miu, 2016-07-27 06:45:48]

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

[commit-bot: I haz the power, 2016-07-27 06:46:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2016-07-27 06:48:56]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-07-27 06:49:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2016-07-27 06:57:39]

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

[commit-bot: I haz the power, 2016-07-27 06:57:40]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[miu, 2016-07-27 07:13:01]

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

[commit-bot: I haz the power, 2016-07-27 07:13:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 09:19:38]

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

[commit-bot: I haz the power, 2016-07-27 09:19:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[DaleCurtis, 2016-07-27 18:01:14]

Can you give an example of how the remoter will be used by WMPI?

[miu, 2016-07-27 20:51:29]

On 2016/07/27 18:01:14, DaleCurtis wrote:
> Can you give an example of how the remoter will be used by WMPI?

The Remoter, provided by the delegate, is a per-render frame service. On the
other hand, there will be one RemoterClient (a service instance in the render
process) per WMPI. The idea is that, upon instantiation of a WMPI, a
RemoterClient will also be instantiated and register itself with the Remoter.

So, in a later change, the ctor/dtor of WMPI would have additional code like the
following:

  WebMediaPlayerImpl(...) {
    Remoter* remoter = delegate_.get() ? delegate_->GetRemoter() : nullptr;
    if (remoter) {
      RemoterClientPtr client_ptr;
      remoter_client_.reset(new RemoterClientImpl(delegate_id_, remoter,
GetProxy(&client_ptr)));
      remoter->RegisterClient(delegate_id_, std::move(client_ptr));
    }
  }

  ~WebMediaPlayerImpl() {
    delegate_->GetRemoter()->DeregisterClient(delegate_id_);
    remoter_client_.reset();
  }

Where RemoterClientPtr is a mojo interface pointer, and the RemoterClientImpl
constructor would bind to a mojo interface request (3rd arg). Here, I'm using
|delegate_id_| as a unique identifier for the client (it will be passed as the
|client_id| to the all the Remoter method calls).

What's missing from this change is RemoterClientImpl, which is something
erickung@ will be introducing in a later change. It'll be responsible for: 1)
deciding whether/when to remote media, 2) orchestrating the pipeline pause +
media::Renderer switcheroo + pipeline resume, and 3) dispatching messages
(media::Renderer et al RPCs) in both directions.

Eric: Feel free to correct any of this or elaborate. :)

[DaleCurtis, 2016-07-27 21:09:58]

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

[DaleCurtis, 2016-07-27 21:09:59]

+rockot from mojo/OWNERS for general mojo advice on interface design. I think
the way this is currently designed is as "IPC as Mojo", versus being designed
for Mojo. Advice either way from mojo owners would be beneficial for all :)

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
File media/mojo/interfaces/remoter.mojom (right):

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
media/mojo/interfaces/remoter.mojom:11: RegisterClient(uint32 client_id,
RemoterClient client);
Mojo is typically a way to avoid needing id's like this. RemoterClient can
already be uniquely identified implicitly via |client| without the id I thought?

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
media/mojo/interfaces/remoter.mojom:28: (bool success, uint32 audio_pipe_id,
uint32 video_pipe_id);
Again, generally we should be able to avoid ids by returning actual objects.

[miu, 2016-07-27 22:25:54]

On 2016/07/27 21:09:59, DaleCurtis wrote:
> +rockot from mojo/OWNERS for general mojo advice on interface design. I think
> the way this is currently designed is as "IPC as Mojo", versus being designed
> for Mojo. Advice either way from mojo owners would be beneficial for all :)

Yes, this was intended to be an IPC mechanism between renderer and browser
process, and NOT a "mojo media services" client/server implementation.

> Mojo is typically a way to avoid needing id's like this. RemoterClient can
> already be uniquely identified implicitly via |client| without the id I
thought?

Agreed. Let's postpone further review on this until I eliminate the need for the
IDs. I have some ideas in-mind, but will need to try them out on my local code
repo where I've been prototyping everything.

[Ken Rockot(use gerrit already), 2016-07-27 22:31:15]

https://codereview.chromium.org/2189533002/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2189533002/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:71:
render_frame()->GetRemoteInterfaces()->GetInterface(GetProxy(&remoter_));
nit: for convenience you can also just GetInterface(&remoter_)

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
File media/mojo/interfaces/remoter.mojom (right):

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
media/mojo/interfaces/remoter.mojom:7: interface Remoter {
Rather than use a client ID, an idiomatic design would have a unique Remoter
pipe per client. In this model, your interfaces could look more like:

interface RemoterProvider {
  GetRemoter(Remoter& request, RemoterClient client);
};

interface PipeControl {
  // Assuming the signature here is wrong, but I'm mimicking
  // SendBufferToRemote from your CL
  ForwardBufferToRemote(int32 num_bytes);

  DiscardPendingBuffers();
};

interface Remoter {
  // NOTE: need for Deregister() - the client can simply close its
  // Remoter pipe to signal that it's done using it. The impl side
  // can trivially detect this and treat it as a deregistration.

  // Any reason for the pipe handles to really be optional?
  // Also the impl could signal failure by simply closing the Remoter
  // pipe and the client can detect this. There's usually no need for
  // a bool "success/fail" reply.
  StartMedia(handle<data_pipe_consumer> audio_pipe,
             PipeControl& audio_pipe_control,
             handle<data_pipe_consumer> video_pipe,
             PipeControl& video_pipe_control);
  StopMedia(string? reason);

  SendMessageToRemote(array<uint8> message);
};

// RemoterClient looks good as-is

[miu, 2016-08-03 00:38:34]

On 2016/07/27 22:31:15, Ken Rockot wrote:
>
https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
> media/mojo/interfaces/remoter.mojom:7: interface Remoter {
> Rather than use a client ID, an idiomatic design would have a unique Remoter
> pipe per client. In this model, your interfaces could look more like:

Thanks for the suggestion. I've gone this route, and eliminated all the IDs.

Still working on the next patch, though. Please stay tuned...  :)

[miu, 2016-09-02 22:02:18]

Description was changed from

==========
Add media Remoter mojo interface and RendererWMPDelegate accessor.

This change introduces the media remoting mojo interface, which handles
control messages and bitstream data flow between a media element in the
render process and the Media Router in the browser process. The
"Remoter" client/service is instantiated lazily when the first media
element is created in a render frame.

Note that, while new code has been added by this change, none of it will
be executed. Later changes will fill in the surrounding implementation.
==========

to

==========
Add media Remoter mojo interface and RendererWMPDelegate accessor.

This change introduces the media remoting mojo interface, which handles
control messages and bitstream data flow between a media element in the
render process and the Media Router in the browser process. The
"RemoterFactory" client/service is instantiated lazily when the first
media element is created in a render frame.

Note that, while new code has been added by this change, there are no
new active code paths yet. Later changes will fill in the surrounding
implementation.
==========

[miu, 2016-09-02 22:13:29]

Hi all. Back again! :)

I've taken the Mojo interface comments into account, among other things. Also,
there were concerns that those reviewing the Mojo interfaces wanted to also see
the surrounding code using those interfaces.

Therefore, I'm going to re-open a new code review shortly that widens the scope
of the change, and that more clearly shows how these interfaces will be used to
connect things end-to-end.

Replies on prior outstanding comments:

https://codereview.chromium.org/2189533002/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2189533002/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:71:
render_frame()->GetRemoteInterfaces()->GetInterface(GetProxy(&remoter_));
On 2016/07/27 22:31:15, Ken Rockot wrote:
> nit: for convenience you can also just GetInterface(&remoter_)

Acknowledged. (Not making changes to WMPIDelegate in current code.)

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
File media/mojo/interfaces/remoter.mojom (right):

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
media/mojo/interfaces/remoter.mojom:7: interface Remoter {
On 2016/07/27 22:31:15, Ken Rockot wrote:
> Rather than use a client ID, an idiomatic design would have a unique Remoter
> pipe per client. In this model, your interfaces could look more like:
> 
> ...

Great ideas. I reworked the Mojo interfaces here pretty much as you suggested
(and there were some other name/interface changes made in the meantime too).
Suffice it to say, I've eliminated all the use of IDs.

https://codereview.chromium.org/2189533002/diff/40001/media/mojo/interfaces/r...
media/mojo/interfaces/remoter.mojom:11: RegisterClient(uint32 client_id,
RemoterClient client);
On 2016/07/27 21:09:59, DaleCurtis wrote:
> Mojo is typically a way to avoid needing id's like this. RemoterClient can
> already be uniquely identified implicitly via |client| without the id I
thought?

Done. (See reply to rockot's comment above.)