Output device enumeration for Alsa, plus unit test.

Output device enumeration for Alsa, plus unit test.

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

[Jói, 2013-09-03 15:47:01]

Based on https://codereview.chromium.org/23453022/

Cheers,
Jói

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

PTAL, I've merged in changes from the parent change
(https://codereview.chromium.org/23453022/) and modified the logic so that the
default device is also added for a non-empty list of output devices, as it was
for input devices.

Cheers,
Jói

[tommi (sloooow) - chröme, 2013-09-04 13:43:29]

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

https://codereview.chromium.org/23480030/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:35: scoped_ptr<AudioManager>
pulse_audio_manager(AudioManagerPulse::Create());
What about:
scoped_ptr<AudioManager> audio_manager(AudioManager::Create());

and remove the OS guards and audio manager specific construction?
(the current build config should make that decision)

[Jói, 2013-09-04 13:44:41]

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

https://codereview.chromium.org/23480030/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:35: scoped_ptr<AudioManager>
pulse_audio_manager(AudioManagerPulse::Create());
On 2013/09/04 13:43:29, tommi wrote:
> What about:
> scoped_ptr<AudioManager> audio_manager(AudioManager::Create());
> 
> and remove the OS guards and audio manager specific construction?
> (the current build config should make that decision)

If I do that, I only get to test either pulseaudio or Alsa on Linux, depending
on whether the pulseaudio daemon is running.

By doing it this way, I get to test both.

[tommi (sloooow) - chröme, 2013-09-04 14:32:50]

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

https://codereview.chromium.org/23480030/diff/6001/media/audio/audio_manager_...
media/audio/audio_manager_unittest.cc:35: scoped_ptr<AudioManager>
pulse_audio_manager(AudioManagerPulse::Create());
On 2013/09/04 13:44:42, Jói wrote:
> On 2013/09/04 13:43:29, tommi wrote:
> > What about:
> > scoped_ptr<AudioManager> audio_manager(AudioManager::Create());
> > 
> > and remove the OS guards and audio manager specific construction?
> > (the current build config should make that decision)
> 
> If I do that, I only get to test either pulseaudio or Alsa on Linux, depending
> on whether the pulseaudio daemon is running.
> 
> By doing it this way, I get to test both.

OK, sounds good this way.

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.cc:209: bool
AudioManagerLinux::IsAlsaDeviceAvailable(
nit: looks like this could also be a static utility function

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.cc:218: if
(strncmp(kInvalidAudioInputDevices[i], device_name,
Don't we also have to check that the length of the strings are equal or is the
intent to do prefix matching?

also as an absolute-ocd-nit, it would be nice to use the arraysize() approach
like you do below for the kInvalidAudioInputDevices[] array (in which case it'd
be an array of a string+length type).

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
File media/audio/linux/audio_manager_linux.h (right):

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.h:72: const char*
UnwantedDeviceTypeWhenEnumerating(StreamType wanted_type);
Can this be just a static method in the cc file? (does it need to be a member
function?)

[Jói, 2013-09-04 15:32:21]

PTAL

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.cc:209: bool
AudioManagerLinux::IsAlsaDeviceAvailable(
On 2013/09/04 14:32:51, tommi wrote:
> nit: looks like this could also be a static utility function

Becuase of the use of StreamType this has the same tradeoffs as with
UnwantedDeviceTypeWhenEnumerating.

I made both this and UnwantedDeviceTypeWhenEnumerating static though, no need to
waste a good this pointer.

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.cc:218: if
(strncmp(kInvalidAudioInputDevices[i], device_name,
On 2013/09/04 14:32:51, tommi wrote:
> Don't we also have to check that the length of the strings are equal or is the
> intent to do prefix matching?

Prefix matching is the intent. I added a comment.

> also as an absolute-ocd-nit, it would be nice to use the arraysize() approach
> like you do below for the kInvalidAudioInputDevices[] array (in which case
it'd
> be an array of a string+length type).

I already tried that :)  Can't make it a fixed-size two-dimensional array since
the strings are of different lengths.

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
File media/audio/linux/audio_manager_linux.h (right):

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.h:72: const char*
UnwantedDeviceTypeWhenEnumerating(StreamType wanted_type);
On 2013/09/04 14:32:51, tommi wrote:
> Can this be just a static method in the cc file? (does it need to be a member
> function?)

In order to let it use the StreamType enum, we have options:
a) Keep like this;
b) Make StreamType public;
c) Make the function a friend; or last but definitely not least,
d) Switch all of the private implementation to a pImpl (OVERKILL :)

I thought it was just most straightforward like this.

[tommi (sloooow) - chröme, 2013-09-04 16:43:52]

lgtm. feel free to go nuts on the static array... or not

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
media/audio/linux/audio_manager_linux.cc:218: if
(strncmp(kInvalidAudioInputDevices[i], device_name,
On 2013/09/04 15:32:22, Jói wrote:
> On 2013/09/04 14:32:51, tommi wrote:
> > Don't we also have to check that the length of the strings are equal or is
the
> > intent to do prefix matching?
> 
> Prefix matching is the intent. I added a comment.

Great. thanks.

> 
> > also as an absolute-ocd-nit, it would be nice to use the arraysize()
approach
> > like you do below for the kInvalidAudioInputDevices[] array (in which case
> it'd
> > be an array of a string+length type).
> 
> I already tried that :)  Can't make it a fixed-size two-dimensional array
since
> the strings are of different lengths.

ah, I see.  What about something like:

#define INVALID_AUDIO_DEVICE_ENTRY(entry) \
    { entry, arraysize(entry) - 1}
struct {
  const char* name;
  const int length;
} const kInvalidAudioInputDevices[] = {
  INVALID_AUDIO_DEVICE_ENTRY("default"),
  INVALID_AUDIO_DEVICE_ENTRY("foo"),
  ...
};
#undef INVALID_AUDIO_DEVICE_ENTRY

[Jói, 2013-09-04 17:15:42]

> ah, I see.  What about something like:
>
> #define INVALID_AUDIO_DEVICE_ENTRY(entry) \
>     { entry, arraysize(entry) - 1}
> struct {
>   const char* name;
>   const int length;
> } const kInvalidAudioInputDevices[] = {
>   INVALID_AUDIO_DEVICE_ENTRY("default"),
>   INVALID_AUDIO_DEVICE_ENTRY("foo"),
>   ...
> };
> #undef INVALID_AUDIO_DEVICE_ENTRY

I'm tempted to do that but it's perhaps not very readable... I could
go either way.

On Wed, Sep 4, 2013 at 6:43 PM,  <tommi@chromium.org> wrote:
> lgtm. feel free to go nuts on the static array... or not
>
>
>
>
https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
> File media/audio/linux/audio_manager_linux.cc (right):
>
>
https://codereview.chromium.org/23480030/diff/6001/media/audio/linux/audio_ma...
> media/audio/linux/audio_manager_linux.cc:218: if
> (strncmp(kInvalidAudioInputDevices[i], device_name,
> On 2013/09/04 15:32:22, Jói wrote:
>>
>> On 2013/09/04 14:32:51, tommi wrote:
>> > Don't we also have to check that the length of the strings are equal
>
> or is the
>>
>> > intent to do prefix matching?
>
>
>> Prefix matching is the intent. I added a comment.
>
>
> Great. thanks.
>
>
>
>> > also as an absolute-ocd-nit, it would be nice to use the arraysize()
>
> approach
>>
>> > like you do below for the kInvalidAudioInputDevices[] array (in
>
> which case
>>
>> it'd
>> > be an array of a string+length type).
>
>
>> I already tried that :)  Can't make it a fixed-size two-dimensional
>
> array since
>>
>> the strings are of different lengths.
>
>
> ah, I see.  What about something like:
>
> #define INVALID_AUDIO_DEVICE_ENTRY(entry) \
>     { entry, arraysize(entry) - 1}
> struct {
>   const char* name;
>   const int length;
> } const kInvalidAudioInputDevices[] = {
>   INVALID_AUDIO_DEVICE_ENTRY("default"),
>   INVALID_AUDIO_DEVICE_ENTRY("foo"),
>   ...
> };
> #undef INVALID_AUDIO_DEVICE_ENTRY
>
> https://codereview.chromium.org/23480030/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2013-09-05 11:19:44]

PTAL, from tryjobs I realized I need to match the use_pulseaudio and use_alsa
logic more closely, so there are minor modifications to
audio_manager_unittest.cc and the media.gyp file.

Also, I haven't switched to the string plus length approach for storing the list
of unwanted input device types, as I felt that it hurt readability, but I could
go either way, let me know which you think is best.

Cheers,
Jói

[tommi (sloooow) - chröme, 2013-09-05 11:54:45]

lgtm.  the string thing isn't worth it :)

https://codereview.chromium.org/23480030/diff/22001/media/audio/audio_manager...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/23480030/diff/22001/media/audio/audio_manager...
media/audio/audio_manager_unittest.cc:42: VLOG(2) << "No pulseaudio on this
system.";
LOG(WARNING)?  That way we'll be able to see from the logs if pulse is being
tested or not (even though the test might be passing).

[Jói, 2013-09-05 12:10:55]

Done.

On Thu, Sep 5, 2013 at 1:54 PM,  <tommi@chromium.org> wrote:
> lgtm.  the string thing isn't worth it :)
>
>
>
https://codereview.chromium.org/23480030/diff/22001/media/audio/audio_manager...
> File media/audio/audio_manager_unittest.cc (right):
>
>
https://codereview.chromium.org/23480030/diff/22001/media/audio/audio_manager...
> media/audio/audio_manager_unittest.cc:42: VLOG(2) << "No pulseaudio on
> this system.";
> LOG(WARNING)?  That way we'll be able to see from the logs if pulse is
> being tested or not (even though the test might be passing).
>
> https://codereview.chromium.org/23480030/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-06 10:02:11]

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

[commit-bot: I haz the power, 2013-09-06 10:19:16]

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

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

TBR=vrk@ for media.gyp change.

[commit-bot: I haz the power, 2013-09-06 11:43:18]

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

[commit-bot: I haz the power, 2013-09-06 14:24:28]

Change committed as 221704