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 110574)
+++ media/audio/win/waveout_output_win.cc	(working copy)
@@ -9,16 +9,13 @@
 #pragma comment(lib, "winmm.lib")
 
 #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;
-
 // 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.
@@ -26,23 +23,13 @@
 // - 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.
 
-// 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
@@ -79,6 +66,18 @@
   // 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);

[tommi (sloooow) - chröme, 2011/11/19 17:23:27]

nit: instead of 15 I find hex better to read for bit operations, so 0xF.  using
~ is a nice touch.

+}
+
+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)
@@ -88,13 +87,10 @@
       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;
@@ -116,7 +112,6 @@
 
 PCMWaveOutAudioOutputStream::~PCMWaveOutAudioOutputStream() {
   DCHECK(NULL == waveout_);
-  ::DeleteCriticalSection(&lock_);
 }
 
 bool PCMWaveOutAudioOutputStream::Open() {
@@ -140,41 +135,29 @@
 }
 
 void PCMWaveOutAudioOutputStream::SetupBuffers() {
-  WAVEHDR* last = NULL;
-  WAVEHDR* first = NULL;
+  buffers_.reset(new char[BufferSize() * 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->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;
+  buffers_.reset(NULL);
 }
 
-// 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,25 @@
     return;
   callback_ = callback;
   state_ = PCMA_PLAYING;
+
+  playing_object_ = new PlayingObject(this);
+
+  // 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);
     // 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 +199,13 @@
   // 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);
+    // Each successfully sent buffer references |playing_object_|.
+    playing_object_->AddRef();
   }
   result = ::waveOutRestart(waveout_);
   if (result != MMSYSERR_NOERROR) {
@@ -223,25 +214,25 @@
   }
 }
 
-// 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 use the lock on the |playing_object_|.
 void PCMWaveOutAudioOutputStream::Stop() {
   if (state_ != PCMA_PLAYING)
     return;
-
-  // 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_);
+  MMRESULT res = MMSYSERR_NOERROR;
+  {
+    base::AutoLock auto_lock(*playing_object_->lock());
+    playing_object_->stop_playing();
+    MemoryBarrier();
+    res = ::waveOutReset(waveout_);
+  }
   if (res != MMSYSERR_NOERROR) {
     HandleError(res);
     return;
   }
 
+  playing_object_ = NULL;

[tommi (sloooow) - chröme, 2011/11/19 17:23:27]

While this correctly releases the object, it also sets the pointer to NULL.  So
if there are pending callbacks, they will crash when trying to Release the
reference for those pending callbacks.  So, the callbacks basically can't use
the same member variable to access the playing_object_ as the stream object
uses.

Instead of sending a pointer to the stream object to the callback, can we send a
pointer to the playing object directly?
Perhaps it would also be a good idea to let the playing object have a pointer to
the 'manager' and own the buffer in case the thread that calls Stop() also
deletes the stream object straight away or calls Start() again, in which case a
new playing object would be used.  Basically I think it would be good if the
playing object doesn't have a pointer back to the stream.

+
   // Don't use callback after Stop().
   callback_ = NULL;
 
@@ -317,39 +308,66 @@
 }
 
 // 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 thread.

[cpu_(ooo_6.6-7.5), 2011/11/21 01:18:43]

I don't agree with this approach. We have added another thread to this
operation, the one calling this callback is already a real-time thread.

The right approach is to use CALLBACK_EVENT mode, and remove this overhead.

[tommi (sloooow) - chröme, 2011/11/22 08:21:45]

Hey Carlos - If we use the event model, won't the runtime properties be similar
to this approach?  Presumably we would be waiting for the event on our own
thread and the event signalled on another.  In both cases there are two threads
in the picture although in one case the signaling thread doesn't enter our
code... right?
Or is what we're saving, several context switches?  I'm guessing, switch from
kernel to user for callback and back, then later to the mq thread to process the
message?

 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;
+    if (stream->playing_object_->playing()) {
+      stream->manager_->GetMessageLoop()->PostTask(
+          FROM_HERE,
+          base::Bind(
+              &PCMWaveOutAudioOutputStream::PlayingObject::FeedBuffer,
+              stream->playing_object_.get(),
+              buffer,
+              hwo));
+    }
+    stream->playing_object_->Release();
+  }
+}
 
-      stream->QueueNextPacket(buffer);
+PCMWaveOutAudioOutputStream::PlayingObject::PlayingObject(
+    PCMWaveOutAudioOutputStream *stream) :
+    stream_(stream) {
+  DCHECK(stream_ != NULL);
+}
 
-      // 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);
+PCMWaveOutAudioOutputStream::PlayingObject::~PlayingObject() {
+  DCHECK(stream_ == NULL);
+}
 
-      stream->pending_bytes_ += buffer->dwBufferLength;
-      ::LeaveCriticalSection(&stream->lock_);
-    }
-  }
+void PCMWaveOutAudioOutputStream::PlayingObject::FeedBuffer(WAVEHDR* buffer,
+                                                            HWAVEOUT hwo) {
+  TRACE_EVENT0("audio",
+               "PCMWaveOutAudioOutputStream::PlayingObject::FeedBuffer");
+  // Try to obtain the lock. Minor optimization: if lock is owned, return
+  // immediately, not wait till PCMWaveOutAudioOutputStream::Stop() release
+  // it and we will return anyways.
+  if (!lock_.Try())

[cpu_(ooo_6.6-7.5), 2011/11/21 01:18:43]

I don't completely follow why we need to take this lock anymore.

+    return;
+  if (!playing())
+    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);
+  AddRef();

[tommi (sloooow) - chröme, 2011/11/19 17:23:27]

maybe a comment on where this reference is released.

[tommi (sloooow) - chröme, 2011/11/19 21:58:42]

Actually, I think this AddRef should not be bere.  It should be where you call
PostTask.  The reason is that we must guarantee that the object lives while a
pointer to it lives in the message queue.

Alternatively, and even simpler, you can skip calling Release inside
WaveCallback when playing:

if (playing_object->playing()) {
  playing_object->message_loop()->PostTask(...);
  // no Release here.
} else {
  playing_object->Release();
}

+  stream_->pending_bytes_ += buffer->dwBufferLength;
+  lock_.Release();
 }