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

media/audio/audio_output_controller.cc

Index: media/audio/audio_output_controller.cc
===================================================================
--- media/audio/audio_output_controller.cc	(revision 106670)
+++ media/audio/audio_output_controller.cc	(working copy)
@@ -176,9 +176,10 @@
   // 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);
 
@@ -199,7 +200,7 @@
 
   // 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()) {
@@ -214,6 +215,7 @@
 
 void AudioOutputController::StartStream() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
+  state_ = kPlaying;
 
   // We start the AudioOutputStream lazily.
   stream_->Start(this);
@@ -225,22 +227,38 @@
 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() {
@@ -287,10 +305,16 @@
   // 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) {