Move Alsa device opening into the audio thread, and add in support for multi-channel audio.

Move Alsa device opening into the audio thread, and add in support for multi-channel audio.

Moving the device opening into the audio thread will prevent browser hangs for badly behaving alsa implementations (like pulseaudio) that can hang snd_pcm_open if pulseaudiod is wedged, even if SND_PCM_NONBLOCK is requested.

For multi-channel audio, device enumeration has been added to try and find a multi-channel device with a stable channel mapping.  According to http://0pointer.de/blog/projects/guide-to-sound-apis.html, default should only be used with mono and stereo stream because the channel ordering is not defined by Alsa. To get a well-defined channel ordering, one must use one of the surround40, surround51, etc., device names.  However, these device names do not always allow multiple opens, so a fallback scheme is implemented to use default if necessary.

BUG=20945, 17703
TEST=listened with built-in soundcard and USB soundcard with various other audio programs running.

[awong, 2009-10-15 01:40:56]

This gets us much closer to correct multi-channel in Alsa.  We still need to
swizzle the channel order per what Frank suggested in the bug.

This CL still needs unittest fixes, but it's solid enough for review right now.

[fbarchard, 2009-10-15 02:31:19]

LGTM - the multichannel/volume looks fine.
It looks like it has limitations - like you can not change drivers while a video
is playing.
each stream will go thru the opening of a device and may end up with multiple
devices?

http://codereview.chromium.org/275022/diff/1001/1002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/1001/1002#newcode117
Line 117: return SND_PCM_FORMAT_S8;
this should be U8

http://codereview.chromium.org/275022/diff/1001/1002#newcode123
Line 123: return SND_PCM_FORMAT_S24;
likely wont work with folding or volume

http://codereview.chromium.org/275022/diff/1001/1002#newcode159
Line 159: return "surround40";
4 and 5 channel are uncommon.  but 7 and 8 are becoming more so... are there
devices for those?

[scherkus (not reviewing), 2009-10-15 03:13:48]

nits

http://codereview.chromium.org/275022/diff/1001/1002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/1001/1002#newcode98
Line 98: static const int kMinLatencyMicros = kSleepErrorMilliseconds * 2000;
I don't see much doubling here :)

comment mismatch?

http://codereview.chromium.org/275022/diff/1001/1002#newcode161
Line 161: default:
would 3 channels be "surround21" ?

http://codereview.chromium.org/275022/diff/1001/1002#newcode210
Line 210: latency_micros_(0),
micros_per_packet_ is not initialized

http://codereview.chromium.org/275022/diff/1001/1002#newcode383
Line 383: const int num_preroll = latency_micros_ / micros_per_packet_;
you cool with integer rounding?

http://codereview.chromium.org/275022/diff/1001/1002#newcode543
Line 543: // If we need to downmix, then the amount data acctually going to the
buffer
acctually -> actually

http://codereview.chromium.org/275022/diff/1001/1002#newcode546
Line 546: next_fill_time_ms /= channels_ / 2;
is integer math ok here?

If we went from 5 channels -> 2 channels, we'd have 3 less channels:
next_fill_time_ms = (next_fill_time_ms * channels) / 2

unless I'm missing something, then nevermind

http://codereview.chromium.org/275022/diff/1001/1002#newcode602
Line 602: void **hints = NULL;
pointer goes with type

http://codereview.chromium.org/275022/diff/1001/1002#newcode609
Line 609: for (void **hint_iter = hints; *hint_iter != NULL; hint_iter++) {
pointer goes with type

http://codereview.chromium.org/275022/diff/1001/1002#newcode619
Line 619: snd_device_name_get_hint(*hint_iter, kNameHintName));
are we not wrapping this ALSA call?

http://codereview.chromium.org/275022/diff/1001/1002#newcode711
Line 711: //   2) If that fails, attempt the "plug:" version of it incase Alsa
can
nit: Alsa -> ALSA

http://codereview.chromium.org/275022/diff/1001/1002#newcode719
Line 719: // Step 1
periods at the end of these step comments

[awong, 2009-10-15 23:57:01]

Fixed the comments, and also added unittests plus a command-line flag to
manually specify an alsa device.

http://codereview.chromium.org/275022/diff/1001/1002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/1001/1002#newcode98
Line 98: static const int kMinLatencyMicros = kSleepErrorMilliseconds * 2000;
On 2009/10/15 03:13:48, scherkus wrote:
> I don't see much doubling here :)
> 
> comment mismatch?

Ah...that's cause its *tricky* doubling. :)  It's * 2 * 1000 to convert millis
to  micros.  I changed it to * 2 * 1000.

http://codereview.chromium.org/275022/diff/1001/1002#newcode117
Line 117: return SND_PCM_FORMAT_S8;
On 2009/10/15 02:31:19, fbarchard wrote:
> this should be U8

Done.

http://codereview.chromium.org/275022/diff/1001/1002#newcode123
Line 123: return SND_PCM_FORMAT_S24;
On 2009/10/15 02:31:19, fbarchard wrote:
> likely wont work with folding or volume

Hmm...should I just disable these formats then?

http://codereview.chromium.org/275022/diff/1001/1002#newcode159
Line 159: return "surround40";
On 2009/10/15 02:31:19, fbarchard wrote:
> 4 and 5 channel are uncommon.  but 7 and 8 are becoming more so... are there
> devices for those?

Yes, there are surround70 and surround71 for those.  I added them in, but we
have no test data.

http://codereview.chromium.org/275022/diff/1001/1002#newcode161
Line 161: default:
On 2009/10/15 03:13:48, scherkus wrote:
> would 3 channels be "surround21" ?

I do not think so.  The "surroundXX" convention is only quasi-standard.  Only
"default" is guaranteed to exist. :(  

Here's the full output device list for our our USB X-Fi device gotten from
"aplay -L" for reference:

default:CARD=S51
    SB X-Fi Surround 5.1, USB Audio
    Default Audio Device
front:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    Front speakers
surround40:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    4.0 Surround output to Front and Rear speakers
surround41:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    4.1 Surround output to Front, Rear and Subwoofer speakers
surround50:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    5.0 Surround output to Front, Center and Rear speakers
surround51:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    5.1 Surround output to Front, Center, Rear and Subwoofer speakers
surround71:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    7.1 Surround output to Front, Center, Side, Rear and Woofer speakers
iec958:CARD=S51,DEV=0
    SB X-Fi Surround 5.1, USB Audio
    IEC958 (S/PDIF) Digital Audio Output

http://codereview.chromium.org/275022/diff/1001/1002#newcode210
Line 210: latency_micros_(0),
On 2009/10/15 03:13:48, scherkus wrote:
> micros_per_packet_ is not initialized

Good catch!

Stupid C++.

http://codereview.chromium.org/275022/diff/1001/1002#newcode383
Line 383: const int num_preroll = latency_micros_ / micros_per_packet_;
On 2009/10/15 03:13:48, scherkus wrote:
> you cool with integer rounding?

Yep.  Totally intentional.  Updated comment to state as much.

http://codereview.chromium.org/275022/diff/1001/1002#newcode543
Line 543: // If we need to downmix, then the amount data acctually going to the
buffer
On 2009/10/15 03:13:48, scherkus wrote:
> acctually -> actually

Done.

http://codereview.chromium.org/275022/diff/1001/1002#newcode546
Line 546: next_fill_time_ms /= channels_ / 2;
On 2009/10/15 03:13:48, scherkus wrote:
> is integer math ok here?
> 
> If we went from 5 channels -> 2 channels, we'd have 3 less channels:
> next_fill_time_ms = (next_fill_time_ms * channels) / 2
> 
> unless I'm missing something, then nevermind

You're right that doing the multiply first is better. I need to be more careful
with the math.

What do you think of the new solution?  Is the overflow check overkill?

http://codereview.chromium.org/275022/diff/1001/1002#newcode602
Line 602: void **hints = NULL;
On 2009/10/15 03:13:48, scherkus wrote:
> pointer goes with type

Done.

http://codereview.chromium.org/275022/diff/1001/1002#newcode609
Line 609: for (void **hint_iter = hints; *hint_iter != NULL; hint_iter++) {
On 2009/10/15 03:13:48, scherkus wrote:
> pointer goes with type

Done.

http://codereview.chromium.org/275022/diff/1001/1002#newcode619
Line 619: snd_device_name_get_hint(*hint_iter, kNameHintName));
On 2009/10/15 03:13:48, scherkus wrote:
> are we not wrapping this ALSA call?

Oops. Just missed the reference.  The unittest should ensure I don't miss
anymore.

http://codereview.chromium.org/275022/diff/1001/1002#newcode711
Line 711: //   2) If that fails, attempt the "plug:" version of it incase Alsa
can
On 2009/10/15 03:13:48, scherkus wrote:
> nit: Alsa -> ALSA

Done.

http://codereview.chromium.org/275022/diff/1001/1002#newcode719
Line 719: // Step 1
On 2009/10/15 03:13:48, scherkus wrote:
> periods at the end of these step comments

Done.

[scherkus (not reviewing), 2009-10-16 00:08:11]

LGTM with nits

http://codereview.chromium.org/275022/diff/1001/1002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/1001/1002#newcode546
Line 546: next_fill_time_ms /= channels_ / 2;
On 2009/10/15 23:57:01, awong wrote:
> On 2009/10/15 03:13:48, scherkus wrote:
> > is integer math ok here?
> > 
> > If we went from 5 channels -> 2 channels, we'd have 3 less channels:
> > next_fill_time_ms = (next_fill_time_ms * channels) / 2
> > 
> > unless I'm missing something, then nevermind
> 
> You're right that doing the multiply first is better. I need to be more
careful
> with the math.
> 
> What do you think of the new solution?  Is the overflow check overkill?

yeah looks safe enough -- is comment out of date?

http://codereview.chromium.org/275022/diff/5001/6003
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/275022/diff/5001/6003#newcode332
Line 332: EXPECT_TRUE(test_stream_->playback_handle_ == NULL);
heads up: there's that guy going around making sure we compare to 0 instead of
NULL

http://codereview.chromium.org/275022/diff/5001/6003#newcode364
Line 364: EXPECT_TRUE(test_stream_->playback_handle_ == NULL);
heads up: there's that guy going around making sure we compare to 0 instead of
NULL

http://codereview.chromium.org/275022/diff/5001/6008
File media/base/media_switches.h (right):

http://codereview.chromium.org/275022/diff/5001/6008#newcode12
Line 12: #if defined(OS_LINUX)
nit: this header file is not hermetic

you should include build/build_config.h at the top similar to chrome_switches.h

[awong, 2009-10-16 01:27:27]

Once more!

Turns out that it wasn't just my comment that was out of date...the whole
packet/frames calcuations for when downmixing was completely broken.

I've fixed it, and it seems to remove the need for the phantom /2 in the timing
code.

-Albert

[scherkus (not reviewing), 2009-10-16 01:44:34]

http://codereview.chromium.org/275022/diff/7003/7004
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/7003/7004#newcode176
Line 176: static void Swizzle50Layout(Format *b, size_t filled) {
pointer goes with type

http://codereview.chromium.org/275022/diff/7003/7004#newcode188
Line 188: template<class Format>
nit: add blank line above template<class Format>

http://codereview.chromium.org/275022/diff/7003/7004#newcode189
Line 189: static void Swizzle51Layout(Format *b, size_t filled) {
pointer goes with type

http://codereview.chromium.org/275022/diff/7003/7004#newcode505
Line 505: packet->size = packet->size / bytes_per_frame_ *
bytes_per_output_frame_;
>80 chars

http://codereview.chromium.org/275022/diff/7003/7004#newcode508
Line 508: }
nit: you can return here to avoid nesting in the else clause

http://codereview.chromium.org/275022/diff/7003/7004#newcode615
Line 615: int frames_leftover = FramesInPacket(*current_packet,
bytes_per_output_frame_);
>80 chars

http://codereview.chromium.org/275022/diff/7003/7004#newcode617
Line 617: (frames_leftover > 0) ? frames_leftover : frames_per_packet_;
hrmm. so FramesInPacket() would be <0 if current_packet.used >
current_packet.size?

when would that happen?  that seems bad

http://codereview.chromium.org/275022/diff/7003/7004#newcode623
Line 623: FramesToMillis(frames_until_empty_enough, sample_rate_);
I'm not seeing a division by two.. unless I'm mistaken

http://codereview.chromium.org/275022/diff/7003/7004#newcode813
Line 813: if (default_channels > 3 && default_channels < 9) {
magic number alert!

[Alpha Left Google, 2009-10-16 01:47:16]

I see this is failing media_unittests, is this being taken care of yet?

[awong, 2009-10-16 01:49:33]

http://codereview.chromium.org/275022/diff/7003/7004
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/7003/7004#newcode176
Line 176: static void Swizzle50Layout(Format *b, size_t filled) {
On 2009/10/16 01:44:35, scherkus wrote:
> pointer goes with type

Done.

http://codereview.chromium.org/275022/diff/7003/7004#newcode188
Line 188: template<class Format>
On 2009/10/16 01:44:35, scherkus wrote:
> nit: add blank line above template<class Format>

Done.

http://codereview.chromium.org/275022/diff/7003/7004#newcode189
Line 189: static void Swizzle51Layout(Format *b, size_t filled) {
On 2009/10/16 01:44:35, scherkus wrote:
> pointer goes with type

Done.

http://codereview.chromium.org/275022/diff/7003/7004#newcode505
Line 505: packet->size = packet->size / bytes_per_frame_ *
bytes_per_output_frame_;
On 2009/10/16 01:44:35, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/275022/diff/7003/7004#newcode508
Line 508: }
On 2009/10/16 01:44:35, scherkus wrote:
> nit: you can return here to avoid nesting in the else clause

In this case, I'm not convinced that's cleaner.

http://codereview.chromium.org/275022/diff/7003/7004#newcode615
Line 615: int frames_leftover = FramesInPacket(*current_packet,
bytes_per_output_frame_);
On 2009/10/16 01:44:35, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/275022/diff/7003/7004#newcode617
Line 617: (frames_leftover > 0) ? frames_leftover : frames_per_packet_;
On 2009/10/16 01:44:35, scherkus wrote:
> hrmm. so FramesInPacket() would be <0 if current_packet.used >
> current_packet.size?
> 
> when would that happen?  that seems bad

The case to check against is actually current_package.used ==
current_packet.size, ie., when the packet is current completely empty.

http://codereview.chromium.org/275022/diff/7003/7004#newcode623
Line 623: FramesToMillis(frames_until_empty_enough, sample_rate_);
On 2009/10/16 01:44:35, scherkus wrote:
> I'm not seeing a division by two.. unless I'm mistaken

Stale comment.  Removed.

[scherkus (not reviewing), 2009-10-16 01:56:08]

LGTM!

http://codereview.chromium.org/275022/diff/7003/7004
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/275022/diff/7003/7004#newcode617
Line 617: (frames_leftover > 0) ? frames_leftover : frames_per_packet_;
On 2009/10/16 01:49:33, awong wrote:
> On 2009/10/16 01:44:35, scherkus wrote:
> > hrmm. so FramesInPacket() would be <0 if current_packet.used >
> > current_packet.size?
> > 
> > when would that happen?  that seems bad
> 
> The case to check against is actually current_package.used ==
> current_packet.size, ie., when the packet is current completely empty.

doh.. forgot about that number inbetween -1 and +1 :)

http://codereview.chromium.org/275022/diff/7013/9002#newcode531
Line 531: } else if (bytes_per_sample_ == 2) {
nice catch :)

[awong, 2009-10-16 02:13:25]

And for Alpha...test fixes. :)

[scherkus (not reviewing), 2009-10-16 03:25:18]

LGTM

[fbarchard, 2009-10-16 15:52:13]

LGTM.  Concerns for future work (todo?)
1. does not deal well with multiple movies.  devices will conflict
2. does not deal with changing of device part way thru a movie.
3. will need more work for channels and bit depths (same as other drivers)