Harden audio output controller making it safe to call Pause() before

media/audio/audio_output_controller.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_output_controller.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
 
@@ skipping 158 matching lines @@
     SubmitOnMoreData_Locked();
   }
 }
 
 void AudioOutputController::DoPlay() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   // We can start from created or paused state.
   if (state_ != kCreated && state_ != kPaused)
     return;
-  state_ = kPlaying;
 
   if (LowLatencyMode()) {
+    state_ = kStarting;
+
     // Ask for first packet.
     sync_reader_->UpdatePendingBytes(0);
 
     // Cannot start stream immediately, should give renderer some time
     // to deliver data.
     number_polling_attempts_left_ = kPollNumAttempts;
     message_loop_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&AudioOutputController::PollAndStartIfDataReady, this),
         kPollPauseInMilliseconds);
   } else {
     StartStream();
   }
 }
 
 void AudioOutputController::PollAndStartIfDataReady() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   // Being paranoic: do nothing if we were stopped/paused
   // after DoPlay() but before DoStartStream().
-  if (state_ != kPlaying)
+  if (state_ != kStarting)
     return;
 
   if (--number_polling_attempts_left_ == 0 || sync_reader_->DataReady()) {
     StartStream();
   } else {
     message_loop_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&AudioOutputController::PollAndStartIfDataReady, this),
         kPollPauseInMilliseconds);
   }
 }
 
 void AudioOutputController::StartStream() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
+  state_ = kPlaying;
 
   // We start the AudioOutputStream lazily.
   stream_->Start(this);
 
   // Tell the event handler that we are now playing.
   handler_->OnPlaying(this);
 }
 
 void AudioOutputController::DoPause() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
-  // We can pause from started state.
-  if (state_ != kPlaying)
-    return;
-  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.
+      // We can set state to "paused" and avoid problem in the simple case,
+      // but we can be asked to start playback for 2nd time before it started
+      // for the 1st time, and there would be new set of problems.
+      // Simplest solution is to delay pause a little till playback starts.
+      message_loop_->PostDelayedTask(

[acolwell GONE FROM CHROMIUM, 2011/10/24 03:47:15]

This doesn't smell right. Can we add a kPauseWhileStarting state to track this?
When PollAndStartIfDataReady() is called and sees this state, it could change
state to kPlaying and then call DoPause(). If we get another DoPlay() then it
will see the kPauseWhileStarting and just transition to kStarting. Would this
work or am I missing something?

[enal1, 2011/10/24 16:28:29]

Initially I wrote code similar to what you suggest, looked at it, and decided to
go to something extra simple. Code was non-trivial and error prone -- even if
correct right now it can become obsolete later, and it always would be hard to
fully test it, as it is not on normal path...

I deliberately reusing kPollPauseInMilliseconds, not introducing new constant
kPauseWhileStarting. We want this task to work after PollAndStartIfDataReady(),
and using the same delay guarantees it. Added comment.

[acolwell GONE FROM CHROMIUM, 2011/10/24 16:59:58]

I'm afraid I'm going to have to insist here. I REALLY don't like using time to
hold state. It is error prone and can introduce subtle bugs. Why would this be
hard to test? It seems that forcing state transitions would be easier to test
than verifying time based interactions.

+          FROM_HERE,
+          base::Bind( &AudioOutputController::DoPause, this),
+          kPollPauseInMilliseconds);
+      break;
+    case kPlaying:
+      state_ = kPaused;
 
-  // 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.
-  stream_->Stop();
+      // 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.
+      stream_->Stop();
 
-  if (LowLatencyMode()) {
-    // Send a special pause mark to the low-latency audio thread.
-    sync_reader_->UpdatePendingBytes(kPauseMark);
+      if (LowLatencyMode()) {
+        // Send a special pause mark to the low-latency audio thread.
+        sync_reader_->UpdatePendingBytes(kPauseMark);
+      }
+
+      handler_->OnPaused(this);
+      break;
+    default:
+      return;
   }
-
-  handler_->OnPaused(this);
 }
 
 void AudioOutputController::DoFlush() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   // TODO(hclam): Actually flush the audio device.
 
   // If we are in the regular latency mode then flush the push source.
   if (!sync_reader_) {
     if (state_ != kPaused)
@@ skipping 26 matching lines @@
   closed_task.Run();
 }
 
 void AudioOutputController::DoSetVolume(double volume) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   // Saves the volume to a member first. We may not be able to set the volume
   // right away but when the stream is created we'll set the volume.
   volume_ = volume;
 
-  if (state_ != kPlaying && state_ != kPaused && state_ != kCreated)
-    return;
-
-  stream_->SetVolume(volume_);
+  switch (state_) {
+    case kCreated:
+    case kStarting:
+    case kPlaying:
+    case kPaused:
+      stream_->SetVolume(volume_);
+      break;
+    default:
+      return;
+  }
 }
 
 void AudioOutputController::DoReportError(int code) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
   if (state_ != kClosed)
     handler_->OnError(this, code);
 }
 
 uint32 AudioOutputController::OnMoreData(
     AudioOutputStream* stream, uint8* dest,
@@ skipping 52 matching lines @@
   buffers_state.pending_bytes += buffer_.forward_bytes();
 
   // If we need more data then call the event handler to ask for more data.
   // It is okay that we don't lock in this block because the parameters are
   // correct and in the worst case we are just asking more data than needed.
   base::AutoUnlock auto_unlock(lock_);
   handler_->OnMoreData(this, buffers_state);
 }
 
 }  // namespace media