IPC->mojo of audio_renderer_host

Work in progress!

BUG=

[Max Morin, 2016-08-29 08:06:32]

Description was changed from

==========
Rebase Ameyas CL. DO NOT SUBMIT.

BUG=
==========

to

==========
Rebase Ameyas CL. DO NOT SUBMIT.

BUG=
==========

[Max Morin, 2016-09-01 10:42:39]

Description was changed from

==========
Rebase Ameyas CL. DO NOT SUBMIT.

BUG=
==========

to

==========
Work in progress!

BUG=
==========

[Max Morin, 2016-09-01 10:42:39]

maxmorin@chromium.org changed reviewers:
+ grunell@chromium.org, olka@chromium.org

[Max Morin, 2016-09-01 10:59:03]

Olga, Henrik: If you want, you can take a look now. Note that not all code makes
sense or is correct yet

[Henrik Grunell, 2016-09-01 15:09:07]

Some high level comments. You should involve Dale and perhaps Tommi before we
start reviewing implementation details.

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:138: class
AudioRendererHost::AudioEntry
Maybe this should be renamed, but let's settle the design first.

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

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.cc:21: #include
"media/media_features.h"
Needed?

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.cc:48:
factory->CreateAudioOutputIPC(render_frame_id), factory->io_task_runner(),
Could AudioOutputDevice access the stream directly?

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
File content/renderer/media/audio_output_ipc_factory.h (right):

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_output_ipc_factory.h:33: // AudioOutputIPCFactory
creates AudioOutputIPC objects that proxy calls to
It seems like the main purpose for AudioOutputIPC is to map between stream id
and AudioOutputDevice, but if we get rid of the id (we should at some point) it
should not be needed. Is there any other need for it?

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/render...
content/renderer/render_thread_impl.cc:719: #if BUILDFLAG(ENABLE_MOJO_AUDIO)
Why do we have this build flag? (I know it came from the original CL.)

https://codereview.chromium.org/2289543003/diff/40001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2289543003/diff/40001/media/BUILD.gn#newcode24
media/BUILD.gn:24: # Flags for Mojo audio.
Same here, what will these be used for?

https://codereview.chromium.org/2289543003/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:291: case
mojom::AudioOutputStreamState::PLAYING:
This seems to be an old IPC legacy. Play() and Pause() (and SetVolume()?) should
send a response instead. I don't think there are any cases where we could not
send a response that replaces usage of the "state changed" function.

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:25:
OnStreamStateChange(AudioOutputStreamState state);
I believe we can replace this with responses. Play and pause for sure. Some
(all?) of the errors could probably be signaled by simply deleting the
AudioOutputStream object, which the other side will be informed of.

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:34: int32 stream_id,
A follow-up CL should remove the stream id, but it requires some analysis of how
to deal with the authorization requests. It could be a good idea to do that
already now.

Historic not: Originally it was planned to be removed when 
RequestDeviceAuthorization was added to this interface. Hence it's existence in
CreateStream in the interface.

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:48: int32 render_frame_id,
Do you know what happened with the discussion around having a service per render
frame? Seems like the design is back to one service per render process.

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:57: CloseStream(int32 stream_id);
This should not be needed. Deleting the AudioOutputStream when closing will
afaik signal to the other side.

[Henrik Grunell, 2016-09-01 15:11:31]

Also, as Olga mentioned, I think a before and after object diagram and maybe
call flow would be good to have to make discussions easier. There is something
in a previous design doc to look at.

[o1ka, 2016-09-02 07:31:47]

Just started looking into it, some random comments for the beginning :)

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:204:
media::mojom::AudioOutputStreamClientPtr output_stream_client;
output_stream_client_

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:219:
CreateStreamCallback callback,
const &

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:248:
base::Bind(&AudioRendererHost::OnCloseStream, host_, stream_id_));
|host_| is a raw pointer which AudioEntry does not own. What is the guarantee
that it remains valid when the task is executed on IO thread?

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:275:
base::Bind(&AudioRendererHost::CloseStream, host_, stream_id_));
Same as above.
AudioRendererHost should probably have public non-thread-bound methods those
would post to IO thread, instead of exposing this implementation detail to the
clients.

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:353: host_,
stream_id_, callback_));
same as above

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:375:
CreateStreamCallback callback) {
const &

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:655: AudioEntry*
entry = LookupById(stream_id);
I suspect it's safe to do on IO thread only.

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.h:166: void
SendAuthorizationMessage(RequestDeviceAuthorizationCallback callback,
pass it as "const &" everywhere?

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.h:196:
RequestDeviceAuthorizationCallback callback,
const &?

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

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_output_ipc_factory.cc:45: void
RequestDeviceAuthorization(media::AudioOutputIPCDelegate* delegate,
Could you split declaration and definition of methods? See
https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_output_ipc_factory.cc:72: delegate_ = delegate;
Do we really want to continue if (delegate_ == delegate) initially? Shouldn't we
just return?

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
File content/renderer/media/audio_output_ipc_factory.h (right):

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_output_ipc_factory.h:47: // the
nit: please fix the formatting.

https://codereview.chromium.org/2289543003/diff/40001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/2289543003/diff/40001/media/media_options.gni...
media/media_options.gni:133: # - "gpu": Use mojo audio application hosted in the
gpu process.
Where does this list of options come from?

[Max Morin, 2016-09-02 10:27:07]

I addressed some comments. Many comments will be dealt with later.

Right now, creation fails with
[10298:10327:0902/112643:FATAL:platform_handle.cc(28)] Check failed: success. :
Bad file descriptor
when running the callback.
Stack trace: http://pastebin.com/raw/RCZZz6Y1

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:375:
CreateStreamCallback callback) {
On 2016/09/02 07:31:46, o1ka wrote:
> const &

mojo gets upset if I do this. This inefficiency is fine for the moment, I'll
figure it out later.

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

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_output_ipc_factory.cc:45: void
RequestDeviceAuthorization(media::AudioOutputIPCDelegate* delegate,
On 2016/09/02 07:31:46, o1ka wrote:
> Could you split declaration and definition of methods? See
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Yes. I'll go through this class later.

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
File content/renderer/media/audio_output_ipc_factory.h (right):

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/media/...
content/renderer/media/audio_output_ipc_factory.h:33: // AudioOutputIPCFactory
creates AudioOutputIPC objects that proxy calls to
On 2016/09/01 15:09:07, Henrik Grunell wrote:
> It seems like the main purpose for AudioOutputIPC is to map between stream id
> and AudioOutputDevice, but if we get rid of the id (we should at some point)
it
> should not be needed. Is there any other need for it?

I haven't looked into this class much yet. Hopefully, it's not needed.

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/renderer/render...
content/renderer/render_thread_impl.cc:719: #if BUILDFLAG(ENABLE_MOJO_AUDIO)
On 2016/09/01 15:09:07, Henrik Grunell wrote:
> Why do we have this build flag? (I know it came from the original CL.)

I don't think we should have it. I removed it for now.

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:25:
OnStreamStateChange(AudioOutputStreamState state);
On 2016/09/01 15:09:07, Henrik Grunell wrote:
> I believe we can replace this with responses. Play and pause for sure. Some
> (all?) of the errors could probably be signaled by simply deleting the
> AudioOutputStream object, which the other side will be informed of.

I agree. I'll change this.

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:48: int32 render_frame_id,
On 2016/09/01 15:09:07, Henrik Grunell wrote:
> Do you know what happened with the discussion around having a service per
render
> frame? Seems like the design is back to one service per render process.

I haven't read that discussion. Why would we have one per frame?

https://codereview.chromium.org/2289543003/diff/40001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:57: CloseStream(int32 stream_id);
On 2016/09/01 15:09:07, Henrik Grunell wrote:
> This should not be needed. Deleting the AudioOutputStream when closing will
> afaik signal to the other side.

Ok, I'll remove it.

[o1ka, 2016-09-02 11:30:55]

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:847:
BrowserThread::PostTask(
Beware of this bug https://bugs.chromium.org/p/chromium/issues/detail?id=633936

[Max Morin, 2016-09-02 11:37:30]

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2289543003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:847:
BrowserThread::PostTask(
On 2016/09/02 11:30:55, o1ka wrote:
> Beware of this bug
https://bugs.chromium.org/p/chromium/issues/detail?id=633936

Thanks for the heads up. Maybe we can eliminate LookupById in this CL (later).

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

Some comment/questions about the interface

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:16: interface AudioOutputStream {
naming conflict with media::AudioOutputStream. This is a bit confusing. But
probably we can survive...

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:25:
OnStreamStateChange(AudioOutputStreamState state);
Why not to have OnPlay/OnPause/OnError instead, and get rid of the enum?

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:28: // AudioOutput manages device
authorizations and AudioOutputStreamClients
How does it manage AudioOutputStreamClients? There is no API...

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:33: RequestDeviceAuthorization(
Make it "CreateStream" which returns a stream interface?

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:41: string matched_device_id);
(Those three together are OutputDeviceInfo - probably consider using it at some
point instead?)

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:46: CreateStream(
Make it a Start() method of AudioOUtputStream interface?

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:48: int32 render_frame_id,
Why do we need it? stream id is a unique identifier, isn't it?

https://codereview.chromium.org/2289543003/diff/60001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:57: CloseStream(int32 stream_id);
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.

[Max Morin, 2016-09-02 13:51:00]

Take a look at the new interfaces.

[o1ka, 2016-09-02 15:02:07]

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:12: interface AudioOutputStream {
We need to make sure there are no cases when browser side can generate an error
without us issuing an operation. It probably can: it can be just some system
device error, like device is unplugged. So we probably still need a client with
OnError() method.

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:22: // AudioOutput manages device
authorizations and AudioOutputStreamClients
It does not manage clients even if we introduce them back.

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:28: int64 session_id,
Why both of them are int64?

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:31: (AudioOutputStream stream,
Probably we should not return a stream if device is not authorized - if this
possible? We basically are not allowed to do anything with the stream if
authorization fails

[Max Morin, 2016-09-02 15:34:53]

Interface updated. The rest of the code probably does not make sense at all. :)

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:22: // AudioOutput manages device
authorizations and AudioOutputStreamClients
On 2016/09/02 15:02:07, o1ka wrote:
> It does not manage clients even if we introduce them back.

Done.

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:28: int64 session_id,
On 2016/09/02 15:02:07, o1ka wrote:
> Why both of them are int64?
They are int in the C++, so we have to make it 64 bit to be sure it's wide
enough. Then we have (safely) cast it back int on the browser side.

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:31: (AudioOutputStream stream,
On 2016/09/02 15:02:07, o1ka wrote:
> Probably we should not return a stream if device is not authorized - if this
> possible? We basically are not allowed to do anything with the stream if
> authorization fails

Done.

[o1ka, 2016-09-05 08:24:28]

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_output.mojom:28: int64 session_id,
On 2016/09/02 15:34:53, Max Morin Chromium wrote:
> On 2016/09/02 15:02:07, o1ka wrote:
> > Why both of them are int64?
> They are int in the C++, so we have to make it 64 bit to be sure it's wide
> enough. Then we have (safely) cast it back int on the browser side.

Ok, we need to make sure there are no security problems with that.

[o1ka, 2016-09-05 10:11:18]

cc: guidou@ since he's working on mojo for device enumeration.
guidou: this is a work in progress now, the main focus is on the interface. The
work will be split into several CLs (interface, browser side, renderer side)

[Max Morin, 2016-09-06 10:49:12]

Olga: I updated the interface. When it looks like this, it can be used in the
renderer as follows:

mojom::AudioOutputPtr audio_output;
mojom::AudioOutputRequest request = GetProxy(audio_output);
RequestDeviceAuthorization(render_frame_id, session_id, device_id,
std::move(proxy), origin);
// Now we can send stuff to audio_output right away:
audio_output->Start(params);

So we don't have to wait for RequestDeviceAuthorization to finish until we start
using the AudioOutput. Does this address your concern about the interface?

[Max Morin, 2016-09-06 10:51:03]

On 2016/09/06 10:49:12, Max Morin Chromium wrote:
> Olga: I updated the interface. When it looks like this, it can be used in the
> renderer as follows:
> 
> mojom::AudioOutputPtr audio_output;
> mojom::AudioOutputRequest request = GetProxy(audio_output);
> RequestDeviceAuthorization(render_frame_id, session_id, device_id,
> std::move(proxy), origin);
> // Now we can send stuff to audio_output right away:
> audio_output->Start(params);
> 
> So we don't have to wait for RequestDeviceAuthorization to finish until we
start
> using the AudioOutput. Does this address your concern about the interface?

Also, the renderer can pass in null, in which case we will not create a stream.

[Henrik Grunell, 2016-09-06 11:33:02]

https://codereview.chromium.org/2289543003/diff/120001/media/mojo/interfaces/...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2289543003/diff/120001/media/mojo/interfaces/...
media/mojo/interfaces/audio_output.mojom:34: RequestDeviceAuthorization(
We need to rethink the naming here and/or how we design the interface. Pepper
does not care about authorization - it only uses the default device. It would be
odd to call RequestDeviceAuthorization() when you don't want authorization. One
could of course argue that the default device could also be "authorized", which
it always will be. But I think it makes it clearer if named otherwise.

Let's discuss all this offline.