Removing a synchronous pause that reduces impact on the IO thread

Audio pause is implemented synchronously in the browser process.
This can easily choke the IO thread when user does a series of
pause of looping a very short audio / video. This change will
make the pause action asynchronous. The drawback of this change
is that we cannot stop audio playback immediately as we receive
the pause request.

This is still an improvement over chrome 3.0 as the "playback"
period after pause is noticably shorter.

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

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

1 files changed, 63 insertions, 36 deletions

diff --git a/chrome/browser/renderer_host/audio_renderer_host.cc b/chrome/browser/renderer_host/audio_renderer_host.cc
index 4b60da8..041023b 100644
--- a/chrome/browser/renderer_host/audio_renderer_host.cc
+++ b/chrome/browser/renderer_host/audio_renderer_host.cc
@@ -1,6 +1,24 @@
 // Copyright (c) 2009 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.
+//
+// TODO(hclam): Several changes need to be made to this code:
+// 1. We should host AudioRendererHost on a dedicated audio thread. Doing
+//    so we don't have to worry about blocking method calls such as
+//    play / stop an audio stream.
+// 2. Move locked data structures into a separate structure that sanity
+//    checks access by different threads that use it.
+//
+// SEMANTICS OF |state_|
+// Note that |state_| of IPCAudioSource is accessed on two thread. Namely
+// the IO thread and the audio thread. IO thread is the thread on which
+// IPAudioSource::Play(), IPCAudioSource::Pause() are called. Audio thread
+// is a thread operated by the audio hardware for requesting data.
+// It is important that |state_| is only written on the IO thread because
+// reading of such state in Play() and Pause() is not protected. However,
+// since OnMoreData() is called on the audio thread and reads |state_|
+// variable. Writing to this variable needs to be protected in Play()
+// and Pause().
 
 #include "base/histogram.h"
 #include "base/lock.h"
@@ -148,10 +166,15 @@ void AudioRendererHost::IPCAudioSource::Play() {
   if (!stream_ || (state_ != kCreated && state_ != kPaused))
     return;
 
-  stream_->Start(this);
+  if (state_ == kCreated) {
+    stream_->Start(this);
+  }
 
   // Update the state and notify renderer.
-  state_ = kPlaying;
+  {
+    AutoLock auto_lock(lock_);
+    state_ = kPlaying;
+  }
 
   ViewMsg_AudioStreamState state;
   state.state = ViewMsg_AudioStreamState::kPlaying;
@@ -164,12 +187,11 @@ void AudioRendererHost::IPCAudioSource::Pause() {
   if (!stream_ || state_ != kPlaying)
     return;
 
-  // TODO(hclam): use stop to simulate pause, make sure the AudioOutpusStream
-  // can be started again after stop.
-  stream_->Stop();
-
   // Update the state and notify renderer.
-  state_ = kPaused;
+  {
+    AutoLock auto_lock(lock_);
+    state_ = kPaused;
+  }
 
   ViewMsg_AudioStreamState state;
   state.state = ViewMsg_AudioStreamState::kPaused;
@@ -213,18 +235,29 @@ size_t AudioRendererHost::IPCAudioSource::OnMoreData(AudioOutputStream* stream,
                                                      void* dest,
                                                      size_t max_size,
                                                      int pending_bytes) {
-  size_t size = push_source_.OnMoreData(stream, dest, max_size, pending_bytes);
-  {
-    AutoLock auto_lock(lock_);
-    pending_bytes_ = pending_bytes + size;
-    last_callback_time_ = base::Time::Now();
-    SubmitPacketRequest(&auto_lock);
+  AutoLock auto_lock(lock_);
+
+  // Record the callback time.
+  last_callback_time_ = base::Time::Now();
+
+  if (state_ == kPaused) {
+    // Don't read anything from the push source and save the number of
+    // bytes in the hardware buffer.
+    pending_bytes_  = pending_bytes;
+    return 0;
   }
+
+  // Push source doesn't need to know the stream and number of pending bytes.
+  // So just pass in NULL and 0 for these two parameters.
+  size_t size = push_source_.OnMoreData(NULL, dest, max_size, 0);
+  pending_bytes_ = pending_bytes + size;
+  SubmitPacketRequest(&auto_lock);
   return size;
 }
 
 void AudioRendererHost::IPCAudioSource::OnClose(AudioOutputStream* stream) {
-  push_source_.OnClose(stream);
+  // Push source doesn't need to know the stream so just pass in NULL.
+  push_source_.OnClose(NULL);
 }
 
 void AudioRendererHost::IPCAudioSource::OnError(AudioOutputStream* stream,
@@ -237,31 +270,25 @@ void AudioRendererHost::IPCAudioSource::OnError(AudioOutputStream* stream,
 
 void AudioRendererHost::IPCAudioSource::NotifyPacketReady(
     size_t decoded_packet_size) {
+  AutoLock auto_lock(lock_);
   bool ok = true;
-  {
-    AutoLock auto_lock(lock_);
-    outstanding_request_ = false;
-    // If reported size is greater than capacity of the shared memory, we have
-    // an error.
-    if (decoded_packet_size <= decoded_packet_size_) {
-      for (size_t i = 0; i < decoded_packet_size; i += hardware_packet_size_) {
-        size_t size = std::min(decoded_packet_size - i, hardware_packet_size_);
-        ok &= push_source_.Write(
-            static_cast<char*>(shared_memory_.memory()) + i, size);
-        if (!ok)
-          break;
-      }
-
-      // Submit packet request if we have written something.
-      if (ok)
-        SubmitPacketRequest(&auto_lock);
+  outstanding_request_ = false;
+  // If reported size is greater than capacity of the shared memory, we have
+  // an error.
+  if (decoded_packet_size <= decoded_packet_size_) {
+    for (size_t i = 0; i < decoded_packet_size; i += hardware_packet_size_) {
+      size_t size = std::min(decoded_packet_size - i, hardware_packet_size_);
+      ok &= push_source_.Write(
+          static_cast<char*>(shared_memory_.memory()) + i, size);
+      // We have received a data packet but we didn't finish writing to push
+      // source. There's error an error and we should stop.
+      if (!ok)
+        NOTREACHED() << "Writing to push source failed.";
     }
-  }
 
-  // We have received a data packet but we didn't finish writing to push source.
-  // There's error an error and we should stop.
-  if (!ok) {
-    NOTREACHED();
+    // Submit packet request if we have written something.
+    if (ok)
+      SubmitPacketRequest(&auto_lock);
   }
 }