Add a unified audio I/O backend for ChromeOS.

Add a unified audio I/O backend for ChromeOS.

Implementation of AudioOutputStream for ChromeOS that supports both
input and output of audio samples in one callback.  This is protected
behind the kEnableWebAudioInput flag, and will remain behind it until
its use can be made conditional on the type of stream being played.

BUG=chromium-os:35272
TEST=added cras_unified_unittest and play with WebAudioInputDemo named
"PedalBoard.js" on Link, Lumpy, and Lucas.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191976

[dgreid, 2013-01-18 19:09:43]

All platform-side changes are landed in ToT ChromeOS.  Need to reduce the buffer
size, but need more test-time on different platforms first, so I have left it
alone for now at 2048.

[Chris Rogers, 2013-01-18 19:48:34]

Dylan, this is awesome!  I've added more reviewers since I'll be on vacation for
two weeks after today.

As soon as we get the |input_channels| information to the AudioManagers:
https://codereview.chromium.org/11878032/

then we'll be able to select the back-end more efficiently, and soon can remove
the enable flag...

[scherkus (not reviewing), 2013-01-23 20:51:21]

https://codereview.chromium.org/11959018/diff/1/media/audio/linux/cras_unified.h
File media/audio/linux/cras_unified.h (right):

https://codereview.chromium.org/11959018/diff/1/media/audio/linux/cras_unifie...
media/audio/linux/cras_unified.h:7: // CrasUnifiedStream object is *not*
thread-safe and should only be used
This diff makes it pretty clear that unified implementation is pretty close to
the regular output one

Would we save on duplicated code by extending the existing output backend with
an optional flag / mode for using unified vs regular?

[dgreid, 2013-02-21 06:38:17]

rebased on top of xians change to add audio_manager_cras.  Also remove
CrasOutput in favor of CrasUnified.  Might be able to remove CrasInput
eventually as well.

[henrika (OOO until Aug 14), 2013-02-21 08:35:57]

Adding xians since he knows the Linux audio backend really well.

Dylan, it would be great if you could add a simple schematic overview of the
main design (as is done for Mac and Windows) to simplify the review somewhat.

Also, list any existing limitations.

Finally, did you consider delay estimates that are required for the WebRTC
client? I assume that we will strive for a unified backend by default in the
long run and then delay estimates will be needed (see the existing unified
Windows implementation as an example).

[Chris Rogers, 2013-02-21 18:17:26]

https://codereview.chromium.org/11959018/diff/11001/media/audio/cras/audio_ma...
File media/audio/cras/audio_manager_cras.cc (right):

https://codereview.chromium.org/11959018/diff/11001/media/audio/cras/audio_ma...
media/audio/cras/audio_manager_cras.cc:85: return new CrasUnifiedStream(params,
this);
For the moment, I think you'll need to put this behind the enable flag, and also
should check that the |input_channels| field is > 0 (or ==2) similar to
AudioManagerMac/Win

Very soon I'll be removing the enable flag, but for now it should be in

[dgreid, 2013-02-22 02:09:34]

Thanks henrika, I added some pictures and plumbed up the delay to OnMoreIOData.

On windows why are you only updating sometimes?  Does one of the system calls
take a long time on win?

https://codereview.chromium.org/11959018/diff/11001/media/audio/cras/audio_ma...
File media/audio/cras/audio_manager_cras.cc (right):

https://codereview.chromium.org/11959018/diff/11001/media/audio/cras/audio_ma...
media/audio/cras/audio_manager_cras.cc:85: return new CrasUnifiedStream(params,
this);
On 2013/02/21 18:17:26, Chris Rogers wrote:
> For the moment, I think you'll need to put this behind the enable flag, and
also
> should check that the |input_channels| field is > 0 (or ==2) similar to
> AudioManagerMac/Win
> 
> Very soon I'll be removing the enable flag, but for now it should be in

the way I wired this up I'm going through the new unified path on the server
side no matter what.  The input path is optimized out if input_channels is set
to zero.  It is also limited to being the same as the number of output channels.

To get anything but zero in params I have to disable the output resampler.  It
creates new params that don't have input_channels set.  I can fix that in
another change.

[Chris Rogers, 2013-02-22 02:13:21]

On 2013/02/22 02:09:34, dgreid wrote:
> Thanks henrika, I added some pictures and plumbed up the delay to
OnMoreIOData.
> 
> On windows why are you only updating sometimes?  Does one of the system calls
> take a long time on win?
> 
>
https://codereview.chromium.org/11959018/diff/11001/media/audio/cras/audio_ma...
> File media/audio/cras/audio_manager_cras.cc (right):
> 
>
https://codereview.chromium.org/11959018/diff/11001/media/audio/cras/audio_ma...
> media/audio/cras/audio_manager_cras.cc:85: return new
CrasUnifiedStream(params,
> this);
> On 2013/02/21 18:17:26, Chris Rogers wrote:
> > For the moment, I think you'll need to put this behind the enable flag, and
> also
> > should check that the |input_channels| field is > 0 (or ==2) similar to
> > AudioManagerMac/Win
> > 
> > Very soon I'll be removing the enable flag, but for now it should be in
> 
> the way I wired this up I'm going through the new unified path on the server
> side no matter what.  The input path is optimized out if input_channels is set
> to zero.  It is also limited to being the same as the number of output
channels.

For optimization that's fine, but you'll still need to check the "Web Audio
Input" flag is enabled (like AudioManagerMac/Win)

> 
> To get anything but zero in params I have to disable the output resampler.  It
> creates new params that don't have input_channels set.  I can fix that in
> another change.

[henrika (OOO until Aug 14), 2013-02-22 11:44:09]

"On windows why are you only updating sometimes?  Does one of the system calls
take a long time on win?"

I want to avoid disrupting the audio by calling Core Audio APIs from a
designated audio thread "back in" again as much as possible. Measurements have
shown that the estimates are very stable and I could basically just take one
initial measurement and use that. This solution is a compromise between saving
resources and keeping the delay up2date.

[dgreid, 2013-03-20 16:13:51]

Rebased and Retested on top of tree.

By itself this won't work yet, I'll need to fix an issue on the renderer side
for ChromeOS that always sets the input channels to zero.

Chris, In RendererWebKitPlatformSupportImpl::createAudioDevice(
 there is a check if the input device is named "default" on ChromeOS the input
device is always named "automatic".  What is the reason for the check?  Is it OK
to also allow "automatic"?

[no longer working on chromium, 2013-03-20 16:55:04]

On 2013/03/20 16:13:51, dgreid wrote:
> Rebased and Retested on top of tree.
> 
> By itself this won't work yet, I'll need to fix an issue on the renderer side
> for ChromeOS that always sets the input channels to zero.
> 
> Chris, In RendererWebKitPlatformSupportImpl::createAudioDevice(
>  there is a check if the input device is named "default" on ChromeOS the input
> device is always named "automatic".  What is the reason for the check?  Is it
OK
> to also allow "automatic"?

Dylan, can you modify AudioManagerBase::kDefaultDeviceName to automatic when
CRAS is being used, and also remove the kCrasAutomaticDeviceName in
AudioManagerCras ?

[dgreid, 2013-03-20 17:01:23]

On 2013/03/20 16:55:04, xians1 wrote:
> On 2013/03/20 16:13:51, dgreid wrote:
> > Rebased and Retested on top of tree.
> > 
> > By itself this won't work yet, I'll need to fix an issue on the renderer
side
> > for ChromeOS that always sets the input channels to zero.
> > 
> > Chris, In RendererWebKitPlatformSupportImpl::createAudioDevice(
> >  there is a check if the input device is named "default" on ChromeOS the
input
> > device is always named "automatic".  What is the reason for the check?  Is
it
> OK
> > to also allow "automatic"?
> 
> Dylan, can you modify AudioManagerBase::kDefaultDeviceName to automatic when
> CRAS is being used, and also remove the kCrasAutomaticDeviceName in
> AudioManagerCras ?

That doesn't seem like a bad idea, but it won't help this issue,
createAudioDevice doesn't compare against that const, it does this:

  if (input_device_id != "default") {

[no longer working on chromium, 2013-03-20 17:10:23]

On 2013/03/20 17:01:23, dgreid wrote:
> On 2013/03/20 16:55:04, xians1 wrote:
> > On 2013/03/20 16:13:51, dgreid wrote:
> > > Rebased and Retested on top of tree.
> > > 
> > > By itself this won't work yet, I'll need to fix an issue on the renderer
> side
> > > for ChromeOS that always sets the input channels to zero.
> > > 
> > > Chris, In RendererWebKitPlatformSupportImpl::createAudioDevice(
> > >  there is a check if the input device is named "default" on ChromeOS the
> input
> > > device is always named "automatic".  What is the reason for the check?  Is
> it
> > OK
> > > to also allow "automatic"?
> > 
> > Dylan, can you modify AudioManagerBase::kDefaultDeviceName to automatic when
> > CRAS is being used, and also remove the kCrasAutomaticDeviceName in
> > AudioManagerCras ?
> 
> That doesn't seem like a bad idea, but it won't help this issue,
> createAudioDevice doesn't compare against that const, it does this:
> 
>   if (input_device_id != "default") {

I think we can change the hard coded "default" to
AudioManagerBase::kDefaultDeviceId, but lets wait for Chris' feedback.

[Chris Rogers, 2013-03-20 17:34:21]

It seems like it would be cleaner to make the string value be consistent across
OS.  Why is it the same on Mac and Windows, but different on CRAS?  Can't we
make CRAS be "default"?

In any case, I agree with Shijing that it would be nice to compare against the
constant instead of a hard-coded "default" string.  That would be more robust.

[dgreid, 2013-03-20 20:43:51]

On 2013/03/20 17:34:21, Chris Rogers wrote:
> It seems like it would be cleaner to make the string value be consistent
across
> OS.  Why is it the same on Mac and Windows, but different on CRAS?  Can't we
> make CRAS be "default"?
> 
> In any case, I agree with Shijing that it would be nice to compare against the
> constant instead of a hard-coded "default" string.  That would be more robust.

Thanks,

Looks like the "Automatic" setting isn't needed anymore.  It was there to be
different than the underlying alsa "default", but now that the audio managers
are separate the alsa devices are not listed.  I'll upload a change to remove
that.  Test shows this change works if automatic is removed.

[no longer working on chromium, 2013-03-21 15:38:27]

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
ditto

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:95: unsigned int input_channels =
params.input_channels();
Curiously, why do we need output_channels to be a member but not input_channels?

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:97: // We must have a manager.
nit, the comment tells nothing, please remove

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:98: DCHECK(manager_);
move the DCHECK to the top of the function.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:116: NOTREACHED() << "Input only streams not
supported by CrasUnifiedStream.";
move this section up to line 101, and combine with those DCHECKs.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:136: int err = cras_client_create(&client_);
why do you need variable err?
You can simply do 
if (cras_client_create(&client_) < 0)
...

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:261: if (latency_ts.tv_sec < 0 ||
latency_ts.tv_nsec < 0)
nit, add {} since else case has more than 1 line.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:356: timespec latency_ts  = {0, 0};
nit, move the declaration to the place where it is being used.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified.h (right):

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
nit, remove the (c), and change 2012 to 2013

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.h:15: #include <ostream>
may I ask where ostream is used?

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified_unittest.cc (right):

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
ditto

[dgreid, 2013-03-21 17:18:44]

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/03/21 15:38:27, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:95: unsigned int input_channels =
params.input_channels();
On 2013/03/21 15:38:27, xians1 wrote:
> Curiously, why do we need output_channels to be a member but not
input_channels?
If there are input channels they will always be the same as output channels
since the input and output format must match.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:95: unsigned int input_channels =
params.input_channels();
On 2013/03/21 15:38:27, xians1 wrote:
> Curiously, why do we need output_channels to be a member but not
input_channels?

If there are input channels there will be the same count as the output channels
since the input and output format match.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:97: // We must have a manager.
On 2013/03/21 15:38:27, xians1 wrote:
> nit, the comment tells nothing, please remove

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:98: DCHECK(manager_);
On 2013/03/21 15:38:27, xians1 wrote:
> move the DCHECK to the top of the function.

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:116: NOTREACHED() << "Input only streams not
supported by CrasUnifiedStream.";
On 2013/03/21 15:38:27, xians1 wrote:
> move this section up to line 101, and combine with those DCHECKs.

I got rid of it and simlified the checks above, there isn't a way to use unified
for just input yet.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:136: int err = cras_client_create(&client_);
On 2013/03/21 15:38:27, xians1 wrote:
> why do you need variable err?
> You can simply do 
> if (cras_client_create(&client_) < 0)
> ...

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:261: if (latency_ts.tv_sec < 0 ||
latency_ts.tv_nsec < 0)
On 2013/03/21 15:38:27, xians1 wrote:
> nit, add {} since else case has more than 1 line.

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:356: timespec latency_ts  = {0, 0};
On 2013/03/21 15:38:27, xians1 wrote:
> nit, move the declaration to the place where it is being used.

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:356: timespec latency_ts  = {0, 0};
On 2013/03/21 15:38:27, xians1 wrote:
> nit, move the declaration to the place where it is being used.

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified.h (right):

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/03/21 15:38:27, xians1 wrote:
> nit, remove the (c), and change 2012 to 2013

Done.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.h:15: #include <ostream>
On 2013/03/21 15:38:27, xians1 wrote:
> may I ask where ostream is used?

It isn't, removed.

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified_unittest.cc (right):

https://codereview.chromium.org/11959018/diff/24002/media/audio/cras/cras_uni...
media/audio/cras/cras_unified_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/03/21 15:38:27, xians1 wrote:
> ditto

Done.

[no longer working on chromium, 2013-03-26 09:32:39]

lgtm

[DaleCurtis, 2013-03-26 19:23:01]

lgtm % nits.  I especially like that you've removed the old implementations in
favor of a single approach :) Much nicer to have a single driver versus the 3 or
4 we're starting to have on other platforms!

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:84:
samples_per_packet_(params.frames_per_buffer()),
Instead of storing these four variables, you could just keep a copy of the
AudioParameters around.

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:100: unsigned int input_channels =
params.input_channels();
Just leave this as int for now. Long term security team wants us to convert
AudioBus etc to size_t for channels and frames.

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:180: if (audio_format == NULL) {
if !audio_format?

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:195: if (stream_params == NULL) {
Ditto.

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:210: callback->OnError(this, err);
Interface has changed to remove err.

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.cc:247: unsigned int
CrasUnifiedStream::GetBytesLatency(
If unsigned types are necessary, using size_t instead of "unsigned int" is
preferred.  In this case it looks like you just want uint32?

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
File media/audio/cras/cras_unified.h (right):

https://codereview.chromium.org/11959018/diff/31001/media/audio/cras/cras_uni...
media/audio/cras/cras_unified.h:66: uint32 DispatchCalback(size_t frames,
Callback

[commit-bot: I haz the power, 2013-04-01 22:26:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgreid@chromium.org/11959018/38001

[commit-bot: I haz the power, 2013-04-01 23:41:53]

Retried try job too often on linux_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-03 01:02:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgreid@chromium.org/11959018/38001

[commit-bot: I haz the power, 2013-04-03 04:16:47]

Change committed as 191976