Clean-up and bug fixes in AudioOutputController:

media/audio/audio_output_controller.cc

Index: media/audio/audio_output_controller.cc
diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index ffeb639b7007039fe57c4f497fac142480180a7d..e3464fd6fdf9acc5f11f56a6b5f20071279159fa 100644
--- a/media/audio/audio_output_controller.cc
+++ b/media/audio/audio_output_controller.cc
@@ -7,16 +7,13 @@
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "build/build_config.h"
 #include "media/audio/shared_memory_util.h"
 
 using base::Time;
 using base::TimeDelta;
-using base::WaitableEvent;
 
 namespace media {
 
@@ -35,28 +32,19 @@ AudioOutputController::AudioOutputController(AudioManager* audio_manager,
       diverting_to_stream_(NULL),
       volume_(1.0),
       state_(kEmpty),
+      num_allowed_io_(0),
       sync_reader_(sync_reader),
       message_loop_(audio_manager->GetMessageLoop()),
       number_polling_attempts_left_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)) {
+  DCHECK(audio_manager);
+  DCHECK(handler_);
+  DCHECK(sync_reader_);
+  DCHECK(message_loop_);
 }
 
 AudioOutputController::~AudioOutputController() {
   DCHECK_EQ(kClosed, state_);
-
-  if (message_loop_->BelongsToCurrentThread()) {
-    DoStopCloseAndClearStream(NULL);
-  } else {
-    // http://crbug.com/120973
-    base::ThreadRestrictions::ScopedAllowWait allow_wait;
-    WaitableEvent completion(true /* manual reset */,
-                             false /* initial state */);
-    message_loop_->PostTask(FROM_HERE,
-        base::Bind(&AudioOutputController::DoStopCloseAndClearStream,
-                   base::Unretained(this),
-                   &completion));
-    completion.Wait();
-  }
 }
 
 // static
@@ -74,50 +62,45 @@ scoped_refptr<AudioOutputController> AudioOutputController::Create(
   scoped_refptr<AudioOutputController> controller(new AudioOutputController(
       audio_manager, event_handler, params, sync_reader));
   controller->message_loop_->PostTask(FROM_HERE, base::Bind(
-      &AudioOutputController::DoCreate, controller));
+      &AudioOutputController::DoCreate, controller, false));
   return controller;
 }
 
 void AudioOutputController::Play() {
-  DCHECK(message_loop_);
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioOutputController::DoPlay, this));
 }
 
 void AudioOutputController::Pause() {
-  DCHECK(message_loop_);
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioOutputController::DoPause, this));
 }
 
 void AudioOutputController::Flush() {
-  DCHECK(message_loop_);
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioOutputController::DoFlush, this));
 }
 
 void AudioOutputController::Close(const base::Closure& closed_task) {
   DCHECK(!closed_task.is_null());
-  DCHECK(message_loop_);
   message_loop_->PostTaskAndReply(FROM_HERE, base::Bind(
       &AudioOutputController::DoClose, this), closed_task);
 }
 
 void AudioOutputController::SetVolume(double volume) {
-  DCHECK(message_loop_);
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioOutputController::DoSetVolume, this, volume));
 }
 
-void AudioOutputController::DoCreate() {
+void AudioOutputController::DoCreate(bool is_for_device_change) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   // Close() can be called before DoCreate() is executed.
   if (state_ == kClosed)
     return;
-  DCHECK(state_ == kEmpty || state_ == kRecreating) << state_;
 
-  DoStopCloseAndClearStream(NULL);  // Calls RemoveOutputDeviceChangeListener().
+  DoStopCloseAndClearStream();  // Calls RemoveOutputDeviceChangeListener().
+  DCHECK_EQ(kEmpty, state_);
 
   stream_ = diverting_to_stream_ ? diverting_to_stream_ :
       audio_manager_->MakeAudioOutputStreamProxy(params_);
@@ -130,8 +113,8 @@ void AudioOutputController::DoCreate() {
   }
 
   if (!stream_->Open()) {
+    DoStopCloseAndClearStream();
     state_ = kError;
-    DoStopCloseAndClearStream(NULL);
 
     // TODO(hclam): Define error types.
     handler_->OnError(this, 0);
@@ -147,11 +130,10 @@ void AudioOutputController::DoCreate() {
   stream_->SetVolume(volume_);
 
   // Finally set the state to kCreated.
-  State original_state = state_;
   state_ = kCreated;
 
   // And then report we have been created if we haven't done so already.
-  if (original_state != kRecreating)
+  if (!is_for_device_change)
     handler_->OnCreated(this);
 }
 
@@ -159,13 +141,8 @@ void AudioOutputController::DoPlay() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   // We can start from created or paused state.
-  if (state_ != kCreated && state_ != kPaused) {
-    // If a pause is pending drop it.  Otherwise the controller might hang since
-    // the corresponding play event has already occurred.
-    if (state_ == kPausedWhenStarting)
-      state_ = kStarting;
+  if (state_ != kCreated && state_ != kPaused)
     return;
-  }
 
   state_ = kStarting;
 
@@ -188,20 +165,12 @@ void AudioOutputController::DoPlay() {
 void AudioOutputController::PollAndStartIfDataReady() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  // Being paranoid: do nothing if state unexpectedly changed.
-  if ((state_ != kStarting) && (state_ != kPausedWhenStarting))
-    return;
+  DCHECK_EQ(kStarting, state_);
 
-  bool pausing = (state_ == kPausedWhenStarting);
   // If we are ready to start the stream, start it.
-  // Of course we may have to stop it immediately...
   if (--number_polling_attempts_left_ == 0 ||
-      pausing ||
       sync_reader_->DataReady()) {
     StartStream();
-    if (pausing) {
-      DoPause();
-    }
   } else {
     message_loop_->PostDelayedTask(
         FROM_HERE,
@@ -216,41 +185,39 @@ void AudioOutputController::StartStream() {
   state_ = kPlaying;
 
   // We start the AudioOutputStream lazily.
+  AllowEntryToOnMoreIOData();
   stream_->Start(this);
 
   // Tell the event handler that we are now playing.
   handler_->OnPlaying(this);
 }
 
-void AudioOutputController::DoPause() {
+void AudioOutputController::StopStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  if (stream_) {
-    // Then we stop the audio device. This is not the perfect solution
-    // because it discards all the internal buffer in the audio device.
-    // TODO(hclam): Actually pause the audio device.
+  if (state_ == kStarting) {
+    // Cancel in-progress polling start.
+    weak_this_.InvalidateWeakPtrs();
+    state_ = kPaused;
+  } else if (state_ == kPlaying) {
     stream_->Stop();
+    DisallowEntryToOnMoreIOData();
+    state_ = kPaused;
   }
+}
 
-  switch (state_) {
-    case kStarting:
-      // We were asked to pause while starting. There is delayed task that will
-      // try starting playback, and there is no way to remove that task from the
-      // queue. If we stop now that task will be executed anyway.
-      // Delay pausing, let delayed task to do pause after it start playback.
-      state_ = kPausedWhenStarting;
-      break;
-    case kPlaying:
-      state_ = kPaused;
+void AudioOutputController::DoPause() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
 
-      // Send a special pause mark to the low-latency audio thread.
-      sync_reader_->UpdatePendingBytes(kPauseMark);
+  StopStream();
 
-      handler_->OnPaused(this);
-      break;
-    default:
-      return;
-  }
+  if (state_ != kPaused)
+    return;
+
+  // Send a special pause mark to the low-latency audio thread.
+  sync_reader_->UpdatePendingBytes(kPauseMark);
+
+  handler_->OnPaused(this);
 }
 
 void AudioOutputController::DoFlush() {
@@ -263,7 +230,7 @@ void AudioOutputController::DoClose() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (state_ != kClosed) {
-    DoStopCloseAndClearStream(NULL);
+    DoStopCloseAndClearStream();
     sync_reader_->Close();
     state_ = kClosed;
   }
@@ -279,7 +246,6 @@ void AudioOutputController::DoSetVolume(double volume) {
   switch (state_) {
     case kCreated:
     case kStarting:
-    case kPausedWhenStarting:
     case kPlaying:
     case kPaused:
       stream_->SetVolume(volume_);
@@ -303,20 +269,15 @@ int AudioOutputController::OnMoreData(AudioBus* dest,
 int AudioOutputController::OnMoreIOData(AudioBus* source,
                                         AudioBus* dest,
                                         AudioBuffersState buffers_state) {
+  DisallowEntryToOnMoreIOData();
   TRACE_EVENT0("audio", "AudioOutputController::OnMoreIOData");
 
-  {
-    // Check state and do nothing if we are not playing.
-    // We are on the hardware audio thread, so lock is needed.
-    base::AutoLock auto_lock(lock_);
-    if (state_ != kPlaying) {
-      return 0;
-    }
-  }
-
-  int frames = sync_reader_->Read(source, dest);
+  const int frames = sync_reader_->Read(source, dest);
+  DCHECK_LE(0, frames);
   sync_reader_->UpdatePendingBytes(
       buffers_state.total_bytes() + frames * params_.GetBytesPerFrame());
+
+  AllowEntryToOnMoreIOData();
   return frames;
 }
 
@@ -344,7 +305,7 @@ void AudioOutputController::OnError(AudioOutputStream* stream, int code) {
       &AudioOutputController::DoReportError, this, code));
 }
 
-void AudioOutputController::DoStopCloseAndClearStream(WaitableEvent* done) {
+void AudioOutputController::DoStopCloseAndClearStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   // Allow calling unconditionally and bail if we don't have a stream_ to close.
@@ -354,18 +315,14 @@ void AudioOutputController::DoStopCloseAndClearStream(WaitableEvent* done) {
     if (stream_ != diverting_to_stream_)
       audio_manager_->RemoveOutputDeviceChangeListener(this);
 
-    stream_->Stop();
+    StopStream();
     stream_->Close();
     if (stream_ == diverting_to_stream_)
       diverting_to_stream_ = NULL;
     stream_ = NULL;
-
-    weak_this_.InvalidateWeakPtrs();
   }
 
-  // Should be last in the method, do not touch "this" from here on.
-  if (done)
-    done->Signal();
+  state_ = kEmpty;
 }
 
 void AudioOutputController::OnDeviceChange() {
@@ -374,8 +331,7 @@ void AudioOutputController::OnDeviceChange() {
   // Recreate the stream (DoCreate() will first shut down an existing stream).
   // Exit if we ran into an error.
   const State original_state = state_;
-  state_ = kRecreating;
-  DoCreate();
+  DoCreate(true);
   if (!stream_ || state_ == kError)
     return;
 
@@ -386,9 +342,8 @@ void AudioOutputController::OnDeviceChange() {
       DoPlay();
       return;
     case kCreated:
-    case kPausedWhenStarting:
     case kPaused:
-      // From the outside these three states are equivalent.
+      // From the outside these two states are equivalent.
       return;
     default:
       NOTREACHED() << "Invalid original state.";
@@ -437,4 +392,14 @@ void AudioOutputController::DoStopDiverting() {
   DCHECK(!diverting_to_stream_);
 }
 
+void AudioOutputController::AllowEntryToOnMoreIOData() {
+  DCHECK(base::AtomicRefCountIsZero(&num_allowed_io_));
+  base::AtomicRefCountInc(&num_allowed_io_);
+}
+
+void AudioOutputController::DisallowEntryToOnMoreIOData() {
+  const bool is_zero = !base::AtomicRefCountDec(&num_allowed_io_);
+  DCHECK(is_zero);
+}
+
 }  // namespace media