IPC->mojo of audio_renderer_host

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
index 895a93e4967727e4d2846679b50433a2de46ff94..e4a5ce3c3b785ef7e022e4dce9693a0990beeadb 100644
--- a/media/mojo/interfaces/audio_output.mojom
+++ b/media/mojo/interfaces/audio_output.mojom
@@ -5,29 +5,54 @@
 module media.mojom;
 
 import "media/mojo/interfaces/audio_parameters.mojom";
+import "url/mojo/origin.mojom";
+
+enum AudioOutputStreamState {
+  PLAYING,
+  PAUSED,
+  ERROR
+};
 
-// 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 {

[o1ka, 2016/09/02 13:07:58]

naming conflict with media::AudioOutputStream. This is a bit confusing. But
probably we can survive...

-  Close();
+  Play();
+  Pause();
+  SetVolume(double volume);
+};
+
+interface AudioOutputStreamClient {
+  // Called by the AudioOutputStreamService after any stream
+  // manipulation by the AudioOutputStreamClient.
+  OnStreamStateChange(AudioOutputStreamState state);

[o1ka, 2016/09/02 13:07:58]

Why not to have OnPlay/OnPause/OnError instead, and get rid of the enum?

 };
 
-// This interface manages audio output streams.
-// It allows to create an AudioOutputStream.
-// TODO(rchtara): Add a method to request device authorization to this
-// interface.
-// See crbug.com/606707 for more details.
+// AudioOutput manages device authorizations and AudioOutputStreamClients

[o1ka, 2016/09/02 13:07:57]

How does it manage AudioOutputStreamClients? There is no API...

 interface AudioOutput {
-  // TODO(rchtara): Remove |stream_id| from AudioOutput::CreateStream when all
-  // the stream operations are mojofied.
+  // Used to request device authorization from the AudioOutputService.
+  // Returns |authorization_id| which will need to be used in CreateStream
+  // if device authorization is successful.
+  RequestDeviceAuthorization(

[o1ka, 2016/09/02 13:07:58]

Make it "CreateStream" which returns a stream interface?

+      int32 stream_id,
+      int32 render_frame_id,
+      int32 session_id,
+      string device_id,
+      url.mojom.Origin origin) =>
+      (int32 state,
+       AudioParameters output_params,
+       string matched_device_id);

[o1ka, 2016/09/02 13:07:58]

(Those three together are OutputDeviceInfo - probably consider using it at some
point instead?)

+
+  // Returns a bound AudioOutputStream that will be used to directly
+  // communicate with an AudioOutputStreamService. It must use the
+  // provided AudioOutputStreamClient to signal errors.
   CreateStream(

[o1ka, 2016/09/02 13:07:58]

Make it a Start() method of AudioOUtputStream interface?

       int32 stream_id,
+      int32 render_frame_id,

[o1ka, 2016/09/02 13:07:57]

Why do we need it? stream id is a unique identifier, isn't it?

+      AudioOutputStreamClient client,
       AudioParameters params) =>
-      (int32 stream_id,
-       AudioOutputStream? stream,
-       handle<shared_buffer>? shared_buffer,
-       handle? socket_descriptor);
-};
+      (AudioOutputStream stream,
+       handle<shared_buffer> shared_buffer,
+       handle socket_descriptor);
+
+  // Closes a (possibly) active AudioOutputStreamService. Clears
+  // authorization data and any associated info with |stream_id|.
+  CloseStream(int32 stream_id);

[o1ka, 2016/09/02 13:07:58]

Should it be called if CreateStream() has been issued before, or does
RequestDeviceAuthorization() also require closing the stream?
If former, than shouldn't it be a method of AudioOutputStream?
If latter, then name it somehow differently?

In any case, if we shift the concept a bit as I mentioned above (make
RequestDeviceAuthorization to return a stream object),this will move to
AudioOutputStream, and AudioOutput will become a pure factory.

+};