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

media/audio/linux/alsa_output.cc

Index: media/audio/linux/alsa_output.cc
===================================================================
--- media/audio/linux/alsa_output.cc	(revision 87982)
+++ media/audio/linux/alsa_output.cc	(working copy)
@@ -4,43 +4,14 @@
 //
 // THREAD SAFETY
 //
-// The AlsaPcmOutputStream object's internal state is accessed by two threads:
-//
+// AlsaPcmOutputStream object is *not* thread-safe -- we assume that
 //   client thread - creates the object and calls the public APIs.
 //   message loop thread - executes all the internal tasks including querying
 //      the data source for more data, writing to the alsa device, and closing
-//      the alsa device.  It does *not* handle opening the device though.
+//      the alsa device.
+// is actually the same thread.
 //
-// The class is designed so that most operations that read/modify the object's
-// internal state are done on the message loop thread.  The exception is data
-// conatined in the |shared_data_| structure.  Data in this structure needs to
-// be accessed by both threads, and should only be accessed when the
-// |shared_data_.lock_| is held.
 //
-// All member variables that are not in |shared_data_| are created/destroyed on
-// the |message_loop_|.  This allows safe access to them from any task posted to
-// |message_loop_|.  The values in |shared_data_| are considered to be read-only
-// signals by tasks posted to |message_loop_| (see the SEMANTICS of
-// |shared_data_| section below).  Because of these two constraints, tasks can,
-// and must, be coded to be safe in the face of a changing |shared_data_|.
-//
-//
-// SEMANTICS OF |shared_data_|
-//
-// Though |shared_data_| is accessable by both threads, the code has been
-// structured so that all mutations to |shared_data_| are only done in the
-// client thread.  The message loop thread only ever reads the shared data.
-//
-// This reduces the need for critical sections because the public API code can
-// assume that no mutations occur to the |shared_data_| between queries.
-//
-// On the message loop side, tasks have been coded such that they can
-// operate safely regardless of when state changes happen to |shared_data_|.
-// Code that is sensitive to the timing of state changes are delegated to the
-// |shared_data_| object so they can executed while holding
-// |shared_data_.lock_|.
-//
-//
 // SEMANTICS OF CloseTask()
 //
 // The CloseTask() is responsible for cleaning up any resources that were
@@ -49,20 +20,17 @@
 // they should not attempt to access any of the internal structures beyond
 // querying the |stop_stream_| flag and no-oping themselves.  This will
 // guarantee that eventually no more tasks will be posted into the message
-// loop, and the AlsaPcmOutputStream will be able to delete itself.
+// loop.
 //
 //
 // SEMANTICS OF ERROR STATES
 //
-// The object has two distinct error states: |shared_data_.state_| == kInError
-// and |stop_stream_|.  The |shared_data_.state_| state is only settable
-// by the client thread, and thus cannot be used to signal when the ALSA device
-// fails due to a hardware (or other low-level) event.  The |stop_stream_|
-// variable is only accessed by the message loop thread; it is used to indicate
+// The object has two distinct error states: |state_| == kInError
+// and |stop_stream_|.  The |stop_stream_| variable is used to indicate
 // that the playback_handle should no longer be used either because of a
 // hardware/low-level event, or because the CloseTask() has been run.
 //
-// When |shared_data_.state_| == kInError, all public API functions will fail
+// When |state_| == kInError, all public API functions will fail
 // with an error (Start() will call the OnError() function on the callback
 // immediately), or no-op themselves with the exception of Close().  Even if an
 // error state has been entered, if Open() has previously returned successfully,
@@ -218,8 +186,7 @@
                                          AlsaWrapper* wrapper,
                                          AudioManagerLinux* manager,
                                          MessageLoop* message_loop)
-    : shared_data_(MessageLoop::current()),
-      requested_device_name_(device_name),
+    : requested_device_name_(device_name),
       pcm_format_(alsa_util::BitsToFormat(params.bits_per_sample)),
       channels_(params.channels),
       sample_rate_(params.sample_rate),
@@ -238,30 +205,36 @@
       manager_(manager),
       playback_handle_(NULL),
       frames_per_packet_(packet_size_ / bytes_per_frame_),
-      client_thread_loop_(MessageLoop::current()),
-      message_loop_(message_loop) {
+      message_loop_(message_loop),
+      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
+      state_(kCreated),
+      volume_(1.0f),
+      source_callback_(NULL) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
   // Sanity check input values.
   if ((params.sample_rate > kAlsaMaxSampleRate) || (params.sample_rate <= 0)) {
     LOG(WARNING) << "Unsupported audio frequency.";
-    shared_data_.TransitionTo(kInError);
+    TransitionTo(kInError);
   }
 
   if (AudioParameters::AUDIO_PCM_LINEAR != params.format &&
       AudioParameters::AUDIO_PCM_LOW_LATENCY != params.format) {
     LOG(WARNING) << "Unsupported audio format";
-    shared_data_.TransitionTo(kInError);
+    TransitionTo(kInError);
   }
 
   if (pcm_format_ == SND_PCM_FORMAT_UNKNOWN) {
     LOG(WARNING) << "Unsupported bits per sample: " << params.bits_per_sample;
-    shared_data_.TransitionTo(kInError);
+    TransitionTo(kInError);
   }
 }
 
 AlsaPcmOutputStream::~AlsaPcmOutputStream() {
-  InternalState state = shared_data_.state();
-  DCHECK(state == kCreated || state == kIsClosed || state == kInError);
+  InternalState current_state = state();
+  DCHECK(current_state == kCreated ||
+         current_state == kIsClosed ||
+         current_state == kInError);
 
   // TODO(ajwong): Ensure that CloseTask has been called and the
   // playback handle released by DCHECKing that playback_handle_ is NULL.
@@ -270,14 +243,14 @@
 }
 
 bool AlsaPcmOutputStream::Open() {
-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  if (shared_data_.state() == kInError) {
+  if (state() == kInError) {
     return false;
   }
 
-  if (!shared_data_.CanTransitionTo(kIsOpened)) {
-    NOTREACHED() << "Invalid state: " << shared_data_.state();
+  if (!CanTransitionTo(kIsOpened)) {
+    NOTREACHED() << "Invalid state: " << state();
     return false;
   }
 
@@ -285,26 +258,26 @@
   // CanTransitionTo() was checked above, and it is assumed that this
   // object's public API is only called on one thread so the state cannot
   // transition out from under us.
-  shared_data_.TransitionTo(kIsOpened);
+  TransitionTo(kIsOpened);
   message_loop_->PostTask(
       FROM_HERE,
-      NewRunnableMethod(this, &AlsaPcmOutputStream::OpenTask));
+      method_factory_.NewRunnableMethod(&AlsaPcmOutputStream::OpenTask));
 
   return true;
 }
 
 void AlsaPcmOutputStream::Close() {
-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
   // Sanity check that the transition occurs correctly.  It is safe to
   // continue anyways because all operations for closing are idempotent.
-  if (shared_data_.TransitionTo(kIsClosed) != kIsClosed) {
+  if (TransitionTo(kIsClosed) != kIsClosed) {
     NOTREACHED() << "Unable to transition Closed.";
   }
 
   message_loop_->PostTask(
       FROM_HERE,
-      NewRunnableMethod(this, &AlsaPcmOutputStream::CloseTask));
+      method_factory_.NewRunnableMethod(&AlsaPcmOutputStream::CloseTask));
 
   // Signal to the manager that we're closed and can be removed.  Since
   // we just posted a CloseTask to the message loop, we won't be deleted
@@ -313,39 +286,39 @@
 }
 
 void AlsaPcmOutputStream::Start(AudioSourceCallback* callback) {
-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
   CHECK(callback);
 
-  shared_data_.set_source_callback(callback);
+  set_source_callback(callback);
 
   // Only post the task if we can enter the playing state.
-  if (shared_data_.TransitionTo(kIsPlaying) == kIsPlaying) {
+  if (TransitionTo(kIsPlaying) == kIsPlaying) {
     message_loop_->PostTask(
         FROM_HERE,
-        NewRunnableMethod(this, &AlsaPcmOutputStream::StartTask));
+        method_factory_.NewRunnableMethod(&AlsaPcmOutputStream::StartTask));
   }
 }
 
 void AlsaPcmOutputStream::Stop() {
-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
   // Reset the callback, so that it is not called anymore.
-  shared_data_.set_source_callback(NULL);
+  set_source_callback(NULL);
 
-  shared_data_.TransitionTo(kIsStopped);
+  TransitionTo(kIsStopped);
 }
 
 void AlsaPcmOutputStream::SetVolume(double volume) {
-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  shared_data_.set_volume(static_cast<float>(volume));
+  set_volume(static_cast<float>(volume));
 }
 
-void AlsaPcmOutputStream::GetVolume(double* volume) {
-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+void AlsaPcmOutputStream::GetVolume(double* pointer_to_volume) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  *volume = shared_data_.volume();
+  *pointer_to_volume = volume();
 }
 
 void AlsaPcmOutputStream::OpenTask() {
@@ -396,7 +369,7 @@
     return;
   }
 
-  if (shared_data_.state() != kIsPlaying) {
+  if (state() != kIsPlaying) {
     return;
   }
 
@@ -471,9 +444,8 @@
     scoped_refptr<media::DataBuffer> packet =
         new media::DataBuffer(packet_size_);
     size_t packet_size =
-        shared_data_.OnMoreData(
-            this, packet->GetWritableData(), packet->GetBufferSize(),
-            AudioBuffersState(buffer_delay, hardware_delay));
+        OnMoreData(this, packet->GetWritableData(), packet->GetBufferSize(),
+                   AudioBuffersState(buffer_delay, hardware_delay));
     CHECK(packet_size <= packet->GetBufferSize()) <<
         "Data source overran buffer.";
 
@@ -489,7 +461,7 @@
                               packet_size,
                               channels_,
                               bytes_per_sample_,
-                              shared_data_.volume())) {
+                              volume())) {
         // Adjust packet size for downmix.
         packet_size =
             packet_size / bytes_per_frame_ * bytes_per_output_frame_;
@@ -525,7 +497,7 @@
                           packet_size,
                           channels_,
                           bytes_per_sample_,
-                          shared_data_.volume());
+                          volume());
     }
 
     if (packet_size > 0) {
@@ -547,7 +519,7 @@
     return;
   }
 
-  if (shared_data_.state() == kIsStopped) {
+  if (state() == kIsStopped) {
     return;
   }
 
@@ -579,7 +551,7 @@
       if (frames_written != -EAGAIN) {
         LOG(ERROR) << "Failed to write to pcm device: "
                    << wrapper_->StrError(frames_written);
-        shared_data_.OnError(this, frames_written);
+        OnError(this, frames_written);
         stop_stream_ = true;
       }
     } else {
@@ -610,7 +582,7 @@
     return;
   }
 
-  if (shared_data_.state() == kIsStopped) {
+  if (state() == kIsStopped) {
     return;
   }
 
@@ -655,17 +627,17 @@
   }
 
   // Only schedule more reads/writes if we are still in the playing state.
-  if (shared_data_.state() == kIsPlaying) {
+  if (state() == kIsPlaying) {
     if (next_fill_time_ms == 0) {
       message_loop_->PostTask(
           FROM_HERE,
-          NewRunnableMethod(this, &AlsaPcmOutputStream::WriteTask));
+          method_factory_.NewRunnableMethod(&AlsaPcmOutputStream::WriteTask));
     } else {
       // TODO(ajwong): Measure the reliability of the delay interval.  Use
       // base/metrics/histogram.h.
       message_loop_->PostDelayedTask(
           FROM_HERE,
-          NewRunnableMethod(this, &AlsaPcmOutputStream::WriteTask),
+          method_factory_.NewRunnableMethod(&AlsaPcmOutputStream::WriteTask),
           next_fill_time_ms);
     }
   }
@@ -843,27 +815,11 @@
 }
 
 AudioManagerLinux* AlsaPcmOutputStream::manager() {

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

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)

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

Done.

-  DCHECK_EQ(MessageLoop::current(), client_thread_loop_);
+  CHECK_EQ(MessageLoop::current(), message_loop_);
   return manager_;
 }
 
-AlsaPcmOutputStream::SharedData::SharedData(
-    MessageLoop* state_transition_loop)
-    : state_(kCreated),
-      volume_(1.0f),
-      source_callback_(NULL),
-      state_transition_loop_(state_transition_loop) {
-}
-
-bool AlsaPcmOutputStream::SharedData::CanTransitionTo(InternalState to) {
-  base::AutoLock l(lock_);
-  return CanTransitionTo_Locked(to);
-}
-
-bool AlsaPcmOutputStream::SharedData::CanTransitionTo_Locked(
-    InternalState to) {
-  lock_.AssertAcquired();
-
+bool AlsaPcmOutputStream::CanTransitionTo(InternalState to) {
   switch (state_) {
     case kCreated:
       return to == kIsOpened || to == kIsClosed || to == kInError;
@@ -890,11 +846,10 @@
 }
 
 AlsaPcmOutputStream::InternalState
-AlsaPcmOutputStream::SharedData::TransitionTo(InternalState to) {
-  DCHECK_EQ(MessageLoop::current(), state_transition_loop_);
+AlsaPcmOutputStream::TransitionTo(InternalState to) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  base::AutoLock l(lock_);
-  if (!CanTransitionTo_Locked(to)) {
+  if (!CanTransitionTo(to)) {
     NOTREACHED() << "Cannot transition from: " << state_ << " to: " << to;
     state_ = kInError;
   } else {
@@ -903,25 +858,22 @@
   return state_;
 }
 
-AlsaPcmOutputStream::InternalState AlsaPcmOutputStream::SharedData::state() {
-  base::AutoLock l(lock_);
+AlsaPcmOutputStream::InternalState AlsaPcmOutputStream::state() {

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

I'd drop this method as it's private

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

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

   return state_;
 }
 
-float AlsaPcmOutputStream::SharedData::volume() {
-  base::AutoLock l(lock_);
+float AlsaPcmOutputStream::volume() {

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

I'd drop this method as it's private

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

Done.

   return volume_;
 }
 
-void AlsaPcmOutputStream::SharedData::set_volume(float v) {
-  base::AutoLock l(lock_);
+void AlsaPcmOutputStream::set_volume(float v) {

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

I'd drop this method as it's private

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

Done.

   volume_ = v;
 }
 
-uint32 AlsaPcmOutputStream::SharedData::OnMoreData(
-    AudioOutputStream* stream, uint8* dest, uint32 max_size,
-    AudioBuffersState buffers_state) {
-  base::AutoLock l(lock_);
+uint32 AlsaPcmOutputStream::OnMoreData(AudioOutputStream* stream,

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

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()

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

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

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

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

want to make the change?

+                                       uint8* dest,
+                                       uint32 max_size,
+                                       AudioBuffersState buffers_state) {
   if (source_callback_) {
     return source_callback_->OnMoreData(stream, dest, max_size, buffers_state);
   }
@@ -929,9 +881,7 @@
   return 0;
 }
 
-void AlsaPcmOutputStream::SharedData::OnError(AudioOutputStream* stream,
-                                              int code) {
-  base::AutoLock l(lock_);
+void AlsaPcmOutputStream::OnError(AudioOutputStream* stream, int code) {

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

ditto, maybe RunErrorCallback()

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

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

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

Ditto

   if (source_callback_) {
     source_callback_->OnError(stream, code);
   }
@@ -939,9 +889,7 @@
 
 // Changes the AudioSourceCallback to proxy calls to.  Pass in NULL to
 // release ownership of the currently registered callback.
-void AlsaPcmOutputStream::SharedData::set_source_callback(
-    AudioSourceCallback* callback) {
-  DCHECK_EQ(MessageLoop::current(), state_transition_loop_);
-  base::AutoLock l(lock_);
+void AlsaPcmOutputStream::set_source_callback(AudioSourceCallback* callback) {

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

I'd drop this method as it's private

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

Called in the unit tests.

+  DCHECK_EQ(MessageLoop::current(), message_loop_);
   source_callback_ = callback;
 }