Problem happens because we cannot stop physical stream in arbitrary moment, callback thread may either ask for more data or feeding new data to audio driver. So we were setting flag and waiting for callback thread to signal that it saw the flag, stopped its activity and now it is safe to stop.

Changed the code to use critical section instead. Callback thread tries to enter critical section before doing work. If it cannot it assumes that main thread issued stop and waits for all buffers to be returned and callback thread to be terminated. By using TryEnterCriticalSection() instead of EnterCriticalSection() we are avoiding deadlock (there are internal Windows locks being involved, so EnterCriticalSection() in the callback will deadlock).

So we would stop immediately if callback is not active, compared for waiting for callback to be called before the change, at a cost of extra enter/leave critical section per callback.

Tested on Win7 and XP.

(That is result of my Windows-specific audio crash investigation -- I was wrong why it happened, but while investigation I had that speedup...)

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

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

2 files changed, 45 insertions, 39 deletions

diff --git a/media/audio/win/waveout_output_win.cc b/media/audio/win/waveout_output_win.cc
index 6933756..288c026 100644
--- a/media/audio/win/waveout_output_win.cc
+++ b/media/audio/win/waveout_output_win.cc
@@ -15,6 +15,10 @@
 #include "media/audio/audio_util.h"
 #include "media/audio/win/audio_manager_win.h"
 
+// Number of times InitializeCriticalSectionAndSpinCount() spins
+// before going to sleep.
+const DWORD kSpinCount = 2000;
+
 // Some general thoughts about the waveOut API which is badly documented :
 // - We use CALLBACK_FUNCTION mode in which XP secretly creates two threads
 //   named _MixerCallbackThread and _waveThread which have real-time priority.
@@ -89,6 +93,7 @@ PCMWaveOutAudioOutputStream::PCMWaveOutAudioOutputStream(
       volume_(1),
       channels_(params.channels),
       pending_bytes_(0) {
+  ::InitializeCriticalSectionAndSpinCount(&lock_, kSpinCount);
 
   format_.Format.wFormatTag = WAVE_FORMAT_EXTENSIBLE;
   format_.Format.nChannels = params.channels;
@@ -107,12 +112,11 @@ PCMWaveOutAudioOutputStream::PCMWaveOutAudioOutputStream(
   }
   format_.SubFormat = KSDATAFORMAT_SUBTYPE_PCM;
   format_.Samples.wValidBitsPerSample = params.bits_per_sample;
-  // The event is auto-reset.
-  stopped_event_.Set(::CreateEventW(NULL, FALSE, FALSE, NULL));
 }
 
 PCMWaveOutAudioOutputStream::~PCMWaveOutAudioOutputStream() {
   DCHECK(NULL == waveout_);
+  ::DeleteCriticalSection(&lock_);
 }
 
 bool PCMWaveOutAudioOutputStream::Open() {
@@ -186,11 +190,18 @@ void PCMWaveOutAudioOutputStream::Start(AudioSourceCallback* callback) {
     buffer = GetNextBuffer(buffer);
   }
   buffer = buffer_;
+
+  // From now on |pending_bytes_| would be accessed by callback thread.
+  // Most likely waveOutPause() or waveOutRestart() has its own memory barrier,
+  // but issuing our own is safer.
+  MemoryBarrier();
+
   MMRESULT result = ::waveOutPause(waveout_);
   if (result != MMSYSERR_NOERROR) {
     HandleError(result);
     return;
   }
+
   // Send the buffers to the audio driver. Note that the device is paused
   // so we avoid entering the callback method while still here.
   for (int ix = 0; ix != num_buffers_; ++ix) {
@@ -211,20 +222,18 @@ void PCMWaveOutAudioOutputStream::Start(AudioSourceCallback* callback) {
 // Stopping is tricky. First, no buffer should be locked by the audio driver
 // or else the waveOutReset() will deadlock and secondly, the callback should
 // not be inside the AudioSource's OnMoreData because waveOutReset() forcefully
-// kills the callback thread.
+// kills the callback thread after releasing all buffers.
 void PCMWaveOutAudioOutputStream::Stop() {
   if (state_ != PCMA_PLAYING)
     return;
-  state_ = PCMA_STOPPING;
-  // Wait for the callback to finish, it will signal us when ready to be reset.
-  if (WAIT_OBJECT_0 != ::WaitForSingleObject(stopped_event_, INFINITE)) {
-    HandleError(::GetLastError());
-    return;
-  }
-  state_ = PCMA_STOPPED;
+
+  // Enter into critical section and call ::waveOutReset(). The fact that we
+  // entered critical section means that callback is out of critical section and
+  // it is safe to reset.
+  ::EnterCriticalSection(&lock_);
   MMRESULT res = ::waveOutReset(waveout_);
+  ::LeaveCriticalSection(&lock_);
   if (res != MMSYSERR_NOERROR) {
-    state_ = PCMA_PLAYING;
     HandleError(res);
     return;
   }
@@ -312,39 +321,31 @@ void PCMWaveOutAudioOutputStream::WaveCallback(HWAVEOUT hwo, UINT msg,
                                                DWORD_PTR param1, DWORD_PTR) {
   TRACE_EVENT0("audio", "PCMWaveOutAudioOutputStream::WaveCallback");
 
-  PCMWaveOutAudioOutputStream* obj =
-      reinterpret_cast<PCMWaveOutAudioOutputStream*>(instance);
-
   if (msg == WOM_DONE) {
     // WOM_DONE indicates that the driver is done with our buffer, we can
     // either ask the source for more data or check if we need to stop playing.
     WAVEHDR* buffer = reinterpret_cast<WAVEHDR*>(param1);
     buffer->dwFlags = WHDR_DONE;
 
-    if (obj->state_ == PCMA_STOPPING) {
-      // The main thread has called Stop() and is waiting to issue waveOutReset
-      // which will kill this thread. We should not enter AudioSourceCallback
-      // code anymore.
-      ::SetEvent(obj->stopped_event_);
-      return;
-    } else if (obj->state_ == PCMA_STOPPED) {
-      // Not sure if ever hit this but just in case.
-      return;
-    }
-
-    // Before we queue the next packet, we need to adjust the number of pending
-    // bytes since the last write to hardware.
-    obj->pending_bytes_ -= buffer->dwBufferLength;
+    PCMWaveOutAudioOutputStream* stream =
+        reinterpret_cast<PCMWaveOutAudioOutputStream*>(instance);
 
-    obj->QueueNextPacket(buffer);
+    // Do real work only if main thread has not yet called waveOutReset().
+    if (::TryEnterCriticalSection(&stream->lock_)) {
+      // Before we queue the next packet, we need to adjust the number of
+      // pending bytes since the last write to hardware.
+      stream->pending_bytes_ -= buffer->dwBufferLength;
 
-    // Time to send the buffer to the audio driver. Since we are reusing
-    // the same buffers we can get away without calling waveOutPrepareHeader.
-    MMRESULT result = ::waveOutWrite(hwo, buffer, sizeof(WAVEHDR));
-    if (result != MMSYSERR_NOERROR)
-      obj->HandleError(result);
+      stream->QueueNextPacket(buffer);
 
-    obj->pending_bytes_ += buffer->dwBufferLength;
+      // Time to send the buffer to the audio driver. Since we are reusing
+      // the same buffers we can get away without calling waveOutPrepareHeader.
+      MMRESULT result = ::waveOutWrite(hwo, buffer, sizeof(WAVEHDR));
+      if (result != MMSYSERR_NOERROR)
+        stream->HandleError(result);
 
+      stream->pending_bytes_ += buffer->dwBufferLength;
+      ::LeaveCriticalSection(&stream->lock_);
+    }
   }
 }
diff --git a/media/audio/win/waveout_output_win.h b/media/audio/win/waveout_output_win.h
index 5b7aef9..10ed073 100644
--- a/media/audio/win/waveout_output_win.h
+++ b/media/audio/win/waveout_output_win.h
@@ -53,8 +53,6 @@ class PCMWaveOutAudioOutputStream : public AudioOutputStream {
     PCMA_BRAND_NEW,    // Initial state.
     PCMA_READY,        // Device obtained and ready to play.
     PCMA_PLAYING,      // Playing audio.
-    PCMA_STOPPING,     // Trying to stop, waiting for callback to finish.
-    PCMA_STOPPED,      // Stopped. Device was reset.
     PCMA_CLOSED        // Device has been released.
   };
 
@@ -112,8 +110,15 @@ class PCMWaveOutAudioOutputStream : public AudioOutputStream {
   // Pointer to the first allocated audio buffer. This object owns it.
   WAVEHDR* buffer_;
 
-  // An event that is signaled when the callback thread is ready to stop.
-  base::win::ScopedHandle stopped_event_;
+  // Lock used to prevent stopping the hardware callback thread while it is
+  // pending for data or feeding it to audio driver, because doing that causes
+  // the deadlock. Main thread gets that lock before stopping the playback.
+  // Callback tries to acquire that lock before entering critical code. If
+  // acquire fails then main thread is stopping the playback, callback should
+  // immediately return.
+  // Use Windows-specific lock, not Chrome one, because there is limited set of
+  // functions callback can use.
+  CRITICAL_SECTION lock_;
 
   DISALLOW_COPY_AND_ASSIGN(PCMWaveOutAudioOutputStream);
 };