Add AudioManager::GetAudioOutputDeviceNames and implement for pulseaudio.

Add AudioManager::GetAudioOutputDeviceNames and implement for pulseaudio.

TBR=vrk@chromium.org
BUG=276894
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221484

[Jói, 2013-09-03 12:54:20]

This is a quick strawman to get answers to a few questions. Maybe it's the final
implementation though, who knows :)

Questions:
a) Is bug 276894 the right bug for this, or would you prefer a separate bug to
track?
b) Should there be the concept of a default device at the start of the list of
output devices, similar to the one for input devices?
c) Does it make sense to start like this, just retrieving the label and ID, then
fill in the other information we may need for devices separately?

Cheers,
Jói

[tommi (sloooow) - chröme, 2013-09-03 15:57:15]

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:297: NOTREACHED() << "Not implemented on this
platform.";
NOTIMPLEMENTED?

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:21: TEST_F(AudioManagerTest,
GetAudioOutputDeviceNames) {
What about putting this with the audiomanagerbase tests?  Seeing as all the
implementations derive from that class and the default implementation is there
(albeit not implemented), this would still "work".

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:29: VLOG(2) << "Device ID(" <<
it->unique_id << "), label: " << it->device_name;
expect neither to be empty?

https://codereview.chromium.org/23453022/diff/6001/media/audio/pulse/audio_ma...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/pulse/audio_ma...
media/audio/pulse/audio_manager_pulse.cc:112:
WaitForOperationCompletion(input_mainloop_, operation);
I think we should also add the default device when the array is non-empty to be
consistent with the input side.

[Jói, 2013-09-04 09:11:03]

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:297: NOTREACHED() << "Not implemented on this
platform.";
On 2013/09/03 15:57:15, tommi wrote:
> NOTIMPLEMENTED?

Done.

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:21: TEST_F(AudioManagerTest,
GetAudioOutputDeviceNames) {
On 2013/09/03 15:57:15, tommi wrote:
> What about putting this with the audiomanagerbase tests?  Seeing as all the
> implementations derive from that class and the default implementation is there
> (albeit not implemented), this would still "work".

There is no audio_manager_base_unittests.cc or any other file specifically for
AudioManager tests. Did you mean audio_input_device_unittest.cc? There are some
tests of AudioManager device enumeration there, so I could move them there if
you think that's better, let me know.

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:29: VLOG(2) << "Device ID(" <<
it->unique_id << "), label: " << it->device_name;
On 2013/09/03 15:57:15, tommi wrote:
> expect neither to be empty?

Done.

https://codereview.chromium.org/23453022/diff/6001/media/audio/pulse/audio_ma...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/pulse/audio_ma...
media/audio/pulse/audio_manager_pulse.cc:112:
WaitForOperationCompletion(input_mainloop_, operation);
On 2013/09/03 15:57:15, tommi wrote:
> I think we should also add the default device when the array is non-empty to
be
> consistent with the input side.

Done.

[tommi (sloooow) - chröme, 2013-09-04 10:48:10]

lgtm.

FYI - I have another cl on the way which adds functionality that I think will be
useful to you when it comes time to fetching the audio parameters for the
enumerated devices:
https://codereview.chromium.org/23523023/

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/23453022/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:21: TEST_F(AudioManagerTest,
GetAudioOutputDeviceNames) {
On 2013/09/04 09:11:04, Jói wrote:
> On 2013/09/03 15:57:15, tommi wrote:
> > What about putting this with the audiomanagerbase tests?  Seeing as all the
> > implementations derive from that class and the default implementation is
there
> > (albeit not implemented), this would still "work".
> 
> There is no audio_manager_base_unittests.cc or any other file specifically for
> AudioManager tests. Did you mean audio_input_device_unittest.cc? There are
some
> tests of AudioManager device enumeration there, so I could move them there if
> you think that's better, let me know.

Sorry, what I was looking at were the platform specific test files. I think it's
a good idea to add this file and start having cross platform tests for the audio
manager (I thought we had those already but apparently we don't!).

[Jói, 2013-09-04 11:03:53]

TBR=vrk@ for adding the audio_manager_unittest.cc file to media.gyp

[commit-bot: I haz the power, 2013-09-04 16:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/23453022/14001

[commit-bot: I haz the power, 2013-09-04 17:15:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-05 08:30:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/23453022/47001

[commit-bot: I haz the power, 2013-09-05 08:55:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/23453022/70001

[commit-bot: I haz the power, 2013-09-05 11:25:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/23453022/64001

[commit-bot: I haz the power, 2013-09-05 19:02:47]

Change committed as 221484