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

media/audio/win/waveout_output_win.h

Index: media/audio/win/waveout_output_win.h
===================================================================
--- media/audio/win/waveout_output_win.h	(revision 110309)
+++ media/audio/win/waveout_output_win.h	(working copy)
@@ -6,11 +6,16 @@
 #define MEDIA_AUDIO_WIN_WAVEOUT_OUTPUT_WIN_H_
 #pragma once
 
+#include <map>

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

 os includes first

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

Done.

+
 #include <windows.h>
 #include <mmsystem.h>
 #include <mmreg.h>
 
 #include "base/basictypes.h"
+#include "base/logging.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"
@@ -29,8 +34,9 @@
 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 audio manager who is creating this object and on which message loop
+  // "feeder" callbacks are running, and |device_id| which is provided by the
+   // operating system.
   PCMWaveOutAudioOutputStream(AudioManagerWin* manager,
                               const AudioParameters& params,
                               int num_buffers,
@@ -49,27 +55,73 @@
   void QueueNextPacket(WAVEHDR* buffer);
 
  private:
+  // Returns pointer to the n-th buffer.
+  WAVEHDR* GetBuffer(int n) const {

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

nice.  you could move this to the cc file to avoid including logging.h in the
header.

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

Done.

+    DCHECK_GE(n, 0);
+    DCHECK_LT(n, num_buffers_);
+    return reinterpret_cast<WAVEHDR*>(buffers_ + n * CbBuffer());
+  }
+
   enum State {
     PCMA_BRAND_NEW,    // Initial state.
     PCMA_READY,        // Device obtained and ready to play.
     PCMA_PLAYING,      // Playing audio.
+    PCMA_STOPPING,     // Stopping the audio.
     PCMA_CLOSED        // Device has been released.
   };
 
-  // Windows calls us back to feed more data to the device here. See msdn
-  // documentation for 'waveOutProc' for details about the parameters.
+  // Size of one buffer in bytes, rounded up to the nearest 16 bytes.
+  DWORD CbBuffer() const {

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

CbBuffer -> BufferSize
Also, return size_t?

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

Done.

+    return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<DWORD>(-16);

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

instead of static_cast<DWORD>(-16), can you just use 0xFFFFFFF0?  imho that's
more readable.

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

In general, 0xFFFFFFF0 is bad -- it can become a problem on 64-bit platform. You
are right, in code as it was 0xFFFFFFF0 is probably better, but not anymore
after I switch to size_t.

+  }
+
+  // Windows calls us back to free the buffer. 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);
 
+  // Feed the buffer to waveOut.
+  // Called on the audio manager thread.
+  static void FeedBuffer(PCMWaveOutAudioOutputStream* stream,
+                         WAVEHDR* buffer,
+                         HWAVEOUT hwo,
+                         DWORD ordinal);
+
   // 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. Only
-  // two buffers are created.
+  // Allocates and prepares the memory that will be used for playback.

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

add an empty line above this one.  same throughout.  Generally when functions or
member variables have a comment, we have have a single empty line above the
comment for separation.

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

Done.

   void SetupBuffers();
   // Deallocates the memory allocated in SetupBuffers.
   void FreeBuffers();
+  // Globals-related functions.

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

This comment isn't very useful.  Can you document which globals we need instead?

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

Done.

+  static void InitializeGlobalsIfNecessary();
+  static void CleanupGlobals(void* not_used);
 
+  // Static lock guarding access to globals.
+  // We are not doing any expensive operations under this lock --
+  // adding/searching/removing object to/in/from map of all live objects, and
+  // trying to acquire object lock, so hopefully we can live with one global
+  // lock for all audio streams. If contention ever becomes the problem we
+  // can use hash of stream address to use one of several global locks.
+  static base::Lock g_lock_;

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

We should avoid global objects that require construction.  If we absolutely must
have global state, we should group it all in a separate class and use one of the
singleton classes from base/

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

Done.

+
+  // When playing, every instance of our audio stream has associated ordinal.
+  // Callback that feeds the audio thread verifies that stream is still alive
+  // by looking stream address and its ordinal in the map of all streams.

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

by looking stream address -> by looking at the stream address

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

Done.

+  // Stream is not touched before  check is complete. This way we don't

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

"Stream is not touched before  check is complete" ->
"The stream is not be touched before the check is complete."

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

Done.

+  // have to wait for all scheduled callbacks to finish when stopping audio
+  // stream. Ordinal is needed because audio stream can be deleted and new one
+  // with same address created.
+  // Ordinal overflow would be a problem only if some scheduled feeder callback
+  // would not run before 4.2*10**9 play() calls are executed, impossible for
+  // now...
+  static volatile DWORD g_ordinal_;
+
+  // Map of all live streams with their ordinals.
+  static scoped_ptr<std::map<PCMWaveOutAudioOutputStream*, DWORD> >
+      g_streams_map_;
+
   // Reader beware. Visual C has stronger guarantees on volatile vars than
   // most people expect. In fact, it has release semantics on write and
   // acquire semantics on reads. See the msdn documentation.
@@ -107,19 +159,17 @@
   // Handle to the instance of the wave device.
   HWAVEOUT waveout_;
 
-  // Pointer to the first allocated audio buffer. This object owns it.
-  WAVEHDR* buffer_;
+  // Pointer to the allocated audio buffers, we allocate all buffers in one big
+  // chunk. Stream owns them.

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

I prefer "This object owns it" to "Stream owns them".

+  char* buffers_;
 
-  // 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_;
+  // Lock used to avoid the conflict between filling audio buffers and
+  // stopping the stream.
+  base::Lock lock_;
 
+  // Ordinal of current stream when playing.
+  DWORD ordinal_;
+
   DISALLOW_COPY_AND_ASSIGN(PCMWaveOutAudioOutputStream);
 };