Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo

media/mojo/interfaces/audio_output.mojom

Index: media/mojo/interfaces/audio_output.mojom
diff --git a/media/mojo/interfaces/audio_output.mojom b/media/mojo/interfaces/audio_output.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..4346a6316fe6911293ec03b2e89b915ec41c3494
--- /dev/null
+++ b/media/mojo/interfaces/audio_output.mojom
@@ -0,0 +1,32 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+module media.mojom;
+
+import "media/mojo/interfaces/media_types.mojom";
+
+// This interface handles audio output stream operations.
+// It allows to close a stream.
+// TODO(rchtara): Add methods that allow the interaction with audio output
+// streams: Play, Pause and SetVolume to this interface.
+// See crbug.com/606707 for more details.
+interface AudioOutputStream {
+  Close();
+};
+
+// This interface manages audio output streams.
+// It allows to create an AudioOutputStream.
+// TODO(rchtara): Add method to request a device authorization to this
+// interface.
+// See crbug.com/606707 for more details.
+interface AudioOutput {
+  CreateStream(
+      int32 stream_id,
+      int32 render_frame_id,

[xhwang, 2016/05/03 17:26:13]

Media should not know about render frames (which is in content/ layer). I chat
with rchtara@ about this offline. Basically we have two options here:

1. Move this mojom file to content/ layer so that we can use |render_frame_id|.

2. As I suggested earlier, we can have AudioOutputImpl on the Render frame layer
(hosted in RenderFrameHostImpl). Since we are already at the RenderFrame level,
AudioOutputImpl should
already know about the |render_frame_id|, and we don't need to pass it in this
interface. Then AudioOutputImpl can delegate the real tasks to the
AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have a
getter for AudioRendererHost on RenderProcessHostImpl, not RenderProcessHost. We
should probably just add a method on RenderProcessHost to get the
AudioRendererHost to get this working.

In general I feel passing |render_frame_id| around in IPC is bad. If it's render
frame specific, it should be handled in render frame level. So I prefer the
second option. But since I am not familiar with audio code, I'll defer to other
owners to help make the decision.

+dalecurtis / grunell / tommi: WDYT?

[DaleCurtis, 2016/05/03 21:07:37]

media/ can't know about render frames, it's a content/ level concept, so there's
not much room for discussion. We have to have a content level piece handle the
render frame ids if they're needed, which I think they are at present (for audio
logging, tab mute control, tab audio notifications, oom killer, power save
blocking, etc).

The other option is only to break apart that functionality such that another
piece which lives in the content layer coordinates the needs of associating
render_frame_id+pid to a given AudioOutput.

I'm actually a little confused by the need for stream_id in this CL. I thought
Mojo allowed us to avoid doing any ID tracking at all.

[rchtara, 2016/05/04 08:38:43]

After discussing with grunell, we think that it's probably a good idea to move
audio_output.mojom to the content: the only reason we added the interface in
media is that before, we used the interface in the audio_output_controller.cc/h
in media. But now we don't.
So, the normal place for audio_output.mojom is in content.
stream_id is used for error detection in the renderer and the browser.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Its' also used for logs:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
So, even if we remove it we are going to still need something for identifying
the streams in logs in the browser and renderer.

But for the play, pause and set_volume we are going to use the
AudioOutputStreamPtr without needing to send the stream_id.

[rchtara, 2016/05/04 08:38:43]

After discussing with grunell, we think that it's probably a good idea to move
audio_output.mojom to the content: the only reason we added the interface in
media is that before, we used the interface in the audio_output_controller.cc/h
in media. But now we don't.
So, the normal place for audio_output.mojom is in content.

[rchtara, 2016/05/05 07:32:10]

Another reason for not doing that is that in the current design we need it to
retrieve the callback in AudioRendererHost::DoComeleteCreation:
AudioOutputImpl::CreateStream will send
media::mojom::AudioOutput::CreateStreamCallback as an argument to
audio_renderer_host::CreateStream which will create an AudioEntry and store the
callback inside it. Then, the audio_output_controller created by the AudioEntry 
will call AudioRendererHost::DoCompeleteCreation which is going to use the
stream_id to find the audio_entry and call the right callback.

https://codereview.chromium.org/1930393002/patch/60001/70004

In the previous architecture, we didn't use the stream_id to retrieve the
callback :
the AudioEntry didn't store the callback and just passed it to the controller
then the audio_output_controller sent it back to the AudioRendererHost as an
argument to DoCompleteCreation. This doesn't involve using the stream_id.
However, it requires that the AudioOutputController is involved in Mojo which is
not possible as the AudioOutputController is in media which cannot know about
mojo and this is why we switched to the new architecture.
https://codereview.chromium.org/1896883002/patch/60001/70011
https://codereview.chromium.org/1896883002/patch/60001/70004


So it's hard to remove stream_id. This is going to require a completely new
audio rendering architecture and is going to break many things which is very
risky.
I will leave on the 3rd of June and me and grunell would like to land this CL
and the play/pause/setvolume CL before I leave, and removing the stream_id is
going to require much more time than I have. So, I prefer focusing on mojofying
other stream operations than to work on removing the stream_id.  

[DaleCurtis, 2016/05/05 17:41:57]

Do we need the stream id to be sent between the renderer and browser though?
I.e. it seems like the control / rendering channel shouldn't need a stream_id,
but to avoid leaking mojo details into AudioOutputController we do need the
stream id as you've mentioned. Thus it seems like the mojo endpoint should track
a mapping of MojoPtr => stream_id on the browser side that it hands to other
browser side components.

If you just remove it from the mojom and construct a new mapping on the service
side, does it require as much changes as you were thinking?

[rchtara, 2016/05/06 07:29:47]

If we do so, the id of streams in the log provided by the renderer and the
browser are not going to be the same (they are many of logs in
audio_renderer_host.cc
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
at least one in the audio_message_filter.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...


Also stream_ids are going to be used to detect many kinds of errors in the
browser and the renderer, so if we have different mapping in the renderer and
the browser, all these error mechanisms are going to fail: for example:
*delegate corresponding to stream_id not found in the delegates_ lookup table in
audio_message_filter.cc:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

*entry corresponding to stream_id not found in the audio_entries_ map in
audio_message_filter.cc:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[DaleCurtis, 2016/05/06 17:31:19]

Those stream ids are worthless on the renderer side since they aren't accurate
across processes. If we want to keep a stream id it shouldn't be passed in from
the renderer side, only returned from the browser side (where it can be kept
correct across processes). I'm fine with keeping the stream id in the return,
but it shouldn't be passed in when calling CreateStream().

[rchtara, 2016/05/09 17:22:07]

The stream_id is now generated in the renderer in 
AudioMessageFilter::AudioOutputIPCImpl::CreateStream
A delegate will be created for this stream_id.
Then AudioHostMsg_CreateStream(stream_id_, render_frame_id_, params) is going to
be sent.
Later when AudioMsg_NotifyStreamCreated is back, the stream_id will be used to
retrieve the corresponding delegate.  
If the stream_id is generated in the browser, it's not going to be possible to
find its corresponding delegate when  AudioMsg_NotifyStreamCreated comes.

[DaleCurtis, 2016/05/09 17:45:03]

This doesn't seem correct, IDs are not necessary to identify mojo endpoints.
Presumably we'll still have something like AudioMessageFilter which lives in
content that acts as an MojoAudioOutputFactory. AudioOutputDevice will have some
way of calling Create() which would do something like:

MojoAudioOutputFactory::Create(params) {
  shell::GetInterface<interfaces::AudioOutput>(interface_provider_,
&audio_output_ptr);
  return new MojoAudioOutput(audio_output_ptr);
}

MojoAudioOutput {
  MojoAudioOutput() {
    audio_output_ptr->Create(params, base::Bind(OnCreated, this, ...));
  }

  OnCreated(...) {
  }
}

The AudioOutputDevice would then use this returned MojoAudioOutput object for
general control mechanisms and playback. There is no longer a
AudioMsg_NotifyStreamCreated in this world.

[rchtara, 2016/05/10 16:04:51]

Cool! we are going to remove the stream_ids.
After discussing with Grunell, we made a plan for the project:
1) Create Mojo interfaces needed for switching audio rendering stream creation
and closing from IPC to Mojo.
2) Switch stream creation and closing in Chrome audio rendering from IPC to
Mojo.
3) Switch all other stream operations (request device authorization, play,
setVolume, pause) from IPC to Mojo.
4)Refactor: merge audio_renderer_host.cc and audio_output_impl.cc and merge
audio_message_filter.cc and audio_output_client.cc. Remove the stream_id for the
Createstream arguments, generate it in the browser and return it to the renderer
(it's going to be just used for logging).

As play and pause stream are not going to be mojofied until the step 3, they
will still rely on having the same stream_id in the renderer and the browser. So
we need to keep stream_id the same until we mojofy them.(Otherwise, after the cl
for the step 2 will land and before the cl for the step 3 will land, because at
this time create and close stream are going to be mojofied while play is going
to be still ipc based. So if the stream_ids in the renderer and browser are not
going to be the same, we are going to have trouble playing the right stream when
play ipc come from the renderer to the browser) 

So, this why we are going to do that in the refactoring step: step 4.
Let's keep the interface like that temporarily and remove stream_ids later.

+      media.interfaces.AudioParameters params) =>
+      (int32 stream_id,
+       AudioOutputStream? stream,
+       handle<shared_buffer>? shared_buffer,
+       handle? socket_descriptor);
+};