Change the way we are sending audio data to driver when using WaveOut API.

media/audio/win/waveout_output_win.cc

Index: media/audio/win/waveout_output_win.cc
===================================================================
--- media/audio/win/waveout_output_win.cc	(revision 110309)
+++ media/audio/win/waveout_output_win.cc	(working copy)
@@ -8,16 +8,19 @@
 #include <mmsystem.h>
 #pragma comment(lib, "winmm.lib")
 
+#include "base/at_exit.h"
 #include "base/basictypes.h"
+#include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "media/audio/audio_io.h"
 #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;
+base::Lock PCMWaveOutAudioOutputStream::g_lock_;
+volatile DWORD PCMWaveOutAudioOutputStream::g_ordinal_;
+scoped_ptr<std::map<PCMWaveOutAudioOutputStream*, DWORD> >
+    PCMWaveOutAudioOutputStream::g_streams_map_;

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

All these objects should belong to one singleton object - assuming we cannot
live without a singleton.

[enal1, 2011/11/19 00:59:19]

Done.

 
 // Some general thoughts about the waveOut API which is badly documented :
 // - We use CALLBACK_FUNCTION mode in which XP secretly creates two threads
@@ -26,23 +29,14 @@
 // - 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
+// - 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().
+//   non- instantaneous Stop() method. waveOutWrite() and waveOutPrepareHeader
+//   seem to be in the same boat.
+// - We have to use separate "feeder" thread that calls waveOutWrite() to feed
+//   buffers to driver, cannot do it from the callback.
+// - Stopping is tricky, see comment below.
 
-// 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
@@ -88,12 +82,13 @@
       waveout_(NULL),
       callback_(NULL),
       num_buffers_(num_buffers),
-      buffer_(NULL),
+      buffers_(NULL),
       buffer_size_(params.GetPacketSize()),
       volume_(1),
       channels_(params.channels),
-      pending_bytes_(0) {
-  ::InitializeCriticalSectionAndSpinCount(&lock_, kSpinCount);
+      pending_bytes_(0),
+      ordinal_(0) {
+  InitializeGlobalsIfNecessary();
 
   format_.Format.wFormatTag = WAVE_FORMAT_EXTENSIBLE;
   format_.Format.nChannels = params.channels;
@@ -116,7 +111,6 @@
 
 PCMWaveOutAudioOutputStream::~PCMWaveOutAudioOutputStream() {
   DCHECK(NULL == waveout_);
-  ::DeleteCriticalSection(&lock_);
 }
 
 bool PCMWaveOutAudioOutputStream::Open() {
@@ -140,41 +134,30 @@
 }
 
 void PCMWaveOutAudioOutputStream::SetupBuffers() {
-  WAVEHDR* last = NULL;
-  WAVEHDR* first = NULL;
+  buffers_ =  new char[CbBuffer() * num_buffers_];
   for (int ix = 0; ix != num_buffers_; ++ix) {
-    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_;
+    WAVEHDR *buffer = GetBuffer(ix);
+    buffer->lpData = reinterpret_cast<char*>(buffer) + sizeof(WAVEHDR);
+    buffer->dwBufferLength = buffer_size_;
+    buffer->dwBytesRecorded = 0;
+    buffer->dwUser = 0;
+    buffer->dwFlags = WHDR_DONE;
+    buffer->dwLoops = 0;
     // 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) {
-    WAVEHDR* next = GetNextBuffer(current);
-    ::waveOutUnprepareHeader(waveout_, current, sizeof(WAVEHDR));
-    delete[] reinterpret_cast<char*>(current);
-    current = next;
+    ::waveOutUnprepareHeader(waveout_, GetBuffer(ix), sizeof(WAVEHDR));
   }
-  buffer_ = NULL;
+  delete[] buffers_;

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

use scoped_array?

[enal1, 2011/11/19 00:59:19]

Done.

 }
 
-// Initially we ask the source to fill up both audio buffers. If we don't do
+// Initially we ask the source to fill up all 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) {
@@ -182,18 +165,33 @@
     return;
   callback_ = callback;
   state_ = PCMA_PLAYING;
+
+  // Place "this" into map. Do it under global lock.
+  {
+    base::AutoLock auto_lock(g_lock_);
+    ordinal_ = g_ordinal_;
+    ++g_ordinal_;
+    DCHECK(g_streams_map_->find(this) == g_streams_map_->end());
+    (*g_streams_map_)[this] = ordinal_;
+  }
+
+  // Queue the buffers.
+  // TODO(enal): If there are more than 2, queue only first 2 and schedule
+  // remaining to be filled in the "feeder" thread. Non-trivial because we have
+  // to be sure that data is ready. Can be done by storing time of last
+  // OnMoreData() call and  scheduling delayed task in FeedBuffer() if data is
+  // not ready yet.
   pending_bytes_ = 0;
-  WAVEHDR* buffer = buffer_;
   for (int ix = 0; ix != num_buffers_; ++ix) {
+    WAVEHDR* buffer = GetBuffer(ix);
+    buffer->dwUser = ordinal_;
     // 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,
@@ -209,12 +207,11 @@
   // 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_, buffer, sizeof(WAVEHDR));
+    result = ::waveOutWrite(waveout_, GetBuffer(ix), sizeof(WAVEHDR));
     if (result != MMSYSERR_NOERROR) {
       HandleError(result);
       break;
     }
-    buffer = GetNextBuffer(buffer);
   }
   result = ::waveOutRestart(waveout_);
   if (result != MMSYSERR_NOERROR) {
@@ -223,28 +220,45 @@
   }
 }
 
-// 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.
+// Stopping is tricky. We want to avoid stopping while feeder thread feeds
+// buffer to driver, so
+// * Here:
+//    (a) get object lock

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

why do we need the object lock while we hold the other lock?
The state_ variable (which isn't protected, hence I'm assuming single
threadedness for these functions) should make sure that Start() isn't called
while we're stopping.

[enal1, 2011/11/19 00:59:19]

Got rid of other lock. Done.

+//    (b) get global lock
+//    (c) remove object from map
+//    (d) release both locks
+//    ...do real stopping...
+// * In feeder thread:
+//    (e) get global lock
+//    (f) if object is not in the map, exit
+//    (g) try getting object lock; if fail, exit
+//    (h) release global lock
+//    ...do real feeding...
+// There is no way for object to be alive in (f) but stopped/deleted in (g).
+// For object to be stopped between (f) and (g), Stop() should acquire global
+// lock, but it is already acquired by feeder. So it is safe to touch object
+// lock at (g)...
 void PCMWaveOutAudioOutputStream::Stop() {
   if (state_ != PCMA_PLAYING)
     return;
+  state_ = PCMA_STOPPING;
+  MMRESULT res = MMSYSERR_NOERROR;
+  {
+    base::AutoLock auto_lock(lock_);
 
-  // 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_);
+    // Remove us from the map.
+    {
+      base::AutoLock auto_lock(g_lock_);
+      g_streams_map_->erase(this);
+    }
+    res = ::waveOutReset(waveout_);
+  }
   if (res != MMSYSERR_NOERROR) {
     HandleError(res);
     return;
   }

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

it's ok to keep the empty line below this one imo.  same for the scope above for
separation.

[enal1, 2011/11/19 00:59:19]

Done.

-
   // Don't use callback after Stop().
   callback_ = NULL;
-
   state_ = PCMA_READY;
 }
 
@@ -316,40 +330,89 @@
   buffer->dwFlags = WHDR_PREPARED;
 }
 
+void PCMWaveOutAudioOutputStream::FeedBuffer(
+    PCMWaveOutAudioOutputStream* stream,
+    WAVEHDR* buffer,
+    HWAVEOUT hwo,
+    DWORD ordinal) {
+  TRACE_EVENT0("audio", "PCMWaveOutAudioOutputStream::FeedBuffer");
+  // Is stream still alive and playing? Check is tricky.
+  // See comment in PCMWaveOutAudioOutputStream::Stop() explaining what
+  // exactly happens.

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

Are we doing this check to avoid touching an object that might be deleted?

If not, then could we move all of this function into a non-static member
function of PCMWaveOutAudioOutputStream and skip all stream-> parts?

[enal1, 2011/11/19 00:59:19]

Yes, we had very complicated checking to avoid touching potentially deleted
object. Not necessary with aux object anymore.

+  bool stopped = true;
+  {
+    base::AutoLock auto_lock(g_lock_);
+    std::map<PCMWaveOutAudioOutputStream*, DWORD>::iterator it =
+        g_streams_map_->find(stream);
+    stopped = (it == g_streams_map_->end() ||
+               it->second != ordinal ||
+               !stream->lock_.Try());

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

Maybe add a comment that here is where you acquire the object's lock.  I missed
it the first time around.

[enal1, 2011/11/19 00:59:19]

 Code totally rewritten.

+  }
+  if (stopped)

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

what about checking the state_ variable for != PCMA_PLAYING?

[enal1, 2011/11/19 00:59:19]

Again, could not do that because object could be deleted. Not necessary anymore.

+    return;
+
+  // 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);
+
+  // 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;
+  stream->lock_.Release();
+}
+
 // 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.
+// when it is done playing a buffer. Cannot feed freed buffer back to the system
+// from here, have to schedule it for the separate therad.

[tommi (sloooow) - chröme, 2011/11/18 09:53:21]

therad -> thread

[enal1, 2011/11/19 00:59:19]

Done.

 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.
+    // WOM_DONE indicates that the driver is done with our buffer,
+    // if still playing ask "feedef" thread to buffer more data.
     WAVEHDR* buffer = reinterpret_cast<WAVEHDR*>(param1);
     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);
-
-      // 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_);
+    if (stream->state_ == PCMA_PLAYING) {
+      stream->manager_->GetMessageLoop()->PostTask(
+          FROM_HERE,
+          base::Bind(&PCMWaveOutAudioOutputStream::FeedBuffer,
+                     stream,
+                     buffer,
+                     hwo,
+                     buffer->dwUser));
     }
   }
 }
+// Initialize globals, if necessary. Use double-checked locking,
+// it is safe on Windows as long as you are checking volatile variable.
+// static
+void PCMWaveOutAudioOutputStream::InitializeGlobalsIfNecessary() {
+  if (g_ordinal_ == 0) {
+    base::AutoLock auto_lock(g_lock_);
+    if (g_ordinal_ == 0) {
+      g_streams_map_.reset(new std::map<PCMWaveOutAudioOutputStream*, DWORD>);
+      base::AtExitManager::RegisterCallback(
+          &PCMWaveOutAudioOutputStream::CleanupGlobals, NULL);
+      // Volatile stores have release semantics, no separate memory
+      // barrier necessary.
+      g_ordinal_ = 1;
+    }
+  }
+}
+
+// static
+void PCMWaveOutAudioOutputStream::CleanupGlobals(void* not_used) {
+  g_streams_map_.reset(NULL);
+}