Fix erratic HTML5 audio playback

This was affecting all platforms.  The major issue was that he SetPlaybackRate(1) which is used to start playback was being done in the middle of the series of Seek operations.  So doing Seek() followed by SetPlaybackRate(1) was done, the Seek could immediately stop the playback.  To fix, Seek() was made atomic with regards to SetPlaybackRate() by delaying the execution of SetPlaybackRate until after the Seek sequence has completely finished.

To make things run even smoother, a short delay was added before recyclilng a used audio stream in the Dispatcher.  This gives the stream time to 'power down' before being reused.

Tested on Linux, Chrome-OS (Cr48) and Mac with great results, making HTML5 audio much more usable for games.

BUG=73045,59369,59370,65618
TEST=Manual, Quickly playing/stopping HTML5 audio should play cleanly.

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

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

4 files changed, 77 insertions, 21 deletions

diff --git a/media/audio/audio_output_dispatcher.cc b/media/audio/audio_output_dispatcher.cc
index 3f9d848..c707069 100644
--- a/media/audio/audio_output_dispatcher.cc
+++ b/media/audio/audio_output_dispatcher.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -6,6 +6,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/message_loop.h"
+#include "base/time.h"
 #include "media/audio/audio_io.h"
 
 AudioOutputDispatcher::AudioOutputDispatcher(
@@ -14,6 +15,8 @@ AudioOutputDispatcher::AudioOutputDispatcher(
     : audio_manager_(audio_manager),
       message_loop_(audio_manager->GetMessageLoop()),
       params_(params),
+      pause_delay_milliseconds_(2 * params.samples_per_packet *
+          base::Time::kMillisecondsPerSecond / params.sample_rate),
       paused_proxies_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(close_timer_(
           base::TimeDelta::FromMilliseconds(close_delay_ms),
@@ -28,7 +31,7 @@ bool AudioOutputDispatcher::StreamOpened() {
   paused_proxies_++;
 
   // Ensure that there is at least one open stream.
-  if (streams_.empty() && !CreateAndOpenStream()) {
+  if (idle_streams_.empty() && !CreateAndOpenStream()) {
     return false;
   }
 
@@ -40,12 +43,12 @@ bool AudioOutputDispatcher::StreamOpened() {
 AudioOutputStream* AudioOutputDispatcher::StreamStarted() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  if (streams_.empty() && !CreateAndOpenStream()) {
+  if (idle_streams_.empty() && !CreateAndOpenStream()) {
     return NULL;
   }
 
-  AudioOutputStream* stream = streams_.back();
-  streams_.pop_back();
+  AudioOutputStream* stream = idle_streams_.back();
+  idle_streams_.pop_back();
 
   DCHECK_GT(paused_proxies_, 0u);
   paused_proxies_--;
@@ -61,20 +64,41 @@ AudioOutputStream* AudioOutputDispatcher::StreamStarted() {
 
 void AudioOutputDispatcher::StreamStopped(AudioOutputStream* stream) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
+
   paused_proxies_++;
-  streams_.push_back(stream);
+
+  pausing_streams_.push_front(stream);
   close_timer_.Reset();
+
+  // Don't recycle stream until two buffers worth of time has elapsed.
+  message_loop_->PostDelayedTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &AudioOutputDispatcher::StopStreamTask),
+      pause_delay_milliseconds_);
+}
+
+void AudioOutputDispatcher::StopStreamTask() {
+  if (pausing_streams_.empty())
+    return;
+  AudioOutputStream* stream = pausing_streams_.back();
+  pausing_streams_.pop_back();
+  idle_streams_.push_back(stream);
 }
 
 void AudioOutputDispatcher::StreamClosed() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
+  while (!pausing_streams_.empty()) {
+    idle_streams_.push_back(pausing_streams_.back());
+    pausing_streams_.pop_back();
+  }
+
   DCHECK_GT(paused_proxies_, 0u);
   paused_proxies_--;
 
-  while (streams_.size() > paused_proxies_) {
-    streams_.back()->Close();
-    streams_.pop_back();
+  while (idle_streams_.size() > paused_proxies_) {
+    idle_streams_.back()->Close();
+    idle_streams_.pop_back();
   }
 }
 
@@ -92,14 +116,15 @@ bool AudioOutputDispatcher::CreateAndOpenStream() {
     stream->Close();
     return false;
   }
-  streams_.push_back(stream);
+  idle_streams_.push_back(stream);
   return true;
 }
 
 void AudioOutputDispatcher::OpenTask() {
   // Make sure that we have at least one stream allocated if there
   // are paused streams.
-  if (paused_proxies_ > 0 && streams_.empty()) {
+  if (paused_proxies_ > 0 && idle_streams_.empty() &&
+      pausing_streams_.empty()) {
     CreateAndOpenStream();
   }
 
@@ -108,8 +133,8 @@ void AudioOutputDispatcher::OpenTask() {
 
 // This method is called by |close_timer_|.
 void AudioOutputDispatcher::ClosePendingStreams() {
-  while (!streams_.empty()) {
-    streams_.back()->Close();
-    streams_.pop_back();
+  while (!idle_streams_.empty()) {
+    idle_streams_.back()->Close();
+    idle_streams_.pop_back();
   }
 }
diff --git a/media/audio/audio_output_dispatcher.h b/media/audio/audio_output_dispatcher.h
index 2289e89..18e1b28 100644
--- a/media/audio/audio_output_dispatcher.h
+++ b/media/audio/audio_output_dispatcher.h
@@ -22,6 +22,7 @@
 #define MEDIA_AUDIO_AUDIO_OUTPUT_DISPATCHER_H_
 
 #include <vector>
+#include <list>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
@@ -43,19 +44,20 @@ class AudioOutputDispatcher
   ~AudioOutputDispatcher();
 
   // Called by AudioOutputProxy when the stream is closed. Opens a new
-  // physical stream if there are no pending streams in |streams_|.
+  // physical stream if there are no pending streams in |idle_streams_|.
   // Returns false, if it fails to open it.
   bool StreamOpened();
 
   // Called by AudioOutputProxy when the stream is started. If there
-  // are pending streams in |streams_| then it returns one of them,
+  // are pending streams in |idle_streams_| then it returns one of them,
   // otherwise creates a new one. Returns a physical stream that must
   // be used, or NULL if it fails to open audio device. Ownership of
   // the result is passed to the caller.
   AudioOutputStream* StreamStarted();
 
-  // Called by AudioOutputProxy when the stream is stopped. Returns
-  // |stream| to the pool of pending streams (i.e. |streams_|).
+  // Called by AudioOutputProxy when the stream is stopped.  Holds the
+  // stream temporarily in |pausing_streams_| and then |stream| is
+  // added to the pool of pending streams (i.e. |idle_streams_|).
   // Ownership of the |stream| is passed to the dispatcher.
   void StreamStopped(AudioOutputStream* stream);
 
@@ -68,14 +70,18 @@ class AudioOutputDispatcher
   friend class AudioOutputProxyTest;
 
   // Creates a new physical output stream, opens it and pushes to
-  // |streams_|.  Returns false if the stream couldn't be created or
+  // |idle_streams_|.  Returns false if the stream couldn't be created or
   // opened.
   bool CreateAndOpenStream();
 
   // A task scheduled by StreamStarted(). Opens a new stream and puts
-  // it in |streams_|.
+  // it in |idle_streams_|.
   void OpenTask();
 
+  // Before a stream is reused, it should sit idle for a bit.  This task is
+  // called once that time has elapsed.
+  void StopStreamTask();
+
   // Called by |close_timer_|. Closes all pending stream.
   void ClosePendingStreams();
 
@@ -83,8 +89,10 @@ class AudioOutputDispatcher
   MessageLoop* message_loop_;
   AudioParameters params_;
 
+  int64 pause_delay_milliseconds_;
   size_t paused_proxies_;
-  std::vector<AudioOutputStream*> streams_;
+  std::vector<AudioOutputStream*> idle_streams_;
+  std::list<AudioOutputStream*> pausing_streams_;
   base::DelayTimer<AudioOutputDispatcher> close_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioOutputDispatcher);
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index f574b85..25bdf47 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -358,6 +358,7 @@ void PipelineImpl::ResetState() {
   seek_pending_     = false;
   tearing_down_     = false;
   error_caused_teardown_ = false;
+  playback_rate_change_pending_ = false;
   duration_         = kZero;
   buffered_time_    = kZero;
   buffered_bytes_   = 0;
@@ -369,6 +370,7 @@ void PipelineImpl::ResetState() {
   volume_           = 1.0f;
   preload_          = AUTO;
   playback_rate_    = 0.0f;
+  pending_playback_rate_ = 0.0f;
   status_           = PIPELINE_OK;
   has_audio_        = false;
   has_video_        = false;
@@ -780,6 +782,14 @@ void PipelineImpl::ErrorChangedTask(PipelineStatus error) {
 
 void PipelineImpl::PlaybackRateChangedTask(float playback_rate) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  // Suppress rate change until after seeking.
+  if (IsPipelineSeeking()) {
+    pending_playback_rate_ = playback_rate;
+    playback_rate_change_pending_ = true;
+    return;
+  }
+
   {
     base::AutoLock auto_lock(lock_);
     clock_->SetPlaybackRate(playback_rate);
@@ -959,6 +969,13 @@ void PipelineImpl::FilterStateTransitionTask() {
     seek_timestamp_ = base::TimeDelta();
     seek_pending_ = false;
 
+    // If a playback rate change was requested during a seek, do it now that
+    // the seek has compelted.
+    if (playback_rate_change_pending_) {
+      playback_rate_change_pending_ = false;
+      PlaybackRateChangedTask(pending_playback_rate_);
+    }
+
     base::AutoLock auto_lock(lock_);
     // We use audio stream to update the clock. So if there is such a stream,
     // we pause the clock until we receive a valid timestamp.
diff --git a/media/base/pipeline_impl.h b/media/base/pipeline_impl.h
index 5dcbcea..7b5fe4a 100644
--- a/media/base/pipeline_impl.h
+++ b/media/base/pipeline_impl.h
@@ -330,6 +330,9 @@ class PipelineImpl : public Pipeline, public FilterHost {
   // Whether or not an error triggered the teardown.
   bool error_caused_teardown_;
 
+  // Whether or not a playback rate change should be done once seeking is done.
+  bool playback_rate_change_pending_;
+
   // Duration of the media in microseconds.  Set by filters.
   base::TimeDelta duration_;
 
@@ -372,6 +375,9 @@ class PipelineImpl : public Pipeline, public FilterHost {
   // the filters.
   float playback_rate_;
 
+  // Playback rate to set when the current seek has finished.
+  float pending_playback_rate_;
+
   // Reference clock.  Keeps track of current playback time.  Uses system
   // clock and linear interpolation, but can have its time manually set
   // by filters.