Reimplement the AlsaPcmOutputStream and fix the threading issues.

Reimplement the AlsaPcmOutputStream and fix the threading issues.

Changes the threading model from one thread per stream to one shared thread.  Also, redoes the alsa reading/buffering logic to assume an asynchronous data source, and respect the different packet sizes.  The Alsa device is set to non-blocking now.  State transitions are cleaned up, and the threading semantics are reworked.  Now linux audio will no longer crash on shutdown, seek, pause, or tab close.  This implementation does still leak though. :(  The leak will be fixed in another CL.

[awong, 2009-08-01 04:09:04]

This is a rewrite of the AlsaPcmOutputStream.  The unittest still needs to be
beefed up a bit (it only covers the Open() function for now), but the code is
ready for review.  i'll add more tests on monday.

agl@, I CCed you in case you were interested in looking over the Alsa changes.

[Alpha Left Google, 2009-08-03 22:26:18]

http://codereview.chromium.org/160497/diff/36/38
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/160497/diff/36/38#newcode148
Line 148: bytes_per_sample_(bits_per_sample/8),
Space before and after /

http://codereview.chromium.org/160497/diff/36/38#newcode208
Line 208: // Configure the device for software resampling, and and enough buffer
for
and add enough buffer?

http://codereview.chromium.org/160497/diff/36/38#newcode218
Line 218: micros_per_packet * 2)) < 0) {
Can you explain why we need to double the size?

http://codereview.chromium.org/160497/diff/36/38#newcode228
Line 228: // We do not need to heck if the transition was successful because
do not need to check

http://codereview.chromium.org/160497/diff/36/38#newcode306
Line 306: // When starting again, prepare the playback device incase we are
restarting
in case

http://codereview.chromium.org/160497/diff/36/38#newcode471
Line 471: PostDelayedTask(
If we don't see an actual problem of using PostDelayedTask I'm fine with it.

http://codereview.chromium.org/160497/diff/36/38#newcode553
Line 553: int64 AlsaPcmOutputStream::FramesToMicros(int frames, int sample_rate)
{
Can these conversion methods be moved to anonymous namespace?

http://codereview.chromium.org/160497/diff/36/38#newcode562
Line 562: bool AlsaPcmOutputStream::CloseDevice(snd_pcm_t* handle) {
Can you move this method before the implementation of
AlsaPcmOutputStream::SharedData?

http://codereview.chromium.org/160497/diff/36/38#newcode573
Line 573: snd_pcm_sframes_t AlsaPcmOutputStream::GetAvailableFrames() {
Move this method up too.

http://codereview.chromium.org/160497/diff/36/39
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/160497/diff/36/39#newcode18
Line 18: // This object is thread-safety is a little tricky.  This object's
public API
This object's thread-safety is a little tricky?

http://codereview.chromium.org/160497/diff/36/39#newcode58
Line 58: AlsaWrapper* wrapper,
Can you explain a bit about what is AlsaWrapper and |message_loop| does?

http://codereview.chromium.org/160497/diff/36/39#newcode72
Line 72: virtual void PostTask(
Instead of making these PostTask and friends virtual I'd make the actual task
methods virtual. See unit tests for more comments.

http://codereview.chromium.org/160497/diff/36/40
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/160497/diff/36/40#newcode176
Line 176: EXPECT_CALL(*test_stream_, PostTask(_, _));
If we expect the actual task method instead of the PostTask helper, we can do
things like:

EXPECT_CALL(*test_stream_, FinishOpen, kFakeHandle, _);

ASSERT_TRUE(test_stream_->Open(kTestPacketSize));

// This method will run all the immediate task posted and return. 
message_loop_.RunAllPending();

EXPECT_EQ(...);

http://codereview.chromium.org/160497/diff/36/42
File media/audio/linux/alsa_wrapper.h (right):

http://codereview.chromium.org/160497/diff/36/42#newcode13
Line 13: class AlsaWrapper {
This is totally awsome, we should have FFmpegWrapper for testing FFmpeg.. Gives
us access to real ffmpeg implementation.

http://codereview.chromium.org/160497/diff/36/43
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/160497/diff/36/43#newcode48
Line 48: active_streams_[stream] = scoped_refptr<AlsaPcmOutputStream>(stream);
Hum...possible dangling reference.. Is there any use to keep a list of active
streams?

[awong, 2009-08-03 22:54:18]

http://codereview.chromium.org/160497/diff/36/38
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/160497/diff/36/38#newcode208
Line 208: // Configure the device for software resampling, and and enough buffer
for
On 2009/08/03 22:26:18, Alpha wrote:
> and add enough buffer?

Done.

http://codereview.chromium.org/160497/diff/36/38#newcode218
Line 218: micros_per_packet * 2)) < 0) {
On 2009/08/03 22:26:18, Alpha wrote:
> Can you explain why we need to double the size?

I think the comment above explains it...

http://codereview.chromium.org/160497/diff/36/38#newcode228
Line 228: // We do not need to heck if the transition was successful because
On 2009/08/03 22:26:18, Alpha wrote:
> do not need to check

Done.

http://codereview.chromium.org/160497/diff/36/38#newcode306
Line 306: // When starting again, prepare the playback device incase we are
restarting
On 2009/08/03 22:26:18, Alpha wrote:
> in case

Done.

http://codereview.chromium.org/160497/diff/36/38#newcode471
Line 471: PostDelayedTask(
On 2009/08/03 22:26:18, Alpha wrote:
> If we don't see an actual problem of using PostDelayedTask I'm fine with it.

I agree. I'm leaving the TODO in though cause I'd like to get some hard data.

http://codereview.chromium.org/160497/diff/36/38#newcode553
Line 553: int64 AlsaPcmOutputStream::FramesToMicros(int frames, int sample_rate)
{
On 2009/08/03 22:26:18, Alpha wrote:
> Can these conversion methods be moved to anonymous namespace?

They could. I prefer leaving them in the class though so I can access them in
the untittest.

http://codereview.chromium.org/160497/diff/36/38#newcode562
Line 562: bool AlsaPcmOutputStream::CloseDevice(snd_pcm_t* handle) {
On 2009/08/03 22:26:18, Alpha wrote:
> Can you move this method before the implementation of
> AlsaPcmOutputStream::SharedData?

Done.

http://codereview.chromium.org/160497/diff/36/38#newcode573
Line 573: snd_pcm_sframes_t AlsaPcmOutputStream::GetAvailableFrames() {
On 2009/08/03 22:26:18, Alpha wrote:
> Move this method up too.

Done.

http://codereview.chromium.org/160497/diff/36/39
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/160497/diff/36/39#newcode18
Line 18: // This object is thread-safety is a little tricky.  This object's
public API
On 2009/08/03 22:26:18, Alpha wrote:
> This object's thread-safety is a little tricky?

Done.

http://codereview.chromium.org/160497/diff/36/39#newcode58
Line 58: AlsaWrapper* wrapper,
On 2009/08/03 22:26:18, Alpha wrote:
> Can you explain a bit about what is AlsaWrapper and |message_loop| does?

Done.

http://codereview.chromium.org/160497/diff/36/39#newcode72
Line 72: virtual void PostTask(
On 2009/08/03 22:26:18, Alpha wrote:
> Instead of making these PostTask and friends virtual I'd make the actual task
> methods virtual. See unit tests for more comments.

Yeah, I wasn't sure where to put the testing hook.  I'll reexamine this. on the
next pass.  I don't really like the idea of mocking out the Task methods either
because using a partial mock in that manner is a bit tacky.

http://codereview.chromium.org/160497/diff/36/40
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/160497/diff/36/40#newcode176
Line 176: EXPECT_CALL(*test_stream_, PostTask(_, _));
On 2009/08/03 22:26:18, Alpha wrote:
> If we expect the actual task method instead of the PostTask helper, we can do
> things like:
> 
> EXPECT_CALL(*test_stream_, FinishOpen, kFakeHandle, _);
> 
> ASSERT_TRUE(test_stream_->Open(kTestPacketSize));
> 
> // This method will run all the immediate task posted and return. 
> message_loop_.RunAllPending();
> 
> EXPECT_EQ(...);
> 

As mentioned in the other comment, I'm not sure I like adding more test points
into the class.  It makes the class maintenance a bit harder.

http://codereview.chromium.org/160497/diff/36/42
File media/audio/linux/alsa_wrapper.h (right):

http://codereview.chromium.org/160497/diff/36/42#newcode13
Line 13: class AlsaWrapper {
On 2009/08/03 22:26:18, Alpha wrote:
> This is totally awsome, we should have FFmpegWrapper for testing FFmpeg..
Gives
> us access to real ffmpeg implementation.

Andrew and I talked about this once. I think there's a want to do this, but
currently we're working around it via weak symbols.

[Alpha Left Google, 2009-08-03 23:01:25]

LGTM

[Alpha Left Google, 2009-08-04 02:55:09]

Nothing big that I noticed, just style.

http://codereview.chromium.org/160497/diff/62/66
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/160497/diff/62/66#newcode194
Line 194: PcmClose(kFakeHandle))
this can go into the previous line.

[awong, 2009-08-04 03:43:18]

Okay, this time for sure.  I added as many unittests as I can do simply.  If
this still looks good in the review, I'll submit.

[awong, 2009-08-04 03:47:31]

http://codereview.chromium.org/160497/diff/62/66
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/160497/diff/62/66#newcode194
Line 194: PcmClose(kFakeHandle))
On 2009/08/04 02:55:09, Alpha wrote:
> this can go into the previous line.

Done.

[scherkus (not reviewing), 2009-08-04 14:31:33]

it looks good, I agree it's scary though

thinking aloud... would it be useful for you to have a data structure similar to
the SharedData where each setter/getter confirms the current message loop?

in release code the dchecks would go away, so I wouldn't assume a perf hit

http://codereview.chromium.org/160497/diff/124/1059
File media/audio/audio_output.h (right):

http://codereview.chromium.org/160497/diff/124/1059#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
should be 2008-2009

http://codereview.chromium.org/160497/diff/124/1060
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/160497/diff/124/1060#newcode16
Line 16: // |shared_data_.lock| is held.
lock -> lock_

how do you hold it?

http://codereview.chromium.org/160497/diff/124/1060#newcode22
Line 22: // structured so that all mutations to |shared_data_| is only done in
the
is -> are

http://codereview.chromium.org/160497/diff/124/1060#newcode45
Line 45: // The object has two distinct error states: |shared_data_.state| ==
kInError
state -> state_

http://codereview.chromium.org/160497/diff/124/1060#newcode46
Line 46: // and |stop_stream_|.  The |shared_data_.state| state is only settable
state -> state_

http://codereview.chromium.org/160497/diff/124/1060#newcode53
Line 53: // When |shared_data_.state| == kInError, all public API functions will
fail
state -> state_

http://codereview.chromium.org/160497/diff/124/1060#newcode64
Line 64: #include "media/audio/linux/alsa_output.h"
the one thing I'm wondering about is whether we ever need to hold on to
SharedData's lock + perform an action.  It looks like you read the state, then
assume that state for the rest of the action.

Do you think we could ever get caught by an action executing assuming kIsPlaying
while a second thread switches it to kIsError (or similar)

http://codereview.chromium.org/160497/diff/124/1060#newcode80
Line 80: #if NDEBUG
hmm I thought we preferred to use "defined"

i.e.,
#if defined(_DEBUG)

http://codereview.chromium.org/160497/diff/124/1060#newcode86
Line 86: const char* AlsaPcmOutputStream::kDefaultDevice = "default";
tiny nit: should use

const char Foo::kBar[] = "baz";

instead of *

http://codereview.chromium.org/160497/diff/124/1060#newcode229
Line 229: // CanTransitionTo was checked above, and it is assumed that this
add ()

http://codereview.chromium.org/160497/diff/124/1060#newcode231
Line 231: // transition out from under us.
ahh I see :)

http://codereview.chromium.org/160497/diff/124/1060#newcode261
Line 261: }
should free callback in the else clause

http://codereview.chromium.org/160497/diff/124/1060#newcode575
Line 575: state_ = kInError;
I'm semi-confused... what about that stop_stream_ flag?

http://codereview.chromium.org/160497/diff/124/1061
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/160497/diff/124/1061#newcode7
Line 7: // On device write failure, the stream will move itself an invalid
state.  No
an -> to an

http://codereview.chromium.org/160497/diff/124/1061#newcode193
Line 193: AudioSourceCallback* source_callback_;
scoped_ptr ?

http://codereview.chromium.org/160497/diff/124/1061#newcode199
Line 199: MessageLoop* client_thread_loop_;
const ?

http://codereview.chromium.org/160497/diff/124/1062
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/160497/diff/124/1062#newcode19
Line 19: class MockAlsaWrapper : public AlsaWrapper {
nice :)

[awong, 2009-08-04 21:12:57]

fixed andrew's comments.

http://codereview.chromium.org/160497/diff/124/1059
File media/audio/audio_output.h (right):

http://codereview.chromium.org/160497/diff/124/1059#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/04 14:31:33, scherkus wrote:
> should be 2008-2009

Done.

http://codereview.chromium.org/160497/diff/124/1060
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/160497/diff/124/1060#newcode16
Line 16: // |shared_data_.lock| is held.
On 2009/08/04 14:31:33, scherkus wrote:
> lock -> lock_
> 
> how do you hold it?
presumably by calling Lock?  Not sure I understand.

http://codereview.chromium.org/160497/diff/124/1060#newcode22
Line 22: // structured so that all mutations to |shared_data_| is only done in
the
On 2009/08/04 14:31:33, scherkus wrote:
> is -> are

Done.

http://codereview.chromium.org/160497/diff/124/1060#newcode45
Line 45: // The object has two distinct error states: |shared_data_.state| ==
kInError
On 2009/08/04 14:31:33, scherkus wrote:
> state -> state_

Done.

http://codereview.chromium.org/160497/diff/124/1060#newcode46
Line 46: // and |stop_stream_|.  The |shared_data_.state| state is only settable
On 2009/08/04 14:31:33, scherkus wrote:
> state -> state_

Done.

http://codereview.chromium.org/160497/diff/124/1060#newcode53
Line 53: // When |shared_data_.state| == kInError, all public API functions will
fail
On 2009/08/04 14:31:33, scherkus wrote:
> state -> state_

Done.

Gha...stupid variable rename. :(

http://codereview.chromium.org/160497/diff/124/1060#newcode64
Line 64: #include "media/audio/linux/alsa_output.h"
On 2009/08/04 14:31:33, scherkus wrote:
> the one thing I'm wondering about is whether we ever need to hold on to
> SharedData's lock + perform an action.  It looks like you read the state, then
> assume that state for the rest of the action.
> 
> Do you think we could ever get caught by an action executing assuming
kIsPlaying
> while a second thread switches it to kIsError (or similar)

Yeah, that's part of the threading scariness.  The code is written so that it
can function safely regardless of when the state change occurs.

This is safe because the state change is used as a signal.

I added more to the file comment about this.

http://codereview.chromium.org/160497/diff/124/1060#newcode80
Line 80: #if NDEBUG
On 2009/08/04 14:31:33, scherkus wrote:
> hmm I thought we preferred to use "defined"
> 
> i.e.,
> #if defined(_DEBUG)

You're right, thought _DEBUG is in the compiler specific identifier space.  I
went to

#if defined(NDEBUG)

http://codereview.chromium.org/160497/diff/124/1060#newcode86
Line 86: const char* AlsaPcmOutputStream::kDefaultDevice = "default";
On 2009/08/04 14:31:33, scherkus wrote:
> tiny nit: should use
> 
> const char Foo::kBar[] = "baz";
> 
> instead of *

Bleh.  Changed it, but I personally don't like it for class statics since the
size of the array can never be known.

http://codereview.chromium.org/160497/diff/124/1060#newcode229
Line 229: // CanTransitionTo was checked above, and it is assumed that this
On 2009/08/04 14:31:33, scherkus wrote:
> add ()

Done.

http://codereview.chromium.org/160497/diff/124/1060#newcode231
Line 231: // transition out from under us.
On 2009/08/04 14:31:33, scherkus wrote:
> ahh I see :)

Yeah...writing this code gave me a headache. :(

http://codereview.chromium.org/160497/diff/124/1060#newcode261
Line 261: }
On 2009/08/04 14:31:33, scherkus wrote:
> should free callback in the else clause

we don't own it.  This is a (fairly serious) bug in the IpcAudioSource API. 
There's a bug open to fix it.

http://codereview.chromium.org/160497/diff/124/1060#newcode575
Line 575: state_ = kInError;
On 2009/08/04 14:31:33, scherkus wrote:
> I'm semi-confused... what about that stop_stream_ flag?

stop_stream_ is only allowed to be changed inside the message_loop_.  Basically

  shared_data_.state_: only changed by public APIs
  stop_stream_: only changed/read by message loop.

That way the two objects don't need to really synchronize with one another.

This all would probably be cleaner if I actually created a specific "AlsaTasks"
class or similar that had all the state + logic to work in the message loop.  I
could try that in a different CL, but I'd rather not do that now.

http://codereview.chromium.org/160497/diff/124/1061
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/160497/diff/124/1061#newcode7
Line 7: // On device write failure, the stream will move itself an invalid
state.  No
On 2009/08/04 14:31:33, scherkus wrote:
> an -> to an

Done.

http://codereview.chromium.org/160497/diff/124/1061#newcode193
Line 193: AudioSourceCallback* source_callback_;
On 2009/08/04 14:31:33, scherkus wrote:
> scoped_ptr ?

Nope, not currently.  The output streams don't own the callback.  This causes a
race which needs a different fix.

http://codereview.chromium.org/160497/diff/124/1061#newcode199
Line 199: MessageLoop* client_thread_loop_;
On 2009/08/04 14:31:33, scherkus wrote:
> const ?

MessageLoop* const seems overkill.

http://codereview.chromium.org/160497/diff/124/1062
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/160497/diff/124/1062#newcode19
Line 19: class MockAlsaWrapper : public AlsaWrapper {
On 2009/08/04 14:31:33, scherkus wrote:
> nice :)

I like it. :)

[Alpha Left Google, 2009-08-04 21:17:17]

LGTM

[scherkus (not reviewing), 2009-08-05 04:05:52]

http://codereview.chromium.org/160497/diff/124/1060
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/160497/diff/124/1060#newcode16
Line 16: // |shared_data_.lock| is held.
On 2009/08/04 21:12:57, awong wrote:
> On 2009/08/04 14:31:33, scherkus wrote:
> > lock -> lock_
> > 
> > how do you hold it?
> presumably by calling Lock?  Not sure I understand.
> 

Sorry wasn't clear.  Was just a comment that lock_ is private, so how can we
hold the lock (answer is the public methods do it for you!)