Add OSX aggregate audio device support for best performance.

Add OSX aggregate audio device support for best performance.

Some audio hardware is presented as separate input and output devices
even though they are really the same physical hardware and
share the same "clock domain" at the lowest levels of the driver.
A common of example of this is the "built-in" audio hardware:
    "Built-in Line Input"
    "Built-in Output"
We would like to use an "aggregate" device for these situations, since
CoreAudio will make the most efficient use of the shared "clock domain"
so we get the lowest latency and use fewer threads.

BUG=none
TEST=extensive manual testing with built-in Mac hardware
http://chromium.googlecode.com/svn/trunk/samples/audio/visualizer-live.html

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

[no longer working on chromium, 2013-04-03 21:11:57]

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
File media/audio/mac/aggregate_device_manager.cc (right):

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:107: if (result != noErr)
nit, simply do
return (result == noErr) ? clockdomain : 0;

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:164: return 0;
nit, return NULL

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:218: char input_device_name_[256];
nit, put this 256 into a static variable like kMaxDeviceNameSize

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:268: return -1;
should we also log the error? like how is done elsewhere.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:273: return -1;
ditto

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:318: OSStatus
AggregateDeviceManager::DestroyAggregateDevice() {
it looks like this return value is never used, make it void

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
File media/audio/mac/aggregate_device_manager.h (right):

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.h:23: bool UseDefaultAggregateDevice();
can we merge this UseDefaultAggregateDevice to GetDefaultAggregateDevice?

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:468: } else {
nit, you don't need the else

[DaleCurtis, 2013-04-04 14:37:45]

Turned out not to have WiFi on the plane over, so I haven't had long to go over
this. My general concern is the impact of device changes on this model. Is the
worst case they are simply out of sync? Or are we suddenly looking at serious
crashes, bad behavior?

[Chris Rogers, 2013-04-04 17:57:10]

On 2013/04/04 14:37:45, DaleCurtis wrote:
> Turned out not to have WiFi on the plane over, so I haven't had long to go
over
> this. My general concern is the impact of device changes on this model. Is the
> worst case they are simply out of sync? Or are we suddenly looking at serious
> crashes, bad behavior?

The worst case is that they're out-of-sync.  I can update the CL to check if the
default input and output have changed since lazy construction to avoid the
aggregate device in that case and fallback to the older varispeed approach.

[DaleCurtis, 2013-04-04 20:59:56]

sgtm, lets do that.

[Chris Rogers, 2013-04-05 20:01:38]

PTAL

dalecurtis: I now avoid re-using lazily created aggregate device if the default
devices have changed

xians1: all comments addressed

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
File media/audio/mac/aggregate_device_manager.cc (right):

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:107: if (result != noErr)
On 2013/04/03 21:11:57, xians1 wrote:
> nit, simply do
> return (result == noErr) ? clockdomain : 0;

Done.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:164: return 0;
On 2013/04/03 21:11:57, xians1 wrote:
> nit, return NULL

Done.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:218: char input_device_name_[256];
On 2013/04/03 21:11:57, xians1 wrote:
> nit, put this 256 into a static variable like kMaxDeviceNameSize

Done.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:268: return -1;
On 2013/04/03 21:11:57, xians1 wrote:
> should we also log the error? like how is done elsewhere.

Done.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:273: return -1;
On 2013/04/03 21:11:57, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.cc:318: OSStatus
AggregateDeviceManager::DestroyAggregateDevice() {
On 2013/04/03 21:11:57, xians1 wrote:
> it looks like this return value is never used, make it void

Done.

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
File media/audio/mac/aggregate_device_manager.h (right):

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/aggregate_...
media/audio/mac/aggregate_device_manager.h:23: bool UseDefaultAggregateDevice();
On 2013/04/03 21:11:57, xians1 wrote:
> can we merge this UseDefaultAggregateDevice to GetDefaultAggregateDevice?

Yes, good point.  DONE

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/13403002/diff/8001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:468: } else {
On 2013/04/03 21:11:57, xians1 wrote:
> nit, you don't need the else

Done.

[DaleCurtis, 2013-04-17 01:01:38]

You'll be removing the unified stream variant after this land right?

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
File media/audio/mac/aggregate_device_manager.cc (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:47: // aggregate device.
Should you destroy the old one at this point?

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:57: VLOG(1) << "input_clockdomain: "
<< input_clockdomain;
DVLOG? Here and all other VLOG.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:231: char
input_device_name_[kMaxDeviceNameLength];
dynamically allocate instead?

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:238: char
output_device_name_[kMaxDeviceNameLength];
Ditto.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:252: if (aggregate_device_dict ==
NULL)
!aggregate_device_dict

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:280: if (input_device_UID == NULL ||
output_device_UID == NULL) {
use ! instead.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
File media/audio/mac/aggregate_device_manager.h (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.h:28: // Helper methods.
Floating comment.

[no longer working on chromium, 2013-04-17 09:16:22]

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
File media/audio/mac/aggregate_device_manager.cc (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:95: OSStatus
AggregateDeviceManager::GetDeviceName(
nit, make it void since we don't care the return value

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:178: CFStringRef
kAggregateDeviceName =
nit, should it be aggregate_device_name here, according to the chromium naming
style, same question for the following code.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:198: CFNumberRef
kAggregateDeviceNumber = CFNumberCreate(
is it a leak here? I guess you might need to call CFRelease or use
base::mac::ScopedCFTypeRef.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:449: LOG(INFO) << "UNIFIED: default input
and output devices are identical";
just a thought, do you want to use uma to record the data?

[Chris Rogers, 2013-04-17 20:42:31]

PTAL - I plan to remove the unified back-end soon once this has a chance to bake
for a little while

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
File media/audio/mac/aggregate_device_manager.cc (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:47: // aggregate device.
On 2013/04/17 01:01:38, DaleCurtis wrote:
> Should you destroy the old one at this point?

I thought about this, but it would not be safe because a client may still be
using this aggregate device actively in an AUHAL

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:57: VLOG(1) << "input_clockdomain: "
<< input_clockdomain;
On 2013/04/17 01:01:38, DaleCurtis wrote:
> DVLOG? Here and all other VLOG.

Done.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:95: OSStatus
AggregateDeviceManager::GetDeviceName(
On 2013/04/17 09:16:22, xians1 wrote:
> nit, make it void since we don't care the return value

Done.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:178: CFStringRef
kAggregateDeviceName =
On 2013/04/17 09:16:22, xians1 wrote:
> nit, should it be aggregate_device_name here, according to the chromium naming
> style, same question for the following code.

changed to "const"

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:198: CFNumberRef
kAggregateDeviceNumber = CFNumberCreate(
On 2013/04/17 09:16:22, xians1 wrote:
> is it a leak here? I guess you might need to call CFRelease or use
> base::mac::ScopedCFTypeRef.

Yes, good catch - Fixed

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:231: char
input_device_name_[kMaxDeviceNameLength];
On 2013/04/17 01:01:38, DaleCurtis wrote:
> dynamically allocate instead?

Done.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:238: char
output_device_name_[kMaxDeviceNameLength];
On 2013/04/17 01:01:38, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:252: if (aggregate_device_dict ==
NULL)
On 2013/04/17 01:01:38, DaleCurtis wrote:
> !aggregate_device_dict

Done.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.cc:280: if (input_device_UID == NULL ||
output_device_UID == NULL) {
On 2013/04/17 01:01:38, DaleCurtis wrote:
> use ! instead.

Done.

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
File media/audio/mac/aggregate_device_manager.h (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/aggregate...
media/audio/mac/aggregate_device_manager.h:28: // Helper methods.
On 2013/04/17 01:01:38, DaleCurtis wrote:
> Floating comment.

Removed comment

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/13403002/diff/20003/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:449: LOG(INFO) << "UNIFIED: default input
and output devices are identical";
On 2013/04/17 09:16:22, xians1 wrote:
> just a thought, do you want to use uma to record the data?

Not a bad idea - for now added TODO

[DaleCurtis, 2013-04-17 21:37:53]

lgtm % nit

https://codereview.chromium.org/13403002/diff/34001/media/audio/mac/audio_syn...
File media/audio/mac/audio_synchronized_mac.cc (right):

https://codereview.chromium.org/13403002/diff/34001/media/audio/mac/audio_syn...
media/audio/mac/audio_synchronized_mac.cc:88: VLOG(1) <<
"AudioSynchronizedStream::AudioSynchronizedStream()";
DVLOG(1) ? The one in AUHAL prints out useful parameter information, so it's
more reasonable as a VLOG, this isn't particularly helpful.

[commit-bot: I haz the power, 2013-04-18 00:38:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/crogers@google.com/13403002/34001

[commit-bot: I haz the power, 2013-04-18 04:51:57]

Change committed as 194765