Implement audio recording for Linux via ALSA.

Implement audio recording for Linux via ALSA.
There are no new unit tests because a cross platform unit test added in CL 3357004 covers this code.

BUG=53598
TEST=media_unittests --gtest_filter=AudioInputTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58409

[Sergey Ulanov, 2010-09-02 18:45:00]

Mostly nits, looks good otherwise. You also need unittests for
AlsaPcmInputStream.

BTW, I recently changed AudioOutputController so that it always uses
AudioManager's thread, and doesn't create its own thread. You may want to do the
same thing for input controller.

http://codereview.chromium.org/3299005/diff/1/2
File media/audio/linux/alsa_input.cc (right):

http://codereview.chromium.org/3299005/diff/1/2#newcode90
media/audio/linux/alsa_input.cc:90: snd_pcm_format_t pcm_format =
SND_PCM_FORMAT_UNKNOWN;
Maybe move the code that finds pcm_format for give bits_per_sample_ to a
separate function.

http://codereview.chromium.org/3299005/diff/1/2#newcode102
media/audio/linux/alsa_input.cc:102: base::Time::kMicrosecondsPerMillisecond;
nit: This line must be aligned 4 chars to the right from beginning of the
previous line.

http://codereview.chromium.org/3299005/diff/1/2#newcode108
media/audio/linux/alsa_input.cc:108: sample_rate_, pcm_format, latency_us);
nit: align this line with wrapper_ on the previous line.

http://codereview.chromium.org/3299005/diff/1/2#newcode115
media/audio/linux/alsa_input.cc:115: audio_packet_.reset(new
uint8[bytes_per_packet_]);
De we need to allocate memory if the device wasn't opened?

http://codereview.chromium.org/3299005/diff/1/2#newcode132
media/audio/linux/alsa_input.cc:132: if (error) {
error < 0

http://codereview.chromium.org/3299005/diff/1/2#newcode150
media/audio/linux/alsa_input.cc:150: error =
wrapper_->PcmRecover(device_handle_, original_error, 1);
can be merged with the previous line, i.e. "int error = ...".

http://codereview.chromium.org/3299005/diff/1/2#newcode174
media/audio/linux/alsa_input.cc:174: void AlsaPcmInputStream::ReadAudio() {
DCHECK that callback_ != NULL?

http://codereview.chromium.org/3299005/diff/1/3
File media/audio/linux/alsa_input.h (right):

http://codereview.chromium.org/3299005/diff/1/3#newcode53
media/audio/linux/alsa_input.h:53: int channels_;
You can use AudioParameters struct here, instead of three separate fields.

http://codereview.chromium.org/3299005/diff/1/7
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/3299005/diff/1/7#newcode31
media/audio/linux/audio_manager_linux.cc:31: // TODO(satish): implement.
Remove TODO. Or copy ajwong's TODO from HasAudioOutputDevices().

http://codereview.chromium.org/3299005/diff/1/7#newcode77
media/audio/linux/audio_manager_linux.cc:77: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kAlsaDevice)) {
I think it is better to add a separate switch for input device.

[scherkus (not reviewing), 2010-09-02 19:08:22]

nits

deferring to seregyu/ajwong -- masters of linux audio!

http://codereview.chromium.org/3299005/diff/1/2
File media/audio/linux/alsa_input.cc (right):

http://codereview.chromium.org/3299005/diff/1/2#newcode21
media/audio/linux/alsa_input.cc:21: const char* kDefaultDevice1 = "default";
typically use array format:
const char kDefaultDevice1[] = "default";

http://codereview.chromium.org/3299005/diff/1/2#newcode65
media/audio/linux/alsa_input.cc:65: const char*
AlsaPcmInputStream::kAutoSelectDevice = "";
ditto

http://codereview.chromium.org/3299005/diff/1/2#newcode92
media/audio/linux/alsa_input.cc:92: pcm_format == SND_PCM_FORMAT_UNKNOWN; ++i) {
indentation

http://codereview.chromium.org/3299005/diff/1/3
File media/audio/linux/alsa_input.h (right):

http://codereview.chromium.org/3299005/diff/1/3#newcode53
media/audio/linux/alsa_input.h:53: int channels_;
we can use AudioParameters here

[Satish, 2010-09-02 19:35:31]

Thanks for the comments, I have addressed all comments except the "const char[]"
one (will change based on the outcome of chromium-dev mail thread).

Sergey, this new code is covered by existing unit tests as mentioned in the CL
description, hence I did not add a new unit test file.

[awong, 2010-09-02 19:53:53]

http://codereview.chromium.org/3299005/diff/8001/9001
File media/audio/linux/alsa_input.cc (right):

http://codereview.chromium.org/3299005/diff/8001/9001#newcode27
media/audio/linux/alsa_input.cc:27: } kBitsToFormatMap[] = {
This looks a bit awkward.  Can we split the type declaration from the array
declaration into two lines?

http://codereview.chromium.org/3299005/diff/8001/9001#newcode34
media/audio/linux/alsa_input.cc:34: 
remove extra newline.

http://codereview.chromium.org/3299005/diff/8001/9001#newcode35
media/audio/linux/alsa_input.cc:35: snd_pcm_format_t BitsToFormat(int
bits_per_sample) {
Since this is used in both the AlsaPCmOutputStream and in the input stream, can
we pull it out into a shared utility function?

Also, is there a reason to prefer iterating over a mapping struct over the
switch statement?  The switch seems a little more explicit.

http://codereview.chromium.org/3299005/diff/8001/9001#newcode43
media/audio/linux/alsa_input.cc:43: snd_pcm_t* OpenDevice(AlsaWrapper* wrapper,
Hmm...this is starting to look like a lot of forked code, both in the scheduling
& device opening/closing.  I know you wanted to avoid the threading complexity,
but is there any way to combine some of this?

Thinking out loud...I wonder if it'd be sensible to just have a AlsaPcmStream
that could be opened in read or write mode.  Or maybe have a utility class that
opened the stream for us.

These would be biggish refactors.  If you're trying to hit a dev channel
deadline tonight, I'm okay with TODOing them as long as we revisit the fork
soonish.

http://codereview.chromium.org/3299005/diff/8001/9001#newcode53
media/audio/linux/alsa_input.cc:53: DLOG(WARNING) << "PcmOpen: " << device_name
<< ","
Do we not want this logging in production?

http://codereview.chromium.org/3299005/diff/8001/9001#newcode122
media/audio/linux/alsa_input.cc:122: return device_handle_ ? true : false;
Just return device_handle != NULL?

http://codereview.chromium.org/3299005/diff/8001/9002
File media/audio/linux/alsa_input.h (right):

http://codereview.chromium.org/3299005/diff/8001/9002#newcode8
media/audio/linux/alsa_input.h:8: #include <alsa/asoundlib.h>
newline between C and C++ headers please.

http://codereview.chromium.org/3299005/diff/8001/9002#newcode68
media/audio/linux/alsa_input.h:68: 
remove extra newline.

[Satish, 2010-09-02 21:12:29]

Thanks for the comments. As we agreed over chat, I have moved common code into
alsa_util.cc/.h. Also changed all DLOG to LOG calls and other style comments.
Please take another look.

[awong, 2010-09-02 21:16:07]

LGTM with a couple of style nits.

http://codereview.chromium.org/3299005/diff/13001/14006
File media/audio/linux/alsa_util.cc (right):

http://codereview.chromium.org/3299005/diff/13001/14006#newcode46
media/audio/linux/alsa_util.cc:46: }
// namespace

http://codereview.chromium.org/3299005/diff/13001/14006#newcode100
media/audio/linux/alsa_util.cc:100: }
// namespace alsa_util

remove extra newline.