Moving FIFO from AudioDestination to RendererWebAudioDeviceImpl

Moving FIFO from AudioDestination to RendererWebAudioDeviceImpl

This is Step 1: FIFO injection in case render callback buffer size does not match buffer
size specified by the client in the constructor.

BUG=564472

[o1ka, 2016-05-19 15:51:18]

Description was changed from

==========
Moving FIFO from AudioDestination to RendererWebAudioDeviceImpl - DRAFT

Step 1: FIFO injection in case render callback buffer size does not match buffer
size specified by the client in the constructor.

*** Android version of the change is unreliable yet.

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

to

==========
Moving FIFO from AudioDestination to RendererWebAudioDeviceImpl - DRAFT

Step 1: FIFO injection in case render callback buffer size does not match buffer
size specified by the client in the constructor.

*** Android version of the change is unreliable yet.

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

[o1ka, 2016-05-19 15:51:18]

olka@chromium.org changed reviewers:
+ hongchan@chromium.org, rtoy@chromium.org

[o1ka, 2016-05-26 11:40:29]

Description was changed from

==========
Moving FIFO from AudioDestination to RendererWebAudioDeviceImpl - DRAFT

Step 1: FIFO injection in case render callback buffer size does not match buffer
size specified by the client in the constructor.

*** Android version of the change is unreliable yet.

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

to

==========
Moving FIFO from AudioDestination to RendererWebAudioDeviceImpl

This is Step 1: FIFO injection in case render callback buffer size does not
match buffer
size specified by the client in the constructor.

BUG=564472
==========

[o1ka, 2016-05-26 16:19:12]

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

[o1ka, 2016-05-26 16:19:52]

olka@chromium.org changed reviewers:
+ tommi@chromium.org
- dalecurtis@chromium.org

[o1ka, 2016-05-26 16:24:49]

PTAL: rtoy@/hongchan@ - does it work for you? (and what is the WebAudio
refactoring bug to refer to here?)
tommi@ - as an owner.

[hongchan, 2016-05-26 16:43:33]

The work-in-progress CL on AudioDestination refactoring is:
https://codereview.chromium.org/1967393002/

(AudioDestination is now AudioRenderSink)

I can merge this CL with mine to finish the refactoring. It will be simply
removing pull FIFO from AudioRenderSink.

https://codereview.chromium.org/1990303002/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/1990303002/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:112:
base::Bind(&media::NullAudioSink::Play, null_audio_sink_));
I am ignorant on the renderer code, but why do we need null_audio_sink?

[o1ka, 2016-05-26 16:47:48]

https://codereview.chromium.org/1990303002/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/1990303002/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:112:
base::Bind(&media::NullAudioSink::Play, null_audio_sink_));
On 2016/05/26 16:43:33, hoch wrote:
> I am ignorant on the renderer code, but why do we need null_audio_sink?

It was introduce in this CL https://codereview.chromium.org/1195633003, to safe
some power on Android.

[o1ka, 2016-05-26 16:54:27]

On 2016/05/26 16:43:33, hoch wrote:
> The work-in-progress CL on AudioDestination refactoring is:
> https://codereview.chromium.org/1967393002/
> 
> (AudioDestination is now AudioRenderSink)
> 
> I can merge this CL with mine to finish the refactoring. It will be simply
> removing pull FIFO from AudioRenderSink.
> 

We need either this change or a fix for
https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the middle of
the next week.
I'm not sure if having dependency on your CL fits in this timeline.

rtoy@, could you fix bug 611377 then? Or will we land this CL separately?

[Raymond Toy, 2016-05-26 16:57:19]

On 2016/05/26 16:54:27, o1ka wrote:
> On 2016/05/26 16:43:33, hoch wrote:
> > The work-in-progress CL on AudioDestination refactoring is:
> > https://codereview.chromium.org/1967393002/
> > 
> > (AudioDestination is now AudioRenderSink)
> > 
> > I can merge this CL with mine to finish the refactoring. It will be simply
> > removing pull FIFO from AudioRenderSink.
> > 
> 
> We need either this change or a fix for
> https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the middle of
> the next week.
> I'm not sure if having dependency on your CL fits in this timeline.
> 
> rtoy@, could you fix bug 611377 then? Or will we land this CL separately?

I (or hongchan@) can remove the check now, if you need it.

[hongchan, 2016-05-26 16:58:04]

On 2016/05/26 16:54:27, o1ka wrote:
> On 2016/05/26 16:43:33, hoch wrote:
> > The work-in-progress CL on AudioDestination refactoring is:
> > https://codereview.chromium.org/1967393002/
> > 
> > (AudioDestination is now AudioRenderSink)
> > 
> > I can merge this CL with mine to finish the refactoring. It will be simply
> > removing pull FIFO from AudioRenderSink.
> > 
> 
> We need either this change or a fix for
> https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the middle of
> the next week.
> I'm not sure if having dependency on your CL fits in this timeline.
> 
> rtoy@, could you fix bug 611377 then? Or will we land this CL separately?

What you need is just to remove pullFIFO from AudioDestination. Right? If that's
the case, I can do that with this CL. I showed you my prototype just to give you
the idea of refactoring.

[o1ka, 2016-05-26 17:21:28]

On 2016/05/26 16:58:04, hoch wrote:
> On 2016/05/26 16:54:27, o1ka wrote:
> > On 2016/05/26 16:43:33, hoch wrote:
> > > The work-in-progress CL on AudioDestination refactoring is:
> > > https://codereview.chromium.org/1967393002/
> > > 
> > > (AudioDestination is now AudioRenderSink)
> > > 
> > > I can merge this CL with mine to finish the refactoring. It will be simply
> > > removing pull FIFO from AudioRenderSink.
> > > 
> > 
> > We need either this change or a fix for
> > https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the middle
of
> > the next week.
> > I'm not sure if having dependency on your CL fits in this timeline.
> > 
> > rtoy@, could you fix bug 611377 then? Or will we land this CL separately?
> 
> What you need is just to remove pullFIFO from AudioDestination. Right? If
that's
> the case, I can do that with this CL. I showed you my prototype just to give
you
> the idea of refactoring.

Yes, removing FIFO in this CL would be ideal. (I'm fine with the check removal
as a temporary solution as well)
Thanks!

[hongchan, 2016-05-26 17:26:47]

On 2016/05/26 16:58:04, hoch wrote:
> On 2016/05/26 16:54:27, o1ka wrote:
> > On 2016/05/26 16:43:33, hoch wrote:
> > > The work-in-progress CL on AudioDestination refactoring is:
> > > https://codereview.chromium.org/1967393002/
> > > 
> > > (AudioDestination is now AudioRenderSink)
> > > 
> > > I can merge this CL with mine to finish the refactoring. It will be simply
> > > removing pull FIFO from AudioRenderSink.
> > > 
> > 
> > We need either this change or a fix for
> > https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the middle
of
> > the next week.
> > I'm not sure if having dependency on your CL fits in this timeline.
> > 
> > rtoy@, could you fix bug 611377 then? Or will we land this CL separately?
> 
> What you need is just to remove pullFIFO from AudioDestination. Right? If
that's
> the case, I can do that with this CL. I showed you my prototype just to give
you
> the idea of refactoring.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Do we still need to pass the callback size of WebAudio? (which is 128 samples,
fixed) I guess Blink just creates the device, and the renderer always pulls 128
samples. The buffer size is a part of client_params_, but I am not sure who uses
it in the renderer code.

[o1ka, 2016-05-26 18:08:10]

On 2016/05/26 17:26:47, hoch wrote:
> On 2016/05/26 16:58:04, hoch wrote:
> > On 2016/05/26 16:54:27, o1ka wrote:
> > > On 2016/05/26 16:43:33, hoch wrote:
> > > > The work-in-progress CL on AudioDestination refactoring is:
> > > > https://codereview.chromium.org/1967393002/
> > > > 
> > > > (AudioDestination is now AudioRenderSink)
> > > > 
> > > > I can merge this CL with mine to finish the refactoring. It will be
simply
> > > > removing pull FIFO from AudioRenderSink.
> > > > 
> > > 
> > > We need either this change or a fix for
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the middle
> of
> > > the next week.
> > > I'm not sure if having dependency on your CL fits in this timeline.
> > > 
> > > rtoy@, could you fix bug 611377 then? Or will we land this CL separately?
> > 
> > What you need is just to remove pullFIFO from AudioDestination. Right? If
> that's
> > the case, I can do that with this CL. I showed you my prototype just to give
> you
> > the idea of refactoring.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Do we still need to pass the callback size of WebAudio? (which is 128 samples,
> fixed) I guess Blink just creates the device, and the renderer always pulls
128
> samples. The buffer size is a part of client_params_, but I am not sure who
uses
> it in the renderer code.

We need to know what buffer size to use to pull the data from WebAudio (It's
used for FIFO initialization). I think it's better to pass it as a parameter,
rather than to have two hard-coded constants (one in WebAudio and one in
renderer) those can diverge.

[hongchan, 2016-05-26 18:09:21]

On 2016/05/26 18:08:10, o1ka wrote:
> On 2016/05/26 17:26:47, hoch wrote:
> > On 2016/05/26 16:58:04, hoch wrote:
> > > On 2016/05/26 16:54:27, o1ka wrote:
> > > > On 2016/05/26 16:43:33, hoch wrote:
> > > > > The work-in-progress CL on AudioDestination refactoring is:
> > > > > https://codereview.chromium.org/1967393002/
> > > > > 
> > > > > (AudioDestination is now AudioRenderSink)
> > > > > 
> > > > > I can merge this CL with mine to finish the refactoring. It will be
> simply
> > > > > removing pull FIFO from AudioRenderSink.
> > > > > 
> > > > 
> > > > We need either this change or a fix for
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=611377 in the
middle
> > of
> > > > the next week.
> > > > I'm not sure if having dependency on your CL fits in this timeline.
> > > > 
> > > > rtoy@, could you fix bug 611377 then? Or will we land this CL
separately?
> > > 
> > > What you need is just to remove pullFIFO from AudioDestination. Right? If
> > that's
> > > the case, I can do that with this CL. I showed you my prototype just to
give
> > you
> > > the idea of refactoring.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Do we still need to pass the callback size of WebAudio? (which is 128
samples,
> > fixed) I guess Blink just creates the device, and the renderer always pulls
> 128
> > samples. The buffer size is a part of client_params_, but I am not sure who
> uses
> > it in the renderer code.
> 
> We need to know what buffer size to use to pull the data from WebAudio (It's
> used for FIFO initialization). I think it's better to pass it as a parameter,
> rather than to have two hard-coded constants (one in WebAudio and one in
> renderer) those can diverge.

Agreed.