Bug 1127213 - Fix various issues with the device change notification in the WASAPI cubeb backend. r=kinetik, a=sledru
authorPaul Adenot <paul@paul.cx>
Mon, 09 Feb 2015 14:42:43 +0100
changeset 243735 9579b9ab68ca
parent 243734 1673712f9408
child 243736 9a36ec122aa5
push id4456
push userryanvm@gmail.com
push date2015-02-09 21:51 +0000
treeherdermozilla-beta@1584db7257a6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskinetik, sledru
bugs1127213
milestone36.0
Bug 1127213 - Fix various issues with the device change notification in the WASAPI cubeb backend. r=kinetik, a=sledru This patch does the following: - Introduces an owned_critical_section object to be able to assert that a current thread owns a critical section - Change the auto_lock to use the above, add auto_unlock (basically like the Gecko AutoEnter/AutoExit things) - Fix an issue during audio output device switch where the clock would use the old sample rate. Apparently I did not notice this because my headset and the sound card on this laptop have the same rate - Check that we could acquire a device_enumerator in the ctor before deallocating in the dtor, as that can happen if a ton of streams are running at once (I had this issue running the full mochitest suite) - Stop getting another device_enumator in unregister_notification_client, fixing a leak - Assert that setup_wasapi_stream and close_wasapi_stream are called with the lock held, this was the cause of the crash for this bug - Make close_wasapi_stream clear out its state to make sure setup_wasapi_stream and close_wasapi_stream are called in the right order (especially, not two setup_wasapi_stream without close in between, since that would leak stuff) - In wasapi_stream_destroy, unregister the notification client before destroying the CRITICAL_SECTION (this was the cause of a crash I duped against this bug)
media/libcubeb/src/cubeb_wasapi.cpp
--- a/media/libcubeb/src/cubeb_wasapi.cpp
+++ b/media/libcubeb/src/cubeb_wasapi.cpp
@@ -1,15 +1,20 @@
 /*
  * Copyright  2013 Mozilla Foundation
  *
  * This program is made available under an ISC-style license.  See the
  * accompanying file LICENSE for details.
  */
+// This enables assert in release, and lets us have debug-only code
+#ifdef NDEBUG
+#define DEBUG
 #undef NDEBUG
+#endif // #ifdef NDEBUG
+
 #if defined(HAVE_CONFIG_H)
 #include "config.h"
 #endif
 #include <assert.h>
 #include <windows.h>
 #include <mmdeviceapi.h>
 #include <windef.h>
 #include <audioclient.h>
@@ -70,28 +75,96 @@ SafeRelease(HANDLE handle)
 template <typename T>
 void SafeRelease(T * ptr)
 {
   if (ptr) {
     ptr->Release();
   }
 }
 
+/* This wraps a critical section to track the owner in debug mode, adapted from
+ * NSPR and http://blogs.msdn.com/b/oldnewthing/archive/2013/07/12/10433554.aspx */
+class owned_critical_section
+{
+public:
+
+  owned_critical_section()
+#ifdef DEBUG
+    : owner(0)
+#endif
+  {
+    InitializeCriticalSection(&critical_section);
+  }
+
+  ~owned_critical_section()
+  {
+    DeleteCriticalSection(&critical_section);
+  }
+
+  void enter()
+  {
+    EnterCriticalSection(&critical_section);
+#ifdef DEBUG
+    assert(owner != GetCurrentThreadId() && "recursive locking");
+    owner = GetCurrentThreadId();
+#endif
+  }
+
+  void leave()
+  {
+#ifdef DEBUG
+    /* GetCurrentThreadId cannot return 0: it is not a the valid thread id */
+    owner = 0;
+#endif
+    LeaveCriticalSection(&critical_section);
+  }
+
+#ifdef DEBUG
+  /* This is guaranteed to have the good behaviour if it succeeds. The behaviour
+   * is undefined otherwise. */
+  void assert_current_thread_owns()
+  {
+    /* This implies owner != 0, because GetCurrentThreadId cannot return 0. */
+    assert(owner == GetCurrentThreadId());
+  }
+#endif
+
+private:
+  CRITICAL_SECTION critical_section;
+#ifdef DEBUG
+  DWORD owner;
+#endif
+};
+
 struct auto_lock {
-  auto_lock(CRITICAL_SECTION * lock)
+  auto_lock(owned_critical_section * lock)
     :lock(lock)
   {
-    EnterCriticalSection(lock);
+    lock->enter();
   }
   ~auto_lock()
   {
-    LeaveCriticalSection(lock);
+    lock->leave();
   }
 private:
-  CRITICAL_SECTION * lock;
+  owned_critical_section * lock;
+};
+
+struct auto_unlock {
+  auto_unlock(owned_critical_section * lock)
+    :lock(lock)
+  {
+    lock->leave();
+  }
+  ~auto_unlock()
+  {
+    lock->enter();
+  }
+private:
+  owned_critical_section * lock;
 };
 
 struct auto_com {
   auto_com()
   : need_uninit(true) {
     HRESULT hr = CoInitializeEx(NULL, COINIT_MULTITHREADED);
     // This is for information purposes only, in anycase, COM is initialized
     // at the end of the constructor.
@@ -181,17 +254,17 @@ struct cubeb_stream
    * function, so the render loop can exit properly. */
   HANDLE shutdown_event;
   /* This is set by WASAPI when we should refill the stream. */
   HANDLE refill_event;
   /* Each cubeb_stream has its own thread. */
   HANDLE thread;
   /* We synthetize our clock from the callbacks. */
   LONG64 clock;
-  CRITICAL_SECTION stream_reset_lock;
+  owned_critical_section * stream_reset_lock;
   /* Maximum number of frames we can be requested in a callback. */
   uint32_t buffer_frame_count;
   /* Resampler instance. Resampling will only happen if necessary. */
   cubeb_resampler * resampler;
   /* Buffer used to downmix or upmix to the number of channels the mixer has.
    * its size is |frames_to_bytes_before_mix(buffer_frame_count)|. */
   float * mix_buffer;
   /* True if the stream is draining. */
@@ -252,17 +325,17 @@ public:
       return S_OK;
     }
 
     auto_com autocom;
 
     /* Close the stream */
     wasapi_stream_stop(stm);
     {
-      auto_lock lock(&stm->stream_reset_lock);
+      auto_lock lock(stm->stream_reset_lock);
       close_wasapi_stream(stm);
       /* Reopen a stream and start it immediately. This will automatically pick the
        * new default device for this role. */
       setup_wasapi_stream(stm);
     }
     wasapi_stream_start(stm);
 
     return S_OK;
@@ -309,16 +382,22 @@ bool should_upmix(cubeb_stream * stream)
   return stream->mix_params.channels > stream->stream_params.channels;
 }
 
 bool should_downmix(cubeb_stream * stream)
 {
   return stream->mix_params.channels < stream->stream_params.channels;
 }
 
+float stream_to_mix_samplerate_ratio(cubeb_stream * stream)
+{
+  auto_lock lock(stream->stream_reset_lock);
+  return float(stream->stream_params.rate) / stream->mix_params.rate;
+}
+
 /* Upmix function, copies a mono channel in two interleaved
  * stereo channel. |out| has to be twice as long as |in| */
 template<typename T>
 void
 mono_to_stereo(T * in, long insamples, T * out)
 {
   int j = 0;
   for (int i = 0; i < insamples; ++i, j += 2) {
@@ -382,19 +461,19 @@ refill(cubeb_stream * stm, float * data,
    * avoid a copy. */
   float * dest;
   if (should_upmix(stm) || should_downmix(stm)) {
     dest = stm->mix_buffer;
   } else {
     dest = data;
   }
 
-  stm->clock = InterlockedAdd64(&stm->clock, frames_needed);
+  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
 
-  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
+  stm->clock = InterlockedAdd64(&stm->clock, frames_needed * stream_to_mix_samplerate_ratio(stm));
 
   /* XXX: Handle this error. */
   if (out_frames < 0) {
     assert(false);
   }
 
   /* Go in draining mode if we got fewer frames than requested. */
   if (out_frames < frames_needed) {
@@ -540,23 +619,20 @@ HRESULT register_notification_client(cub
     return hr;
   }
 
   return hr;
 }
 
 HRESULT unregister_notification_client(cubeb_stream * stm)
 {
-  HRESULT hr = CoCreateInstance(__uuidof(MMDeviceEnumerator),
-                                NULL, CLSCTX_INPROC_SERVER,
-                                IID_PPV_ARGS(&stm->device_enumerator));
+  assert(stm);
 
-  if (FAILED(hr)) {
-    LOG("Could not get device enumerator: %x", hr);
-    return hr;
+  if (!stm->device_enumerator) {
+    return S_OK;
   }
 
   stm->device_enumerator->UnregisterEndpointNotificationCallback(stm->notification_client);
 
   SafeRelease(stm->notification_client);
   SafeRelease(stm->device_enumerator);
 
   return S_OK;
@@ -842,18 +918,22 @@ handle_channel_layout(cubeb_stream * stm
 }
 
 int setup_wasapi_stream(cubeb_stream * stm)
 {
   HRESULT hr;
   IMMDevice * device;
   WAVEFORMATEX * mix_format;
 
+  stm->stream_reset_lock->assert_current_thread_owns();
+
   auto_com com;
 
+  assert(!stm->client && "WASAPI stream already setup, close it first.");
+
   hr = get_default_endpoint(&device);
   if (FAILED(hr)) {
     LOG("Could not get default endpoint, error: %x", hr);
     wasapi_stream_destroy(stm);
     return CUBEB_ERROR;
   }
 
   /* Get a client. We will get all other interfaces we need from
@@ -973,17 +1053,26 @@ wasapi_stream_init(cubeb * context, cube
   stm->context = context;
   stm->data_callback = data_callback;
   stm->state_callback = state_callback;
   stm->user_ptr = user_ptr;
   stm->stream_params = stream_params;
   stm->draining = false;
   stm->latency = latency;
   stm->clock = 0;
-  InitializeCriticalSection(&stm->stream_reset_lock);
+
+  /* Null out WASAPI-specific state */
+  stm->resampler = nullptr;
+  stm->client = nullptr;
+  stm->render_client = nullptr;
+  stm->audio_stream_volume = nullptr;
+  stm->device_enumerator = nullptr;
+  stm->notification_client = nullptr;
+
+  stm->stream_reset_lock = new owned_critical_section();
 
   stm->shutdown_event = CreateEvent(NULL, 0, 0, NULL);
   stm->refill_event = CreateEvent(NULL, 0, 0, NULL);
 
   if (!stm->shutdown_event) {
     LOG("Can't create the shutdown event, error: %x.", GetLastError());
     wasapi_stream_destroy(stm);
     return CUBEB_ERROR;
@@ -991,19 +1080,25 @@ wasapi_stream_init(cubeb * context, cube
 
   if (!stm->refill_event) {
     SafeRelease(stm->shutdown_event);
     LOG("Can't create the refill event, error: %x.", GetLastError());
     wasapi_stream_destroy(stm);
     return CUBEB_ERROR;
   }
 
-  rv = setup_wasapi_stream(stm);
-  if (rv != CUBEB_OK) {
-    return rv;
+  {
+    /* Locking here is not stricly necessary, because we don't have a
+       notification client that can reset the stream yet, but it lets us
+       assert that the lock is held in the function. */
+    auto_lock lock(stm->stream_reset_lock);
+    rv = setup_wasapi_stream(stm);
+    if (rv != CUBEB_OK) {
+      return rv;
+    }
   }
 
   hr = register_notification_client(stm);
   if (FAILED(hr)) {
     /* this is not fatal, we can still play audio, but we won't be able
      * to keep using the default audio endpoint if it changes. */
     LOG("failed to register notification client, %x", hr);
   }
@@ -1012,51 +1107,64 @@ wasapi_stream_init(cubeb * context, cube
 
   return CUBEB_OK;
 }
 
 void close_wasapi_stream(cubeb_stream * stm)
 {
   assert(stm);
 
+  stm->stream_reset_lock->assert_current_thread_owns();
+
   SafeRelease(stm->client);
+  stm->client = nullptr;
+
   SafeRelease(stm->render_client);
+  stm->client = nullptr;
 
-  cubeb_resampler_destroy(stm->resampler);
+  if (stm->resampler) {
+    cubeb_resampler_destroy(stm->resampler);
+    stm->resampler = nullptr;
+  }
 
   free(stm->mix_buffer);
+  stm->mix_buffer = nullptr;
 }
 
 void wasapi_stream_destroy(cubeb_stream * stm)
 {
   assert(stm);
 
+  unregister_notification_client(stm);
+
   if (stm->thread) {
     SetEvent(stm->shutdown_event);
     WaitForSingleObject(stm->thread, INFINITE);
     CloseHandle(stm->thread);
     stm->thread = 0;
   }
 
   SafeRelease(stm->shutdown_event);
   SafeRelease(stm->refill_event);
-  DeleteCriticalSection(&stm->stream_reset_lock);
 
-  close_wasapi_stream(stm);
+  {
+    auto_lock lock(stm->stream_reset_lock);
+    close_wasapi_stream(stm);
+  }
 
-  unregister_notification_client(stm);
+  delete stm->stream_reset_lock;
 
   free(stm);
 }
 
 int wasapi_stream_start(cubeb_stream * stm)
 {
   HRESULT hr;
 
-  auto_lock lock(&stm->stream_reset_lock);
+  auto_lock lock(stm->stream_reset_lock);
 
   assert(stm);
 
   stm->thread = (HANDLE) _beginthreadex(NULL, 256 * 1024, wasapi_stream_render_loop, stm, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
   if (stm->thread == NULL) {
     LOG("could not create WASAPI render thread.");
     return CUBEB_ERROR;
   }
@@ -1071,26 +1179,27 @@ int wasapi_stream_start(cubeb_stream * s
 
   return CUBEB_OK;
 }
 
 int wasapi_stream_stop(cubeb_stream * stm)
 {
   assert(stm && stm->shutdown_event);
 
-  auto_lock lock(&stm->stream_reset_lock);
+  auto_lock lock(stm->stream_reset_lock);
 
   SetEvent(stm->shutdown_event);
 
   HRESULT hr = stm->client->Stop();
   if (FAILED(hr)) {
     LOG("could not stop AudioClient");
   }
 
   if (stm->thread) {
+    auto_unlock lock(stm->stream_reset_lock);
     WaitForSingleObject(stm->thread, INFINITE);
     CloseHandle(stm->thread);
     stm->thread = NULL;
   }
 
   stm->state_callback(stm, stm->user_ptr, CUBEB_STATE_STOPPED);
 
   return CUBEB_OK;
@@ -1104,17 +1213,17 @@ int wasapi_stream_get_position(cubeb_str
 
   return CUBEB_OK;
 }
 
 int wasapi_stream_get_latency(cubeb_stream * stm, uint32_t * latency)
 {
   assert(stm && latency);
 
-  auto_lock lock(&stm->stream_reset_lock);
+  auto_lock lock(stm->stream_reset_lock);
 
   /* The GetStreamLatency method only works if the
    * AudioClient has been initialized. */
   if (!stm->client) {
     return CUBEB_ERROR;
   }
 
   REFERENCE_TIME latency_hns;
@@ -1127,17 +1236,17 @@ int wasapi_stream_get_latency(cubeb_stre
 
 int wasapi_stream_set_volume(cubeb_stream * stm, float volume)
 {
   HRESULT hr;
   uint32_t channels;
   /* up to 9.1 for now */
   float volumes[10];
 
-  auto_lock lock(&stm->stream_reset_lock);
+  auto_lock lock(stm->stream_reset_lock);
 
   hr = stm->audio_stream_volume->GetChannelCount(&channels);
   if (hr != S_OK) {
     LOG("could not get the channel count: %x", hr);
     return CUBEB_ERROR;
   }
 
   assert(channels <= 10 && "bump the array size");