Simplify AlsaPcmOutputStream and AudioManagerLinux. Code was thread-safe, but now

Simplify AlsaPcmOutputStream and AudioManagerLinux. Code was thread-safe, but now
client and audio threads are the same, so we don't need locks/ref counts/etc.

http://codereview.chromium.org/7117001

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

[scherkus (not reviewing), 2011-06-07 00:19:00]

looking pretty good!!  few nits and a comment about deletion+CloseTask

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.cc
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:285: manager()->ReleaseOutputStream(this);
hmm.. if I follow this correctly:
  1) We're executing on message_loop_
  2) We post CloseTask()
  3) ReleaseOutputStream() will delete this object
  4) method_factory_ will cancel CloseTask() task

...meaning CloseTask() is never executed.

Unfortunately media_unittests fail to catch this because gmock tests for the
expectation of ReleaseOutputStream -- but doesn't actually delete the object :)

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:817: AudioManagerLinux*
AlsaPcmOutputStream::manager() {
inline this method as it no longer needs to be split

(a lot of these methods were added for the sake of checking we're on the same
thread)

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:861: AlsaPcmOutputStream::InternalState
AlsaPcmOutputStream::state() {
I'd drop this method as it's private

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:865: float AlsaPcmOutputStream::volume() {
I'd drop this method as it's private

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:869: void AlsaPcmOutputStream::set_volume(float
v) {
I'd drop this method as it's private

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:873: uint32
AlsaPcmOutputStream::OnMoreData(AudioOutputStream* stream,
it looks like we pass "this" for the stream parameter.. perhaps we can do
without

we should also have a clearer method name such as RunDataCallback()

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:884: void
AlsaPcmOutputStream::OnError(AudioOutputStream* stream, int code) {
ditto, maybe RunErrorCallback()

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:892: void
AlsaPcmOutputStream::set_source_callback(AudioSourceCallback* callback) {
I'd drop this method as it's private

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.h
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.h...
media/audio/linux/alsa_output.h:203: // This should only be used on the
|client_thread_loop_|.  Access via
comment is out of date

[enal, 2011-06-07 17:29:00]

Addressed all issues.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.cc
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:285: manager()->ReleaseOutputStream(this);
On 2011/06/07 00:19:01, scherkus wrote:
> hmm.. if I follow this correctly:
>   1) We're executing on message_loop_
>   2) We post CloseTask()
>   3) ReleaseOutputStream() will delete this object
>   4) method_factory_ will cancel CloseTask() task
> 
> ...meaning CloseTask() is never executed.
> 
> Unfortunately media_unittests fail to catch this because gmock tests for the
> expectation of ReleaseOutputStream -- but doesn't actually delete the object
:)

Fixed. Don't see how to fully test it, though...

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:817: AudioManagerLinux*
AlsaPcmOutputStream::manager() {
On 2011/06/07 00:19:01, scherkus wrote:
> inline this method as it no longer needs to be split
> 
> (a lot of these methods were added for the sake of checking we're on the same
> thread)

Done.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:861: AlsaPcmOutputStream::InternalState
AlsaPcmOutputStream::state() {
On 2011/06/07 00:19:01, scherkus wrote:
> I'd drop this method as it's private

It is used in the unit tests, I'd prefer tests to use accessor.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:865: float AlsaPcmOutputStream::volume() {
On 2011/06/07 00:19:01, scherkus wrote:
> I'd drop this method as it's private

Done.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:869: void AlsaPcmOutputStream::set_volume(float
v) {
On 2011/06/07 00:19:01, scherkus wrote:
> I'd drop this method as it's private

Done.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:873: uint32
AlsaPcmOutputStream::OnMoreData(AudioOutputStream* stream,
On 2011/06/07 00:19:01, scherkus wrote:
> it looks like we pass "this" for the stream parameter.. perhaps we can do
> without
> 
> we should also have a clearer method name such as RunDataCallback()

Name and arguments are defined in the parent class AudioOutputStream. We are
implementing abstract virtual function.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:884: void
AlsaPcmOutputStream::OnError(AudioOutputStream* stream, int code) {
On 2011/06/07 00:19:01, scherkus wrote:
> ditto, maybe RunErrorCallback()

Name and arguments are defined in the parent class AudioOutputStream. We are
implementing abstract virtual function.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:892: void
AlsaPcmOutputStream::set_source_callback(AudioSourceCallback* callback) {
On 2011/06/07 00:19:01, scherkus wrote:
> I'd drop this method as it's private

Called in the unit tests.

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.h
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.h...
media/audio/linux/alsa_output.h:203: // This should only be used on the
|client_thread_loop_|.  Access via
On 2011/06/07 00:19:01, scherkus wrote:
> comment is out of date

Done.

[enal, 2011-06-07 17:43:16]

Re-sending, this time uploaded version...

[scherkus (not reviewing), 2011-06-07 23:01:59]

LGTM w/ 2 nits re: callback executing functions

the abstract virtual functions are defined in AudioSourceCallback, but not
AudioOutputStream

we should be able to make the change to improve readability inside
AlsaPcmOutputStream

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.cc
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:873: uint32
AlsaPcmOutputStream::OnMoreData(AudioOutputStream* stream,
On 2011/06/07 17:29:00, enal wrote:
> On 2011/06/07 00:19:01, scherkus wrote:
> > it looks like we pass "this" for the stream parameter.. perhaps we can do
> > without
> > 
> > we should also have a clearer method name such as RunDataCallback()
> 
> Name and arguments are defined in the parent class AudioOutputStream. We are
> implementing abstract virtual function.


the abstract virtual function is actually from AudioSourceCallback (the
source_callback_ object), this function is a private helper

want to make the change?

http://codereview.chromium.org/7117001/diff/1/media/audio/linux/alsa_output.c...
media/audio/linux/alsa_output.cc:884: void
AlsaPcmOutputStream::OnError(AudioOutputStream* stream, int code) {
On 2011/06/07 17:29:00, enal wrote:
> On 2011/06/07 00:19:01, scherkus wrote:
> > ditto, maybe RunErrorCallback()
> 
> Name and arguments are defined in the parent class AudioOutputStream. We are
> implementing abstract virtual function.

Ditto

[enal, 2011-06-08 15:57:05]

Addressed everything.

[commit-bot: I haz the power, 2011-06-08 18:56:56]

Change committed as 88369