Harden audio output controller making it safe to call Pause() before Play() really started. Not sure if it really fixes the crash, but crash started happening only after Play() was split into several tasks and become much slower (http://codereview.chromium.org/8229013/ change 105311).

Added unit test.

BUG=100650

Review URL: http://codereview.chromium.org/8371013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@106983 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 120 insertions, 40 deletions

diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index 7118eaa..cd57bed 100644
--- a/media/audio/audio_output_controller.cc
+++ b/media/audio/audio_output_controller.cc
@@ -176,9 +176,10 @@ void AudioOutputController::DoPlay() {
   // 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);
 
@@ -197,13 +198,20 @@ void AudioOutputController::DoPlay() {
 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)
+  // Being paranoic: do nothing if state unexpectedly changed.
+  if ((state_ != kStarting) && (state_ != kPausedWhenStarting))
     return;
 
-  if (--number_polling_attempts_left_ == 0 || sync_reader_->DataReady()) {
+  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,
@@ -214,6 +222,7 @@ void AudioOutputController::PollAndStartIfDataReady() {
 
 void AudioOutputController::StartStream() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
+  state_ = kPlaying;
 
   // We start the AudioOutputStream lazily.
   stream_->Start(this);
@@ -225,22 +234,32 @@ void AudioOutputController::StartStream() {
 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.
+      // Delay pausing, let delayed task to do pause after it start playback.
+      state_ = kPausedWhenStarting;
+      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 +306,17 @@ void AudioOutputController::DoSetVolume(double 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 kPausedWhenStarting:
+    case kPlaying:
+    case kPaused:
+      stream_->SetVolume(volume_);
+      break;
+    default:
+      return;
+  }
 }
 
 void AudioOutputController::DoReportError(int code) {
diff --git a/media/audio/audio_output_controller.h b/media/audio/audio_output_controller.h
index 2c2246b..e359d27c 100644
--- a/media/audio/audio_output_controller.h
+++ b/media/audio/audio_output_controller.h
@@ -25,15 +25,21 @@ class MessageLoop;
 // All the public methods of AudioOutputController are non-blocking except
 // close, the actual operations are performed on the audio controller thread.
 //
-// Here is a state diagram for the AudioOutputController:
+// Here is a state diagram for the AudioOutputController for default low
+// latency mode; in normal latency mode there is no "starting" or "paused when
+// starting" states, "created" immediately switches to "playing":
 //
-//                    .---->  [ Closed / Error ]  <------.
-//                    |                ^                 |
-//                    |                |                 |
-//               [ Created ]  -->  [ Playing ]  -->  [ Paused ]
-//                    ^                ^                 |
-//                    |                |                 |
-//              *[  Empty  ]           `-----------------'
+//             .----------------------->  [ Closed / Error ]  <------.
+//             |                                   ^                 |
+//             |                                   |                 |
+//        [ Created ]  -->  [ Starting ]  -->  [ Playing ]  -->  [ Paused ]
+//             ^                 |                 ^                |  ^
+//             |                 |                 |                |  |
+//             |                 |                 `----------------'  |
+//             |                 V                                     |
+//             |        [ PausedWhenStarting ] ------------------------'
+//             |
+//       *[  Empty  ]
 //
 // * Initial state
 //
@@ -55,16 +61,6 @@ class MEDIA_EXPORT AudioOutputController
     : public base::RefCountedThreadSafe<AudioOutputController>,
       public AudioOutputStream::AudioSourceCallback {
  public:
-  // Internal state of the source.
-  enum State {
-    kEmpty,
-    kCreated,
-    kPlaying,
-    kPaused,
-    kClosed,
-    kError,
-  };
-
   // Value sent by the controller to the renderer in low-latency mode
   // indicating that the stream is paused.
   static const int kPauseMark;
@@ -169,6 +165,19 @@ class MEDIA_EXPORT AudioOutputController
                             uint32 max_size, AudioBuffersState buffers_state);
   virtual void OnError(AudioOutputStream* stream, int code);
 
+ protected:
+    // Internal state of the source.
+  enum State {
+    kEmpty,
+    kCreated,
+    kPlaying,
+    kStarting,
+    kPausedWhenStarting,
+    kPaused,
+    kClosed,
+    kError,
+  };
+
  private:
   // We are polling sync reader if data became available.
   static const int kPollNumAttempts;
diff --git a/media/audio/audio_output_controller_unittest.cc b/media/audio/audio_output_controller_unittest.cc
index 6a58bb5..74e0e68 100644
--- a/media/audio/audio_output_controller_unittest.cc
+++ b/media/audio/audio_output_controller_unittest.cc
@@ -258,6 +258,51 @@ TEST(AudioOutputControllerTest, PlayPauseClose) {
   CloseAudioController(controller);
 }
 
+TEST(AudioOutputControllerTest, PlayPauseCloseLowLatency) {
+  if (!HasAudioOutputDevices() || IsRunningHeadless())
+    return;
+
+  MockAudioOutputControllerEventHandler event_handler;
+  base::WaitableEvent event(false, false);
+  base::WaitableEvent pause_event(false, false);
+
+  // If OnCreated is called then signal the event.
+  EXPECT_CALL(event_handler, OnCreated(NotNull()))
+      .WillOnce(InvokeWithoutArgs(&event, &base::WaitableEvent::Signal));
+
+  // OnPlaying() will be called only once.
+  EXPECT_CALL(event_handler, OnPlaying(NotNull()));
+
+  MockAudioOutputControllerSyncReader sync_reader;
+  EXPECT_CALL(sync_reader, UpdatePendingBytes(_))
+      .Times(AtLeast(2));
+  EXPECT_CALL(sync_reader, Read(_, kHardwareBufferSize))
+      .WillRepeatedly(DoAll(SignalEvent(&event),
+                            Return(4)));
+  EXPECT_CALL(event_handler, OnPaused(NotNull()))
+      .WillOnce(InvokeWithoutArgs(&pause_event, &base::WaitableEvent::Signal));
+  EXPECT_CALL(sync_reader, Close());
+
+  AudioParameters params(AudioParameters::AUDIO_PCM_LINEAR, kChannelLayout,
+                         kSampleRate, kBitsPerSample, kSamplesPerPacket);
+  scoped_refptr<AudioOutputController> controller =
+      AudioOutputController::CreateLowLatency(&event_handler,
+                                              params,
+                                              &sync_reader);
+  ASSERT_TRUE(controller.get());
+
+  // Wait for OnCreated() to be called.
+  event.Wait();
+
+  ASSERT_FALSE(pause_event.IsSignaled());
+  controller->Play();
+  controller->Pause();
+  pause_event.Wait();
+
+  // Now stop the controller.
+  CloseAudioController(controller);
+}
+
 TEST(AudioOutputControllerTest, PlayPausePlay) {
   if (!HasAudioOutputDevices() || IsRunningHeadless())
     return;