Revert 142862 - Change the way we are stopping audio stream on Mac once again.

r142235 was not enough, we were not touching audio buffers after stop,
but OS could still touch audio queue. That causes problems if we were
deleting audio stream immediately after stopping, e.g. if audio mixer
tried to keep audio stream opened after last logical stream stopped
and then stopped/closed it.

I was able to reproduce the problem locally (for whatever reason this time
it worked), and after the fix it (finally) went away -- browsert_tests
PPAPITest.Audio_Creation successfully runs for 300 iterations. When I revert
to previos way of signaling, problem resurfaces again.

Fix is to use "property listener" to listen to "is running" audio queue
property, and signal "stream stopped" event only after "is running" property
changes to false. Functions that do are documented in the Apple core audio
documentation but not used in samples, so they are hard to find if you don't
know what to look for.
 
Also re-enabling mixer change to keep physical stream opened for some time,
That should complete browser-side mixer work and fix several related bugs.

BUG=102395
BUG=114701
BUG=129190
BUG=131720

TEST=No observable diffs, but crashes and seek problems should go away.

Review URL: https://chromiumcodereview.appspot.com/10560038

TBR=enal@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10583009

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

3 files changed, 56 insertions, 79 deletions

diff --git a/media/audio/audio_output_mixer.cc b/media/audio/audio_output_mixer.cc
index 542db78..edce4ea 100644
--- a/media/audio/audio_output_mixer.cc
+++ b/media/audio/audio_output_mixer.cc
@@ -47,8 +47,6 @@ bool AudioOutputMixer::OpenStream() {
   }
   pending_bytes_ = 0;  // Just in case.
   physical_stream_.reset(stream);
-  physical_stream_->SetVolume(1.0);
-  physical_stream_->Start(this);
   close_timer_.Reset();
   return true;
 }
@@ -66,24 +64,46 @@ bool AudioOutputMixer::StartStream(
 
   double volume = 0.0;
   stream_proxy->GetVolume(&volume);
-
-  base::AutoLock lock(lock_);
-  ProxyData* proxy_data = &proxies_[stream_proxy];
-  proxy_data->audio_source_callback = callback;
-  proxy_data->volume = volume;
-  proxy_data->pending_bytes = 0;
+  bool should_start = proxies_.empty();
+  {
+    base::AutoLock lock(lock_);
+    ProxyData* proxy_data = &proxies_[stream_proxy];
+    proxy_data->audio_source_callback = callback;
+    proxy_data->volume = volume;
+    proxy_data->pending_bytes = 0;
+  }
+  // We cannot start physical stream under the lock,
+  // OnMoreData() would try acquiring it...
+  if (should_start) {
+    physical_stream_->SetVolume(1.0);
+    physical_stream_->Start(this);
+  }
   return true;
 }
 
 void AudioOutputMixer::StopStream(AudioOutputProxy* stream_proxy) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  base::AutoLock lock(lock_);
-  ProxyMap::iterator it = proxies_.find(stream_proxy);
-  if (it != proxies_.end())
-    proxies_.erase(it);
-  if (physical_stream_.get())
+  // Because of possible deadlock we cannot stop physical stream under the lock
+  // (physical_stream_->Stop() can call OnError(), and it acquires the lock to
+  // iterate through proxies), so acquire the lock, update proxy list, release
+  // the lock, and only then stop physical stream if necessary.
+  bool stop_physical_stream = false;
+  {
+    base::AutoLock lock(lock_);
+    ProxyMap::iterator it = proxies_.find(stream_proxy);
+    if (it != proxies_.end()) {
+      proxies_.erase(it);
+      stop_physical_stream = proxies_.empty();
+    }
+  }
+  if (physical_stream_.get()) {
+    if (stop_physical_stream) {
+      physical_stream_->Stop();
+      pending_bytes_ = 0;  // Just in case.
+    }
     close_timer_.Reset();
+  }
 }
 
 void AudioOutputMixer::StreamVolumeSet(AudioOutputProxy* stream_proxy,
@@ -124,10 +144,8 @@ void AudioOutputMixer::Shutdown() {
 void AudioOutputMixer::ClosePhysicalStream() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  if (proxies_.empty() && physical_stream_.get() != NULL) {
-    physical_stream_->Stop();
+  if (proxies_.empty() && physical_stream_.get() != NULL)
     physical_stream_.release()->Close();
-  }
 }
 
 // AudioSourceCallback implementation.
diff --git a/media/audio/mac/audio_output_mac.cc b/media/audio/mac/audio_output_mac.cc
index 6f49017..ecfd0ad 100644
--- a/media/audio/mac/audio_output_mac.cc
+++ b/media/audio/mac/audio_output_mac.cc
@@ -60,7 +60,7 @@ PCMQueueOutAudioOutputStream::PCMQueueOutAudioOutputStream(
       should_swizzle_(false),
       should_down_mix_(false),
       stopped_event_(true /* manual reset */, false /* initial state */),
-      stop_requested_(false) {
+      num_buffers_left_(kNumBuffers) {
   // We must have a manager.
   DCHECK(manager_);
   // A frame is one sample across all channels. In interleaved audio the per
@@ -379,37 +379,6 @@ bool PCMQueueOutAudioOutputStream::CheckForAdjustedLayout(
   return false;
 }
 
-void PCMQueueOutAudioOutputStream::IsRunningCallback(
-    void* p_this,
-    AudioQueueRef queue,
-    AudioQueuePropertyID inID) {
-  PCMQueueOutAudioOutputStream* audio_stream =
-      static_cast<PCMQueueOutAudioOutputStream*>(p_this);
-  UInt32 running = 0;
-  UInt32 size = sizeof(running);
-  OSStatus err = AudioQueueGetProperty(queue,
-                                       kAudioQueueProperty_IsRunning,
-                                       &running,
-                                       &size);
-  if (err) {
-    audio_stream->HandleError(err);
-    return;
-  }
-  if (!running) {
-    // Remove property listener, we don't need it anymore.
-    OSStatus err = AudioQueueRemovePropertyListener(
-        queue,
-        kAudioQueueProperty_IsRunning,
-        PCMQueueOutAudioOutputStream::IsRunningCallback,
-        audio_stream);
-    if (err != noErr)
-      audio_stream->HandleError(err);
-
-    // Ok, finally signal that stream is not running.
-    audio_stream->stopped_event_.Signal();
-  }
-}
-
 // Note to future hackers of this function: Do not add locks to this function
 // that are held through any calls made back into AudioQueue APIs, or other
 // OS audio functions.  This is because the OS dispatch may grab external
@@ -427,28 +396,25 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
   AudioSourceCallback* source = audio_stream->GetSource();
   if (!source) {
     // PCMQueueOutAudioOutputStream::Stop() is waiting for callback to
-    // stop the stream and signal when all activity ceased (we probably
-    // can stop the stream there, but it is better to have all the
-    // complex logic in one place; stopping latency is not very important
-    // if you reuse audio stream in the mixer and not close it immediately).
-    if (!audio_stream->stop_requested_) {
-      audio_stream->stop_requested_ = true;
-      // Before actually stopping the stream, register callback that
-      // OS would call when status of "is running" property changes.
-      // This way we (hopefully) would know when stream is actually
-      // stopped and it is safe to dispose it.
-      OSStatus err = AudioQueueAddPropertyListener(
-          queue,
-          kAudioQueueProperty_IsRunning,
-          PCMQueueOutAudioOutputStream::IsRunningCallback,
-          audio_stream);
-      if (err != noErr)
-        audio_stream->HandleError(err);
-      // Now stop the queue.
-      err = AudioQueueStop(queue, true);
+    // stop the stream and signal when all callbacks are done.
+    // (we probably can stop the stream there, but it is better to have
+    // all the complex logic in one place; stopping latency is not very
+    // important if you reuse audio stream in the mixer and not close it
+    // immediately).
+    --audio_stream->num_buffers_left_;
+    if (audio_stream->num_buffers_left_ == kNumBuffers - 1) {
+      // First buffer after stop requested, stop the queue.
+      OSStatus err = AudioQueueStop(audio_stream->audio_queue_, true);
       if (err != noErr)
         audio_stream->HandleError(err);
     }
+    if (audio_stream->num_buffers_left_ == 0) {
+      // Now we finally saw all the buffers.
+      // Signal that stopping is complete.
+      // Should never touch audio_stream after signaling as it
+      // can be deleted at any moment.
+      audio_stream->stopped_event_.Signal();
+    }
     return;
   }
 
@@ -543,7 +509,7 @@ void PCMQueueOutAudioOutputStream::Start(AudioSourceCallback* callback) {
   SetSource(callback);
   pending_bytes_ = 0;
   stopped_event_.Reset();
-  stop_requested_ = false;
+  num_buffers_left_ = kNumBuffers;
   // Ask the source to pre-fill all our buffers before playing.
   for (uint32 ix = 0; ix != kNumBuffers; ++ix) {
     buffer_[ix]->mAudioDataByteSize = 0;
diff --git a/media/audio/mac/audio_output_mac.h b/media/audio/mac/audio_output_mac.h
index c04f546..381592b 100644
--- a/media/audio/mac/audio_output_mac.h
+++ b/media/audio/mac/audio_output_mac.h
@@ -51,11 +51,6 @@ class PCMQueueOutAudioOutputStream : public AudioOutputStream {
   // Check and move channels if surround sound layout needs adjusted.
   bool CheckForAdjustedLayout(Channels input_channel, Channels output_channel);
 
-  // Callback we are installing to find out when the stream is stopped.
-  static void IsRunningCallback(void* p_this,
-                                AudioQueueRef queue,
-                                AudioQueuePropertyID inID);
-
   // The OS calls back here when an audio buffer has been processed.
   static void RenderCallback(void* p_this, AudioQueueRef queue,
                              AudioQueueBufferRef buffer);
@@ -103,13 +98,11 @@ class PCMQueueOutAudioOutputStream : public AudioOutputStream {
   bool should_down_mix_;
 
   // Event used for synchronization when stopping the stream.
-  // Callback sets it after stream is fully stopped.
+  // Callback sets it after stream is stopped.
   base::WaitableEvent stopped_event_;
-  // Flag set by rendering callback when it asks OS to stop the stream.
-  // All subsequent renderer callbacks calls just exit immediately, they
-  // are called to release the buffer (and we do it when closing the stream,
-  // not in the callback).
-  bool stop_requested_;
+  // When stopping we keep track of number of buffers in flight and
+  // signal "stop completed" from the last buffer's callback.
+  int num_buffers_left_;
 
   DISALLOW_COPY_AND_ASSIGN(PCMQueueOutAudioOutputStream);
 };