Implement GetDefaultOutputDeviceID, GetAssociatedOutputDeviceID and ...

Implement GetDefaultOutputDeviceID, GetAssociatedOutputDeviceID and ...
...GetPreferredOutputStreamParameters on Mac.
The GetPreferredOutputStreamParameters implementation now supports non-default output device ids.

BUG=276894

[henrika (OOO until Aug 14), 2013-09-07 08:04:25]

Great stuff Tommi. I am not able to provide comments about the Core Audio parts
like "you could have used another approach here" since I don't know the
low-level Mac parts well enough. But if this version works I am fine.

LGTM w/ nits.

https://codereview.chromium.org/23533045/diff/1/media/audio/audio_manager_uni...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/23533045/diff/1/media/audio/audio_manager_uni...
media/audio/audio_manager_unittest.cc:38: #if defined(OS_WIN) ||
defined(OS_MACOSX)
Why inside and not outside the complete method?

https://codereview.chromium.org/23533045/diff/1/media/audio/audio_manager_uni...
media/audio/audio_manager_unittest.cc:74:
EXPECT_TRUE(found_an_associated_device);
Are we sure that this will be true on all boths with hardware?

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:452: kAudioDevicePropertyRelatedDevices,
Guess thus guy is the key here. Nice.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:462:
devices(reinterpret_cast<AudioDeviceID*>(malloc(size)));
I always hesitate to use reinterpret; you don't ;-)

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:472: result =
AudioObjectGetPropertyDataSize(devices.get()[i],
Nit, I did clang-format on my latest CL and I think it lines up params in an
alternative way. You could try it with git cl format if you like. Guess it will
be mandatory soon in Chrome anyhow.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:478: continue;  // Skip if there aren't any
output channels.
Don't we have methods to detect number of output devices which can be utilized
at the top of this function to ensure that we don't even try if there are no
outputs?

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:493: std::string
ret(base::SysCFStringRefToUTF8(uid));
What does these IDs look like?

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:580: OSStatus status =
AudioObjectGetPropertyData(device_id, &property_address, 0,
Different style than above ;-)

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:615: DLOG(ERROR) << "Invalid device " <<
output_device_id;
I would say Invalid output device just in case

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:619: int hardware_channels = 2;
Don't understand this "fallback" scheme. Do you?

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:636: // TODO(crogers): given the
limitations of the AudioOutputStream
Change crogers to xians?

[henrika (OOO until Aug 14), 2013-09-07 08:05:02]

% if it compiles ;-)

[tommi (sloooow) - chröme, 2013-09-08 18:45:26]

Add support for opening non-default output streams and fix a related todo for
the input side

[tommi (sloooow) - chröme, 2013-09-08 19:08:54]

Address comments

[tommi (sloooow) - chröme, 2013-09-08 19:11:19]

Thanks Henrik.  Comments addressed.  I did a few more updates after testing with
a few different devices. Some of which I was planning on doing later but since
the work wasn't that much and it was closely related to the work in this cl, I
decided to add it. sorry! :)  ptal!

https://codereview.chromium.org/23533045/diff/1/media/audio/audio_manager_uni...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/23533045/diff/1/media/audio/audio_manager_uni...
media/audio/audio_manager_unittest.cc:38: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2013/09/07 08:04:25, henrika wrote:
> Why inside and not outside the complete method?

I'm following the style of the other test in this file.

https://codereview.chromium.org/23533045/diff/1/media/audio/audio_manager_uni...
media/audio/audio_manager_unittest.cc:74:
EXPECT_TRUE(found_an_associated_device);
On 2013/09/07 08:04:25, henrika wrote:
> Are we sure that this will be true on all boths with hardware?

I think it's a reasonable thing to expect (we have checks for both output and
input devies at the top, if none of them match, it'll be a fairly unusual
setup).  If it turns out to be a problem, it's easy to fix.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:462:
devices(reinterpret_cast<AudioDeviceID*>(malloc(size)));
On 2013/09/07 08:04:25, henrika wrote:
> I always hesitate to use reinterpret; you don't ;-)

Not when it's the right thing to do ;)

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:472: result =
AudioObjectGetPropertyDataSize(devices.get()[i],
On 2013/09/07 08:04:25, henrika wrote:
> Nit, I did clang-format on my latest CL and I think it lines up params in an
> alternative way. You could try it with git cl format if you like. Guess it
will
> be mandatory soon in Chrome anyhow.

This is fine style-wize and consistent with all other call sites of
AudioObjectGetPropertyDataSize in this file.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:478: continue;  // Skip if there aren't any
output channels.
On 2013/09/07 08:04:25, henrika wrote:
> Don't we have methods to detect number of output devices which can be utilized
> at the top of this function to ensure that we don't even try if there are no
> outputs?

See line 453 where the "scope" is set to output.
When we hit this line, the outputs of the matched device aren't active.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:493: std::string
ret(base::SysCFStringRefToUTF8(uid));
On 2013/09/07 08:04:25, henrika wrote:
> What does these IDs look like?

Here's an example of a USB device I tested with:
input: AppleUSBAudioEngine:Fractal Audio Systems:AXE-FX II:1a120000:2,1
output: AppleUSBAudioEngine:Fractal Audio Systems:AXE-FX II:1a120000:2,1

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:580: OSStatus status =
AudioObjectGetPropertyData(device_id, &property_address, 0,
On 2013/09/07 08:04:25, henrika wrote:
> Different style than above ;-)

Fixed.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:615: DLOG(ERROR) << "Invalid device " <<
output_device_id;
On 2013/09/07 08:04:25, henrika wrote:
> I would say Invalid output device just in case

Done.

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:619: int hardware_channels = 2;
On 2013/09/07 08:04:25, henrika wrote:
> Don't understand this "fallback" scheme. Do you?

Not really.  I guess there must be a case where we fail to get the stream
configuration but the rest works regardless?

https://codereview.chromium.org/23533045/diff/1/media/audio/mac/audio_manager...
media/audio/mac/audio_manager_mac.cc:636: // TODO(crogers): given the
limitations of the AudioOutputStream
On 2013/09/07 08:04:25, henrika wrote:
> Change crogers to xians?

Done.

[tommi (sloooow) - chröme, 2013-09-08 19:15:50]

Fix apparent sdk build problem

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

LGTM++

[commit-bot: I haz the power, 2013-09-10 04:25:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23533045/58001

[commit-bot: I haz the power, 2013-09-10 05:34:09]

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

[commit-bot: I haz the power, 2013-09-10 05:47:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23533045/58001

[commit-bot: I haz the power, 2013-09-10 06:43:57]

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

[commit-bot: I haz the power, 2013-09-10 08:21:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23533045/58001

[commit-bot: I haz the power, 2013-09-10 09:27:38]

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

[tommi (sloooow) - chröme, 2013-09-10 13:05:53]

Something weird seems to have happened.  This CL was actually landed in r222244
but somehow the issue did not get updated.