Revert 112147 - Did not get LGTM from all reviewers.

Change the way we are sending audio data to driver when using WaveOut API.
Before, we were "feeding" the driver in the callback when OS returned audio
buffer to us. MSDN disallows that, but We were avoiding deadlock when stopping
the stream by using some clever tricks. Unfortunately, exactly the same
deadlock happens when Windows were draining audio stream when switching
the device, and our tricks did not help, as we were not controlling exact timing.

Fix is to separate receiving freed buffer and "feeding" audio driver. Now we are
using CALLBACK_EVENT wave out mode in which Windows signal event when buffer runs
out of data, and using RegisterWaitForSingleObject() so our callback is called by
one of thread pool threads.

Stopping of playback became slower because now it is synchronous. If that ever
become a problem we can use singleton that keeps track of the playing audio
streams, than stopping can become instaneous, at a cost of more complex checks
in the callback.

Unfortunately, no automatic test, as there is no documented way to programmatically
switch default audio device on Windows. People were able to figure it out (see
http://www.daveamenta.com/2011-05/programmatically-or-command-line-change-the-default-sound-playback-device-in-windows-7/),
but there is no guarantee it will continue to work in Win8, or even in next Service Pack
for Win7.

BUG=41036

TEST=Play any HTML5 audio/video on Windows and change default audio device
TEST=using "Control Panel | Sound". There should be no hang, you should hear
TEST=audio from the newly chosen device.

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

TBR=enal@chromium.org
Review URL: http://codereview.chromium.org/8753001

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

3 files changed, 113 insertions, 163 deletions

diff --git a/media/audio/audio_util.cc b/media/audio/audio_util.cc
index 48be2db..4aec30d 100644
--- a/media/audio/audio_util.cc
+++ b/media/audio/audio_util.cc
@@ -293,11 +293,6 @@ size_t GetAudioHardwareBufferSize() {
 #if defined(OS_MACOSX)
   return 128;
 #elif defined(OS_WIN)
-  if (base::win::GetVersion() <= base::win::VERSION_XP) {
-    // Fall back to Windows Wave implementation on Windows XP or lower
-    // and assume 48kHz as default sample rate.
-    return 2048;
-  }
   // This call must be done on a COM thread configured as MTA.
   // TODO(tommi): http://code.google.com/p/chromium/issues/detail?id=103835.
   int mixing_sample_rate =
diff --git a/media/audio/win/waveout_output_win.cc b/media/audio/win/waveout_output_win.cc
index 0783cdb..f8446cb 100644
--- a/media/audio/win/waveout_output_win.cc
+++ b/media/audio/win/waveout_output_win.cc
@@ -15,18 +15,34 @@
 #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_EVENT mode in which XP signals events such as buffer
-//   releases.
-// - We use RegisterWaitForSingleObject() so one of threads in thread pool
-//   automatically calls our callback that feeds more data to Windows.
+// - We use CALLBACK_FUNCTION mode in which XP secretly creates two threads
+//   named _MixerCallbackThread and _waveThread which have real-time priority.
+//   The callbacks occur in _waveThread.
 // - Windows does not provide a way to query if the device is playing or paused
 //   thus it forces you to maintain state, which naturally is not exactly
 //   synchronized to the actual device state.
+// - Some functions, like waveOutReset cannot be called in the callback thread
+//   or called in any random state because they deadlock. This results in a
+//   non- instantaneous Stop() method. waveOutPrepareHeader seems to be in the
+//   same boat.
+// - waveOutReset() will forcefully kill the _waveThread so it is important
+//   to make sure we are not executing inside the audio source's OnMoreData()
+//   or that we take locks inside WaveCallback() or QueueNextPacket().
 
 // Sixty four MB is the maximum buffer size per AudioOutputStream.
 static const uint32 kMaxOpenBufferSize = 1024 * 1024 * 64;
 
+// Our sound buffers are allocated once and kept in a linked list using the
+// the WAVEHDR::dwUser variable. The last buffer points to the first buffer.
+static WAVEHDR* GetNextBuffer(WAVEHDR* current) {
+  return reinterpret_cast<WAVEHDR*>(current->dwUser);
+}
+
 // See Also
 // http://www.thx.com/consumer/home-entertainment/home-theater/surround-sound-speaker-set-up/
 // http://en.wikipedia.org/wiki/Surround_sound
@@ -63,18 +79,6 @@ static const unsigned int kChannelsToMask[kMaxChannelsToMask + 1] = {
   // TODO(fbarchard): Add additional masks for 7.2 and beyond.
 };
 
-inline size_t PCMWaveOutAudioOutputStream::BufferSize() const {
-  // Round size of buffer up to the nearest 16 bytes.
-  return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<size_t>(~15);
-}
-
-inline WAVEHDR* PCMWaveOutAudioOutputStream::GetBuffer(int n) const {
-  DCHECK_GE(n, 0);
-  DCHECK_LT(n, num_buffers_);
-  return reinterpret_cast<WAVEHDR*>(&buffers_[n * BufferSize()]);
-}
-
-
 PCMWaveOutAudioOutputStream::PCMWaveOutAudioOutputStream(
     AudioManagerWin* manager, const AudioParameters& params, int num_buffers,
     UINT device_id)
@@ -84,10 +88,13 @@ PCMWaveOutAudioOutputStream::PCMWaveOutAudioOutputStream(
       waveout_(NULL),
       callback_(NULL),
       num_buffers_(num_buffers),
+      buffer_(NULL),
       buffer_size_(params.GetPacketSize()),
       volume_(1),
       channels_(params.channels),
       pending_bytes_(0) {
+  ::InitializeCriticalSectionAndSpinCount(&lock_, kSpinCount);
+
   format_.Format.wFormatTag = WAVE_FORMAT_EXTENSIBLE;
   format_.Format.nChannels = params.channels;
   format_.Format.nSamplesPerSec = params.sample_rate;
@@ -109,35 +116,21 @@ PCMWaveOutAudioOutputStream::PCMWaveOutAudioOutputStream(
 
 PCMWaveOutAudioOutputStream::~PCMWaveOutAudioOutputStream() {
   DCHECK(NULL == waveout_);
+  ::DeleteCriticalSection(&lock_);
 }
 
 bool PCMWaveOutAudioOutputStream::Open() {
   if (state_ != PCMA_BRAND_NEW)
     return false;
-  if (BufferSize() * num_buffers_ > kMaxOpenBufferSize)
-    return false;
   if (num_buffers_ < 2 || num_buffers_ > 5)
     return false;
-
-  // Create buffer event.
-  buffer_event_.Set(::CreateEvent(NULL,   // Security attributes
-                                  FALSE,  // It will auto-reset
-                                  FALSE,  // Initial state
-                                  NULL    // No name
-                                  ));
-  if (!buffer_event_.Get()) {
-    return false;
-  }
-
-  // Open the device.
-  // We'll be getting buffer_event_ events when it's time to refill the buffer.
-  MMRESULT result = ::waveOutOpen(
-      &waveout_,
-      device_id_,
-      reinterpret_cast<LPCWAVEFORMATEX>(&format_),
-      reinterpret_cast<DWORD_PTR>(buffer_event_.Get()),
-      NULL,
-      CALLBACK_EVENT);
+  // Open the device. We'll be getting callback in WaveCallback function.
+  // They occur in a magic, time-critical thread that windows creates.
+  MMRESULT result = ::waveOutOpen(&waveout_, device_id_,
+                                  reinterpret_cast<LPCWAVEFORMATEX>(&format_),
+                                  reinterpret_cast<DWORD_PTR>(WaveCallback),
+                                  reinterpret_cast<DWORD_PTR>(this),
+                                  CALLBACK_FUNCTION);
   if (result != MMSYSERR_NOERROR)
     return false;
 
@@ -147,65 +140,60 @@ bool PCMWaveOutAudioOutputStream::Open() {
 }
 
 void PCMWaveOutAudioOutputStream::SetupBuffers() {
-  buffers_.reset(new char[BufferSize() * num_buffers_]);
+  WAVEHDR* last = NULL;
+  WAVEHDR* first = NULL;
   for (int ix = 0; ix != num_buffers_; ++ix) {
-    WAVEHDR* buffer = GetBuffer(ix);
-    buffer->lpData = reinterpret_cast<char*>(buffer) + sizeof(WAVEHDR);
-    buffer->dwBufferLength = buffer_size_;
-    buffer->dwBytesRecorded = 0;
-    buffer->dwFlags = WHDR_DONE;
-    buffer->dwLoops = 0;
+    uint32 sz = sizeof(WAVEHDR) + buffer_size_;
+    buffer_ =  reinterpret_cast<WAVEHDR*>(new char[sz]);
+    buffer_->lpData = reinterpret_cast<char*>(buffer_) + sizeof(WAVEHDR);
+    buffer_->dwBufferLength = buffer_size_;
+    buffer_->dwBytesRecorded = 0;
+    buffer_->dwUser = reinterpret_cast<DWORD_PTR>(last);
+    buffer_->dwFlags = WHDR_DONE;
+    buffer_->dwLoops = 0;
+    if (ix == 0)
+      first = buffer_;
+    last = buffer_;
     // Tell windows sound drivers about our buffers. Not documented what
     // this does but we can guess that causes the OS to keep a reference to
     // the memory pages so the driver can use them without worries.
-    ::waveOutPrepareHeader(waveout_, buffer, sizeof(WAVEHDR));
+    ::waveOutPrepareHeader(waveout_, buffer_, sizeof(WAVEHDR));
   }
+  // Fix the first buffer to point to the last one.
+  first->dwUser = reinterpret_cast<DWORD_PTR>(last);
 }
 
 void PCMWaveOutAudioOutputStream::FreeBuffers() {
+  WAVEHDR* current = buffer_;
   for (int ix = 0; ix != num_buffers_; ++ix) {
-    ::waveOutUnprepareHeader(waveout_, GetBuffer(ix), sizeof(WAVEHDR));
+    WAVEHDR* next = GetNextBuffer(current);
+    ::waveOutUnprepareHeader(waveout_, current, sizeof(WAVEHDR));
+    delete[] reinterpret_cast<char*>(current);
+    current = next;
   }
-  buffers_.reset(NULL);
+  buffer_ = NULL;
 }
 
-// Initially we ask the source to fill up all audio buffers. If we don't do
+// Initially we ask the source to fill up both audio buffers. If we don't do
 // this then we would always get the driver callback when it is about to run
 // samples and that would leave too little time to react.
 void PCMWaveOutAudioOutputStream::Start(AudioSourceCallback* callback) {
   if (state_ != PCMA_READY)
     return;
   callback_ = callback;
-
-  // Start watching for buffer events.
-  {
-    HANDLE waiting_handle = NULL;
-    ::RegisterWaitForSingleObject(&waiting_handle,
-                                  buffer_event_.Get(),
-                                  &BufferCallback,
-                                  static_cast<void*>(this),
-                                  INFINITE,
-                                  WT_EXECUTEDEFAULT);
-    if (!waiting_handle) {
-      HandleError(MMSYSERR_ERROR);
-      return;
-    }
-    waiting_handle_.Set(waiting_handle);
-  }
-
   state_ = PCMA_PLAYING;
-
-  // Queue the buffers.
   pending_bytes_ = 0;
+  WAVEHDR* buffer = buffer_;
   for (int ix = 0; ix != num_buffers_; ++ix) {
-    WAVEHDR* buffer = GetBuffer(ix);
     // Caller waits for 1st packet to become available, but not for others,
     // so we wait for them here.
     if (ix != 0)
       callback_->WaitTillDataReady();
     QueueNextPacket(buffer);  // Read more data.
     pending_bytes_ += buffer->dwBufferLength;
+    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,
@@ -221,11 +209,12 @@ void PCMWaveOutAudioOutputStream::Start(AudioSourceCallback* callback) {
   // 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) {
-    result = ::waveOutWrite(waveout_, GetBuffer(ix), sizeof(WAVEHDR));
+    result = ::waveOutWrite(waveout_, buffer, sizeof(WAVEHDR));
     if (result != MMSYSERR_NOERROR) {
       HandleError(result);
       break;
     }
+    buffer = GetNextBuffer(buffer);
   }
   result = ::waveOutRestart(waveout_);
   if (result != MMSYSERR_NOERROR) {
@@ -234,42 +223,25 @@ void PCMWaveOutAudioOutputStream::Start(AudioSourceCallback* callback) {
   }
 }
 
-// Stopping is tricky if we want it be fast.
-// For now just do it synchronously and avoid all the complexities.
-// TODO(enal): if we want faster Stop() we can create singleton that keeps track
-//             of all currently playing streams. Then you don't have to wait
-//             till all callbacks are completed. Of course access to singleton
-//             should be under its own lock, and checking the liveness and
-//             acquiring the lock on stream should be done atomically.
+// 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 after releasing all buffers.
 void PCMWaveOutAudioOutputStream::Stop() {
   if (state_ != PCMA_PLAYING)
     return;
-  state_ = PCMA_STOPPING;
-  MemoryBarrier();
 
-  // Stop playback.
+  // 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;
   }
 
-  // Stop watching for buffer event, wait till all the callbacks are complete.
-  BOOL unregister  = UnregisterWaitEx(waiting_handle_.Take(),
-                                      INVALID_HANDLE_VALUE);
-  if (!unregister) {
-    state_ = PCMA_PLAYING;
-    HandleError(MMSYSERR_ERROR);
-    return;
-  }
-
-  // waveOutReset() leaves buffers in the unpredictable state, causing
-  // problems if we want to release or reuse them. Fix the states.
-  for (int ix = 0; ix != num_buffers_; ++ix) {
-    GetBuffer(ix)->dwFlags = WHDR_PREPARED;
-  }
-
   // Don't use callback after Stop().
   callback_ = NULL;
 
@@ -279,8 +251,8 @@ void PCMWaveOutAudioOutputStream::Stop() {
 // We can Close in any state except that trying to close a stream that is
 // playing Windows generates an error, which we propagate to the source.
 void PCMWaveOutAudioOutputStream::Close() {
-  Stop();  // Just to be sure. No-op if not playing.
   if (waveout_) {
+    // waveOutClose generates a callback with WOM_CLOSE id in the same thread.
     MMRESULT res = ::waveOutClose(waveout_);
     if (res != MMSYSERR_NOERROR) {
       HandleError(res);
@@ -344,48 +316,40 @@ void PCMWaveOutAudioOutputStream::QueueNextPacket(WAVEHDR *buffer) {
   buffer->dwFlags = WHDR_PREPARED;
 }
 
-// One of the threads in our thread pool asynchronously calls this function when
-// buffer_event_ is signalled. Search through all the buffers looking for freed
-// ones, fills them with data, and "feed" the Windows.
-// Note: by searching through all the buffers we guarantee that we fill all the
-//       buffers, even when "event loss" happens, i.e. if Windows signals event
-//       when it did not flip into unsignaled state from the previous signal.
-void NTAPI PCMWaveOutAudioOutputStream::BufferCallback(PVOID lpParameter,
-                                                       BOOLEAN) {
-  TRACE_EVENT0("audio", "PCMWaveOutAudioOutputStream::BufferCallback");
-
-  PCMWaveOutAudioOutputStream* stream =
-      reinterpret_cast<PCMWaveOutAudioOutputStream*>(lpParameter);
-
-  // Lock the stream so 2 callbacks do not interfere with each other.
-  // Two callbacks can be called simultaneously by 2 different threads in the
-  // thread pool if one of the callbacks is slow, or system is very busy and
-  // one of scheduled callbacks is not called on time.
-  base::AutoLock auto_lock(stream->lock_);
-  if (stream->state_ != PCMA_PLAYING)
-    return;
+// Windows call us back in this function when some events happen. Most notably
+// when it is done playing a buffer. Since we use double buffering it is
+// convenient to think of |buffer| as free and GetNextBuffer(buffer) as in
+// use by the driver.
+void PCMWaveOutAudioOutputStream::WaveCallback(HWAVEOUT hwo, UINT msg,
+                                               DWORD_PTR instance,
+                                               DWORD_PTR param1, DWORD_PTR) {
+  TRACE_EVENT0("audio", "PCMWaveOutAudioOutputStream::WaveCallback");
+
+  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;
 
-  for (int ix = 0; ix != stream->num_buffers_; ++ix) {
-    WAVEHDR* buffer = stream->GetBuffer(ix);
-    if (buffer->dwFlags & WHDR_DONE) {
+    PCMWaveOutAudioOutputStream* stream =
+        reinterpret_cast<PCMWaveOutAudioOutputStream*>(instance);
+
+    // 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;
-      stream->QueueNextPacket(buffer);
 
-      // QueueNextPacket() can take a long time, especially if several of them
-      // were called back-to-back. Check if we are stopping now.
-      if (stream->state_ != PCMA_PLAYING)
-        return;
+      stream->QueueNextPacket(buffer);
 
       // 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(stream->waveout_,
-                                       buffer,
-                                       sizeof(WAVEHDR));
+      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 3c0f89a..10ed073 100644
--- a/media/audio/win/waveout_output_win.h
+++ b/media/audio/win/waveout_output_win.h
@@ -11,8 +11,6 @@
 #include <mmreg.h>
 
 #include "base/basictypes.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/synchronization/lock.h"
 #include "base/win/scoped_handle.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_parameters.h"
@@ -30,9 +28,9 @@ class AudioManagerWin;
 // you are keeping state in it.
 class PCMWaveOutAudioOutputStream : public AudioOutputStream {
  public:
-  // The ctor takes all the usual parameters, plus |manager| which is the the
-  // audio manager who is creating this object and |device_id| which is provided
-  // by the operating system.
+  // The ctor takes all the usual parameters, plus |manager| which is the
+  // the audio manager who is creating this object and |device_id| which
+  // is provided by the operating system.
   PCMWaveOutAudioOutputStream(AudioManagerWin* manager,
                               const AudioParameters& params,
                               int num_buffers,
@@ -55,27 +53,20 @@ class PCMWaveOutAudioOutputStream : public AudioOutputStream {
     PCMA_BRAND_NEW,    // Initial state.
     PCMA_READY,        // Device obtained and ready to play.
     PCMA_PLAYING,      // Playing audio.
-    PCMA_STOPPING,     // Audio is stopping, do not "feed" data to Windows.
     PCMA_CLOSED        // Device has been released.
   };
 
-  // Returns pointer to the n-th buffer.
-  inline WAVEHDR* GetBuffer(int n) const;
-
-  // Size of one buffer in bytes, rounded up if necessary.
-  inline size_t BufferSize() const;
-
-  // Windows calls us back asking for more data when buffer_event_ signalled.
-  // See MSDN for help on RegisterWaitForSingleObject() and waveOutOpen().
-  static void NTAPI BufferCallback(PVOID lpParameter, BOOLEAN);
+  // Windows calls us back to feed more data to the device here. See msdn
+  // documentation for 'waveOutProc' for details about the parameters.
+  static void CALLBACK WaveCallback(HWAVEOUT hwo, UINT msg, DWORD_PTR instance,
+                                    DWORD_PTR param1, DWORD_PTR param2);
 
   // If windows reports an error this function handles it and passes it to
   // the attached AudioSourceCallback::OnError().
   void HandleError(MMRESULT error);
-
-  // Allocates and prepares the memory that will be used for playback.
+  // Allocates and prepares the memory that will be used for playback. Only
+  // two buffers are created.
   void SetupBuffers();
-
   // Deallocates the memory allocated in SetupBuffers.
   void FreeBuffers();
 
@@ -116,18 +107,18 @@ class PCMWaveOutAudioOutputStream : public AudioOutputStream {
   // Handle to the instance of the wave device.
   HWAVEOUT waveout_;
 
-  // Handle to the buffer event.
-  base::win::ScopedHandle buffer_event_;
-
-  // Handle returned by RegisterWaitForSingleObject().
-  base::win::ScopedHandle waiting_handle_;
-
-  // Pointer to the allocated audio buffers, we allocate all buffers in one big
-  // chunk. This object owns them.
-  scoped_array<char> buffers_;
-
-  // Lock used to avoid the conflict when callbacks are called simultaneously.
-  base::Lock lock_;
+  // Pointer to the first allocated audio buffer. This object owns it.
+  WAVEHDR* buffer_;
+
+  // 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);
 };