Implement MojoAudioRendererSink and use it in UtilityMojoMediaClient

Implement MojoAudioRendererSink and use it in UtilityMojoMediaClient

Can be switch on with existing gn args:
 enable_mojo_media = true
 mojo_media_services = [ "renderer" ]
 mojo_media_host = "utility"

This allow to play audio instead of using NullAudioSink.
The audio is decoded in the utility process and then it
is sent back to the renderer process.

Once AudioSyncReader / AudioInputSyncWriter / AudioRendererHost
are mojified it could be possible to move the actual audio device
rendering from the browser process to the utility process.

Add MojoAudioRendererSinkImpl / Adapter.
Mojify media::OutputDeviceInfo.

BUG=682252
R=dalecurtis@chromium.org, sandersd@chromium.org, tguilbert@chromium.org, xhwang@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[Julien Isorce, 2017-01-18 12:34:33]

Description was changed from

==========
Implement MojoAudioRendererSink and use it in UtilityMojoMediaClient

Can be switch on with existing gn args:
 enable_mojo_media = true
 mojo_media_services = [ "renderer" ]
 mojo_media_host = "utility"

This allow to play audio instead of using NullAudioSink.
The audio is decoded in the utility process and then it
is sent back to the renderer process.

Once AudioSyncReader / AudioInputSyncWriter / AudioRendererHost
are mojified it could be possible to move the actual audio device
rendering from the browser process to the utility process.

Add MojoAudioRendererSinkImpl / Adapter.
Mojify media::OutputDeviceInfo.

BUG=681852
R=dalecurtis@chromium.org, sandersd@chromium.org, tguilbert@chromium.org,
xhwang@chromium.org
==========

to

==========
Implement MojoAudioRendererSink and use it in UtilityMojoMediaClient

Can be switch on with existing gn args:
 enable_mojo_media = true
 mojo_media_services = [ "renderer" ]
 mojo_media_host = "utility"

This allow to play audio instead of using NullAudioSink.
The audio is decoded in the utility process and then it
is sent back to the renderer process.

Once AudioSyncReader / AudioInputSyncWriter / AudioRendererHost
are mojified it could be possible to move the actual audio device
rendering from the browser process to the utility process.

Add MojoAudioRendererSinkImpl / Adapter.
Mojify media::OutputDeviceInfo.

BUG=681852
R=dalecurtis@chromium.org, sandersd@chromium.org, tguilbert@chromium.org,
xhwang@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Julien Isorce, 2017-01-18 15:13:58]

Description was changed from

==========
Implement MojoAudioRendererSink and use it in UtilityMojoMediaClient

Can be switch on with existing gn args:
 enable_mojo_media = true
 mojo_media_services = [ "renderer" ]
 mojo_media_host = "utility"

This allow to play audio instead of using NullAudioSink.
The audio is decoded in the utility process and then it
is sent back to the renderer process.

Once AudioSyncReader / AudioInputSyncWriter / AudioRendererHost
are mojified it could be possible to move the actual audio device
rendering from the browser process to the utility process.

Add MojoAudioRendererSinkImpl / Adapter.
Mojify media::OutputDeviceInfo.

BUG=681852
R=dalecurtis@chromium.org, sandersd@chromium.org, tguilbert@chromium.org,
xhwang@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Implement MojoAudioRendererSink and use it in UtilityMojoMediaClient

Can be switch on with existing gn args:
 enable_mojo_media = true
 mojo_media_services = [ "renderer" ]
 mojo_media_host = "utility"

This allow to play audio instead of using NullAudioSink.
The audio is decoded in the utility process and then it
is sent back to the renderer process.

Once AudioSyncReader / AudioInputSyncWriter / AudioRendererHost
are mojified it could be possible to move the actual audio device
rendering from the browser process to the utility process.

Add MojoAudioRendererSinkImpl / Adapter.
Mojify media::OutputDeviceInfo.

BUG=682252
R=dalecurtis@chromium.org, sandersd@chromium.org, tguilbert@chromium.org,
xhwang@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2017-01-25 01:04:37]

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

[DaleCurtis, 2017-01-25 01:04:38]

Hmm, why is this change necessary? The way the mojo media renderer works today,
decoded audio is returned to the renderer process where it is then sent to the
browser process? I'd have guessed you're trying to remove that hop, but your CL
description says that would be done later.

+maxmorin,olka who are working on mojofying the audio stack. I don't think we
want to carry the existing primitives forward in the long run.

[Max Morin, 2017-01-25 09:29:07]

Yes, MojoAudioRendererSink looks a lot like AudioOutput from
https://codereview.chromium.org/2319493002/ (in progress) with the main
difference being AudioOutput still using the quirky socket+shared mem interface.
It would be very nice if we could unify these interfaces, I think. I've been
meaning to benchmark the current audio flow mechanism against alternatives, but
haven't gotten around to it yet.

[o1ka, 2017-01-25 10:31:06]

Could you give some more context for the change? (The doc the bug points to does
not tell me much.)

This CL wraps AudioRendererSink into mojo interface, right?

AudioOutputDevice implementation of AudioRendererSink (one which delivers audio
from renderer to browser and eventually to the physical output device) is IPC
between browser and renderer. And this IPC is being converted to Mojo now (see
CL maxmorin@ pointed to for the browser-side part). AudioRendererSink will have
an implementation which renders audio via this new Mojo interface.

I don't think we want two IPC hops here. How about switching  mojo media
renderer to use this future AudioRendererSink which will be rendering over Mojo?

https://codereview.chromium.org/2640003002/diff/20001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_renderer_sink.mojom (right):

https://codereview.chromium.org/2640003002/diff/20001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_renderer_sink.mojom:17: int32 prior_frames_skipped)
=> (int32 nb_filled_frames, AudioBus? dest);
Today audio rendering is performed synchronously to minimize playback latency.
Have you considered an impact of receiving frames asynchronously and potential
problems with delays caused by tasks being queued up/executed on the receiving
thread?

[o1ka, 2017-01-25 11:25:12]

Could you also clarify how device access permissions are/will be handled by
media renderer in the utility process?

[Julien Isorce, 2017-01-25 23:54:10]

On 2017/01/25 01:04:38, DaleCurtis wrote:
> Hmm, why is this change necessary? The way the mojo media renderer works
today,
> decoded audio is returned to the renderer process where it is then sent to the
> browser process?
Is it a question on my CL ? Because this is what my CL does.
Without that CL the user does not hear anything when the MojoRendererService
uses the DefaultRendererFactory.
Also see last comment below.

> I'd have guessed you're trying to remove that hop, but your CL
> description says that would be done later.
My CL does not remove that hop. It would require
https://bugs.chromium.org/p/chromium/issues/detail?id=425368 first.

> 
> +maxmorin,olka who are working on mojofying the audio stack. I don't think we
> want to carry the existing primitives forward in the long run.

Without this CL the MojoRenderer does not start its AudioRendererSink.
With this CL the start is ipc from utility process, from AudioRendererImpl.
See second graph in
https://docs.google.com/document/d/19n22cLElIoBEBepXA4TslHfVWRbzf37eThzppmXgT...
just replace VideoRendererImpl by AudioRendererImpl.

[Julien Isorce, 2017-01-26 00:00:56]

On 2017/01/25 10:31:06, o1ka wrote:
> Could you give some more context for the change? (The doc the bug points to
does
> not tell me much.)
> 
> This CL wraps AudioRendererSink into mojo interface, right?

Yes. See second diagram, just replace video by audio for the ipc part.

> 
> AudioOutputDevice implementation of AudioRendererSink (one which delivers
audio
> from renderer to browser and eventually to the physical output device) is IPC
> between browser and renderer. And this IPC is being converted to Mojo now (see
> CL maxmorin@ pointed to for the browser-side part). AudioRendererSink will
have
> an implementation which renders audio via this new Mojo interface.
> 
> I don't think we want two IPC hops here. How about switching  mojo media
> renderer to use this future AudioRendererSink which will be rendering over
Mojo?

Ideally whatever is in the browser process should be moveable to a separate
process.
Otherwise I missed your point.

> 
>
https://codereview.chromium.org/2640003002/diff/20001/media/mojo/interfaces/a...
> File media/mojo/interfaces/audio_renderer_sink.mojom (right):
> 
>
https://codereview.chromium.org/2640003002/diff/20001/media/mojo/interfaces/a...
> media/mojo/interfaces/audio_renderer_sink.mojom:17: int32
prior_frames_skipped)
> => (int32 nb_filled_frames, AudioBus? dest);
> Today audio rendering is performed synchronously to minimize playback latency.
> Have you considered an impact of receiving frames asynchronously and potential
> problems with delays caused by tasks being queued up/executed on the receiving
> thread?
I have considered it but just with the intention to make it an async ipc, just
because it is always better, I mean not for any other reasons.
I had not time to try it and already spent lot of time with mojification of the
demuxers :)

Thx for your comments!

[Julien Isorce, 2017-01-26 00:03:14]

On 2017/01/25 11:25:12, o1ka wrote:
> Could you also clarify how device access permissions are/will be handled by
> media renderer in the utility process?

The idea is for an embedder to customize the sandbox policy for the utility
process.
If restriction is maximum then there will be no other choice for him to keep the
hop.
If it can relaxe at least shared memory / shm the it will be able to allow pulse
audio on Linux
without the need to allow accessing the audio device directly.

[Julien Isorce, 2017-01-26 01:18:45]

On 2017/01/25 09:29:07, Max Morin wrote:
> Yes, MojoAudioRendererSink looks a lot like AudioOutput from
> https://codereview.chromium.org/2319493002/ (in progress) with the main
> difference being AudioOutput still using the quirky socket+shared mem
interface.
> It would be very nice if we could unify these interfaces, I think. I've been
> meaning to benchmark the current audio flow mechanism against alternatives,
but
> haven't gotten around to it yet.

Thx for your comments. For me it looks different :)
My understanding is that you CL make whatever audio bits is in the browser
process
to be a service (so eventually moveable to another process). My CL makes
possible
to control from another process the audio bit in the renderer process.
I admit it is not a clear explanation so let take a situation.

Suppose I use MojoRendererService in a utility process. This service
is setup to use the DefaultRendererFactory. Then suppose also I use your audio
output
service and make it run in the utility process.
How is started the AudioRendererSink that lives in the renderer process ?
Because in current
upstream, MojoRenderer(the client so in renderer process) does not start the
audio renderer sink
on which it has a pointer as member.

(My CL makes it starts through IPC)

Thx!

[o1ka, 2017-01-26 09:17:05]

On 2017/01/26 00:00:56, Julien Isorce wrote:
> On 2017/01/25 10:31:06, o1ka wrote:
> > Could you give some more context for the change? (The doc the bug points to
> does
> > not tell me much.)
> > 
> > This CL wraps AudioRendererSink into mojo interface, right?
> 
> Yes. See second diagram, just replace video by audio for the ipc part.
> 
> > 
> > AudioOutputDevice implementation of AudioRendererSink (one which delivers
> audio
> > from renderer to browser and eventually to the physical output device) is
IPC
> > between browser and renderer. And this IPC is being converted to Mojo now
(see
> > CL maxmorin@ pointed to for the browser-side part). AudioRendererSink will
> have
> > an implementation which renders audio via this new Mojo interface.
> > 
> > I don't think we want two IPC hops here. How about switching  mojo media
> > renderer to use this future AudioRendererSink which will be rendering over
> Mojo?
> 
> Ideally whatever is in the browser process should be moveable to a separate
> process.
> Otherwise I missed your point.
> 


AudioRendererSink is not in the browser process: it's in the renderer process
(or any other process which wants to access system audio).
It's a client side. There is no plan to move it anywhere.
But we'll make it to be able to talk to Mojo audio service.


> > Today audio rendering is performed synchronously to minimize playback
latency.
> > Have you considered an impact of receiving frames asynchronously and
potential
> > problems with delays caused by tasks being queued up/executed on the
receiving
> > thread?
> I have considered it but just with the intention to make it an async ipc, just
> because it is always better, I mean not for any other reasons.
> I had not time to try it and already spent lot of time with mojification of
the
> demuxers :)

Could you elaborate on "async is always better"?

[o1ka, 2017-01-26 09:24:44]

On 2017/01/26 00:03:14, Julien Isorce wrote:
> On 2017/01/25 11:25:12, o1ka wrote:
> > Could you also clarify how device access permissions are/will be handled by
> > media renderer in the utility process?
> 
> The idea is for an embedder to customize the sandbox policy for the utility
> process.
> If restriction is maximum then there will be no other choice for him to keep
the
> hop.

My question was about user permission. How the rendering process makes sure the
user allowed access to the device?

> If it can relaxe at least shared memory / shm the it will be able to allow
pulse
> audio on Linux
> without the need to allow accessing the audio device directly.


Please keep in mind that audio functionality is more than rendering to output
device: it's also capturing audio from microphones and enumerating audio
devices/accessing their parameters.
And it works not only on top of pulse audio, but also on other platforms.

[o1ka, 2017-01-26 09:28:28]

On 2017/01/26 01:18:45, Julien Isorce wrote:
> On 2017/01/25 09:29:07, Max Morin wrote:
> > Yes, MojoAudioRendererSink looks a lot like AudioOutput from
> > https://codereview.chromium.org/2319493002/ (in progress) with the main
> > difference being AudioOutput still using the quirky socket+shared mem
> interface.
> > It would be very nice if we could unify these interfaces, I think. I've been
> > meaning to benchmark the current audio flow mechanism against alternatives,
> but
> > haven't gotten around to it yet.
> 
> Thx for your comments. For me it looks different :)
> My understanding is that you CL make whatever audio bits is in the browser
> process
> to be a service (so eventually moveable to another process). My CL makes
> possible
> to control from another process the audio bit in the renderer process.
> I admit it is not a clear explanation so let take a situation.
> 
> Suppose I use MojoRendererService in a utility process. This service
> is setup to use the DefaultRendererFactory. Then suppose also I use your audio
> output
> service and make it run in the utility process.
> How is started the AudioRendererSink that lives in the renderer process ?
> Because in current
> upstream, MojoRenderer(the client so in renderer process) does not start the
> audio renderer sink
> on which it has a pointer as member.

Sorry, I don't understand what you mean. AudioRendererSink is used by various
renderer-side audio paths (media player, WebRTC, WebAudio) to render audio. It's
a client-side interface for a sandbox process to access platform audio.

[o1ka, 2017-01-26 14:36:50]

Please see the following docs for more info:

https://docs.google.com/a/chromium.org/document/d/1e1xeuWQyG-2nwQERX_4oKbExjN...
https://docs.google.com/a/chromium.org/document/d/1awQoajq_DLmz2AIU9iweC0zEYl...

[o1ka, 2017-01-26 14:42:55]

Corrected links:

https://docs.google.com/a/chromium.org/document/d/1fVHVJjd5zTqc6O7b0lprILyuFP...
https://docs.google.com/a/chromium.org/document/d/1awQoajq_DLmz2AIU9iweC0zEYl...

[Julien Isorce, 2017-01-27 22:41:31]

On 2017/01/26 14:42:55, o1ka wrote:
> Corrected links:
> 
>
https://docs.google.com/a/chromium.org/document/d/1fVHVJjd5zTqc6O7b0lprILyuFP...
>
https://docs.google.com/a/chromium.org/document/d/1awQoajq_DLmz2AIU9iweC0zEYl...

Thx for the links. Do you know how to zoom on the diagram because since I cannot
edit, the browser zoom does not help much. So I cannot see at all :)

In any case, I could not find "MojoRenderer" in these 2 docs. I think this
scenario is not taken into account and it should. So when no using MojoRenderer,
this is the AudioRendererImpl that call "Start" in the AudioRendererSink living
in the renderer process.
But when using MojoRenderer, there is no AudioRendererImpl in the renderer
process so who start the AudioRendererSink then ?
In my scenario the DefaultRendererFactory runs into the utility process and so
the AudioRendererImpl lives in the utility process. So It does the start through
IPC.
Is it more clear ? Let me know, thx.

[Julien Isorce, 2017-01-28 00:24:36]

On 2017/01/26 09:17:05, o1ka wrote:
> > > Today audio rendering is performed synchronously to minimize playback
> latency.
> > > Have you considered an impact of receiving frames asynchronously and
> potential
> > > problems with delays caused by tasks being queued up/executed on the
> receiving
> > > thread?

> > I have considered it but just with the intention to make it an async ipc,
just
> > because it is always better, I mean not for any other reasons.
> > I had not time to try it and already spent lot of time with mojification of
> the
> > demuxers :)

> 
> Could you elaborate on "async is always better"?

See the 3 bullets on top here
https://www.chromium.org/developers/design-documents/mojo/synchronous-calls
In my CL it is a sync call because existing
media::AudioRendererSink::RenderCallback::Render expects a return value. This
API should be changed to use callbacks in order to move here to an async IPC.

[Julien Isorce, 2017-01-28 00:29:16]

On 2017/01/26 09:24:44, o1ka wrote:
> On 2017/01/26 00:03:14, Julien Isorce wrote:
> > On 2017/01/25 11:25:12, o1ka wrote:
> > > Could you also clarify how device access permissions are/will be handled
by
> > > media renderer in the utility process?
> > 
> > The idea is for an embedder to customize the sandbox policy for the utility
> > process.
> > If restriction is maximum then there will be no other choice for him to keep
> the
> > hop.
> 
> My question was about user permission. How the rendering process makes sure
the
> user allowed access to the device?
> 
> > If it can relaxe at least shared memory / shm the it will be able to allow
> pulse
> > audio on Linux
> > without the need to allow accessing the audio device directly.
> 
> 
> Please keep in mind that audio functionality is more than rendering to output
> device: it's also capturing audio from microphones and enumerating audio
> devices/accessing their parameters.
> And it works not only on top of pulse audio, but also on other platforms.

I think this also re-enforce the idea that this CL is different than your work.
Because MojoRendererSinkImpl/Adapter are only useable with MojoRenderer/Service.
As you can see here
https://codereview.chromium.org/2640003002/patch/20001/30012, this is Mojorender
that instantiates MojoRendererSinkImpl.
And only MojoRendererService instantiates MojoRendererSinkAdapter. Maybe the
name I picked are confusing. 
This is similar design as MojoDemuxerStreamImpl/Adapter, these are not services
that you can request from the client.

[o1ka, 2017-01-30 15:50:49]

> Thx for the links. Do you know how to zoom on the diagram because since I
cannot
> edit, the browser zoom does not help much. So I cannot see at all :)

The diagrams should be clickable, with a big picture opening in a new window.
Does it work for you? 

> In any case, I could not find "MojoRenderer" in these 2 docs. I think this
> scenario is not taken into account and it should. 

According to your diagrams, MojoRenderer lives in the renderer process and works
on top of AudioRendererSink. Correct? Then it is just another user of it.

> So when no using MojoRenderer,
> this is the AudioRendererImpl that call "Start" in the AudioRendererSink
living
> in the renderer process.
> But when using MojoRenderer, there is no AudioRendererImpl in the renderer
> process so who start the AudioRendererSink then ?
> In my scenario the DefaultRendererFactory runs into the utility process and so
> the AudioRendererImpl lives in the utility process. So It does the start
through
> IPC.

Could you explain why AudioRendererSink audio is rendered to should live in the
renderer process? Why the utility process can’t render through AudioRendererSink
directly to the browser process?

The way CL works now, the utility process passes audio data to the renderer
process through MojoAudioRendererSink, and the renderer passes it to the browser
process through AudioRendererSink.
When we have Mojo Audio Service, nothing will really change: the utility process
will pass data to the renderer process, and it will pass data to audio process.

So how are you going to eliminate that extra process hop?

> > Could you elaborate on "async is always better"?
> See the 3 bullets on top here
> https://www.chromium.org/developers/design-documents/mojo/synchronous-calls
> In my CL it is a sync call because existing
> media::AudioRendererSink::RenderCallback::Render expects a return value. This
> API should be changed to use callbacks in order to move here to an async IPC.

This is “always better” in terms of code complexity. Async is not better if
system requirements can’t be met while using it, right? Audio rendering has
real-time requirements, which restricts us quite a lot in what can be done
asynchronously. System renders audio on a real-time thread. Data request
callbacks are issued every 5-20ms, and if audio pipeline fails to deliver data
on time the user will hear that - there will be audio glitches.

So, asynchronous audio rendering can be landed only after very thorough testing
and analysis. 

> I think this also re-enforce the idea that this CL is different than your
work.

I understand that it is different, but I do not quite understand the point of
it: while you render to MojoAudioRendererSink, you always have 2 process hops on
the rendering path.
Could you provide more explanations and some diagrams of what you are doing now
and what you want to have in the future?

[Julien Isorce, 2017-01-31 16:46:59]

On 2017/01/30 15:50:49, o1ka (CET) wrote:
> > Thx for the links. Do you know how to zoom on the diagram because since I
> cannot
> > edit, the browser zoom does not help much. So I cannot see at all :)
> 
> The diagrams should be clickable, with a big picture opening in a new window.
> Does it work for you? 

Yes, thx.

> 
> > In any case, I could not find "MojoRenderer" in these 2 docs. I think this
> > scenario is not taken into account and it should. 
> 
> According to your diagrams, MojoRenderer lives in the renderer process and
works
> on top of AudioRendererSink. Correct? Then it is just another user of it.

Yes. Ideally instead of being created in the renderer process, it could be
created in the
separate process. So I could create from here
https://codereview.chromium.org/2640003002/patch/20001/30031 .
I.e. instead of creating the adapter that this CL introduced, I could create
what is currently created here:
https://chromium.googlesource.com/chromium/src.git/+/master/content/renderer/...
.
So far so good :)

> 
> > So when no using MojoRenderer,
> > this is the AudioRendererImpl that call "Start" in the AudioRendererSink
> living
> > in the renderer process.
> > But when using MojoRenderer, there is no AudioRendererImpl in the renderer
> > process so who start the AudioRendererSink then ?
> > In my scenario the DefaultRendererFactory runs into the utility process and
so
> > the AudioRendererImpl lives in the utility process. So It does the start
> through
> > IPC.
> 
> Could you explain why AudioRendererSink audio is rendered to should live in
the
> renderer process? Why the utility process can’t render through
AudioRendererSink
> directly to the browser process?

Just to clarify, I am not defending that it should. It is just that currently in
upstream
code I do not see how it could not be in the renderer process.

The reason is because of WebMediaPlayerImpl::setSinkId +
HTMLMediaElementAudioOutputDevice 
+ blink::WebSetSinkIdCallbacks (+ WebAudioSourceProvider), so for me it
impossible to just do
the move I mentioned above.


> 
> The way CL works now, the utility process passes audio data to the renderer
> process through MojoAudioRendererSink, and the renderer passes it to the
browser
> process through AudioRendererSink.
> When we have Mojo Audio Service, nothing will really change: the utility
process
> will pass data to the renderer process, and it will pass data to audio
process.
> 
> So how are you going to eliminate that extra process hop?

So since it has to be something in the renderer process, the hop-removal
will be to make the AudioRendererSinkAdapter talk to your Mojo Audio Service
too. So it will send the buffer to it directly.

> 
> > > Could you elaborate on "async is always better"?
> > See the 3 bullets on top here
> > https://www.chromium.org/developers/design-documents/mojo/synchronous-calls
> > In my CL it is a sync call because existing
> > media::AudioRendererSink::RenderCallback::Render expects a return value.
This
> > API should be changed to use callbacks in order to move here to an async
IPC.
> 
> This is “always better” in terms of code complexity. Async is not better if
> system requirements can’t be met while using it, right? Audio rendering has
> real-time requirements, which restricts us quite a lot in what can be done
> asynchronously. System renders audio on a real-time thread. Data request
> callbacks are issued every 5-20ms, and if audio pipeline fails to deliver data
> on time the user will hear that - there will be audio glitches.

I understand, thx for the details. I think they both are arguable.
In any case, having both MojoRenderService and MojoAudioService, it will be
possible that IPC if they live both in the same utility process for example.

> 
> So, asynchronous audio rendering can be landed only after very thorough
testing
> and analysis. 

I have no plan to change that so no worries :)

> 
> > I think this also re-enforce the idea that this CL is different than your
> work.
> 
> I understand that it is different, but I do not quite understand the point of
> it: while you render to MojoAudioRendererSink, you always have 2 process hops
on
> the rendering path.
> Could you provide more explanations and some diagrams of what you are doing
now
> and what you want to have in the future?

Once the MojoAudioService lands I will re-work this CL to remove the hop, but as
explain
above in my last reply it won't be done just by moving the bit from renderer
process to
utility process. It will be something different.

So for me we have replied to Dale's concerned: this CL depends on audio
mojification
(and I mentioned that from the initial CL description) and this is independent
so there
is no need to worry about this CL or no need to integrate this CL while you guys
are
doing the audio mojification.

Are you ok with this conclusion ? Thx for all your comments, it was very
helpful.
Otherwise let me know, no worries.

[o1ka, 2017-02-02 15:31:55]

> > According to your diagrams, MojoRenderer lives in the renderer process and
> works
> > on top of AudioRendererSink. Correct? Then it is just another user of it.
> 
> Yes. Ideally instead of being created in the renderer process, it could be
> created in the
> separate process. So I could create from here
> https://codereview.chromium.org/2640003002/patch/20001/30031 .
> I.e. instead of creating the adapter that this CL introduced, I could create
> what is currently created here:
>
https://chromium.googlesource.com/chromium/src.git/+/master/content/renderer/...

Oh, right, I forgot about that place. So, the situation is more complicated:
that SwitchableAudioRendererSink is in fact
AudioRendererMixerInput, which works on top of AudioOutputDevice.
There is no plan to mojofy it.
So, how an extra process hop will be avoided?

> 
> > 
> > > So when no using MojoRenderer,
> > > this is the AudioRendererImpl that call "Start" in the AudioRendererSink
> > living
> > > in the renderer process.
> > > But when using MojoRenderer, there is no AudioRendererImpl in the renderer
> > > process so who start the AudioRendererSink then ?
> > > In my scenario the DefaultRendererFactory runs into the utility process
and
> so
> > > the AudioRendererImpl lives in the utility process. So It does the start
> > through
> > > IPC.
> > 
> > Could you explain why AudioRendererSink audio is rendered to should live in
> the
> > renderer process? Why the utility process can’t render through
> AudioRendererSink
> > directly to the browser process?
> 
> Just to clarify, I am not defending that it should. It is just that currently
in
> upstream
> code I do not see how it could not be in the renderer process.
> 
> The reason is because of WebMediaPlayerImpl::setSinkId +
> HTMLMediaElementAudioOutputDevice 
> + blink::WebSetSinkIdCallbacks (+ WebAudioSourceProvider), so for me it
> impossible to just do
> the move I mentioned above.

For design decisions like that it would be great to have a proper design doc
with a before/after diagram.
Audio stack is different from video, and design for it deserves to be documented
separately.
Taking into account the problem of an extra process hop in the current approach,
we need to figure what
the next step (of avoiding that process hop) will look like, before considering
this CL.

Have you considered an approach where the sink created at 
https://chromium.googlesource.com/chromium/src.git/+/master/content/renderer/...
is an adapter which provide control interface to a remote Mojo sink which
renders audio in the utility process?

> So for me we have replied to Dale's concerned: this CL depends on audio
> mojification
> (and I mentioned that from the initial CL description) and this is independent
> so there
> is no need to worry about this CL or no need to integrate this CL while you
guys
> are
> doing the audio mojification.
> 
> Are you ok with this conclusion ? Thx for all your comments, it was very
> helpful.
> Otherwise let me know, no worries.

Thank you for your explanations! I'm not worrying, I'm doing a code review :)
And I have not enough data on the design to evaluate the code yet - that's why
the questions.

[Julien Isorce, 2017-09-13 12:24:09]

Moved to https://chromium-review.googlesource.com/c/chromium/src/+/664879