Bug 1308418 - Avoid double-constructing owned_critical_section via copy constructor. r=jya a=jcristau
authorMatthew Gregan <kinetik@flim.org>
Wed, 19 Oct 2016 14:42:03 +1300
changeset 356224 3a45c899d5015ba64e1cdbe206687951561e6fca
parent 356223 518df730e6bafdca866c69a7beddb0b3c0b7d116
child 356225 1f35cc719c5755327684736e561387c3f6f09a9a
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjya, jcristau
bugs1308418, 1310206
milestone51.0a2
Bug 1308418 - Avoid double-constructing owned_critical_section via copy constructor. r=jya a=jcristau This uplifts the upstream change 22557d466eceb6ff6ba70ae30d2dcd87648cde0b from https://github.com/kinetiknz/cubeb as a standalone patch suitable for uplift. This also cleans a previous uplift from bug 1310206.
media/libcubeb/bug1308418-mutex-copy-ctor.patch
media/libcubeb/src/cubeb_audiounit.cpp
media/libcubeb/src/cubeb_utils_unix.h
media/libcubeb/src/cubeb_utils_win.h
media/libcubeb/src/cubeb_wasapi.cpp
media/libcubeb/update.sh
media/libcubeb/wasapi-drift.patch
media/libcubeb/wasapi-stereo-mic.patch
new file mode 100644
--- /dev/null
+++ b/media/libcubeb/bug1308418-mutex-copy-ctor.patch
@@ -0,0 +1,123 @@
+commit 22557d466eceb6ff6ba70ae30d2dcd87648cde0b
+Author: Cervantes Yu <cyu@mozilla.com>
+Date:   Tue Oct 11 10:44:35 2016 +0800
+
+    Avoid double-constructing owned_critical_section via copy constructor.
+
+diff --git a/src/cubeb_audiounit.cpp b/src/cubeb_audiounit.cpp
+index c8a135f..d2f144b 100644
+--- a/src/cubeb_audiounit.cpp
++++ b/src/cubeb_audiounit.cpp
+@@ -423,14 +423,13 @@ audiounit_init(cubeb ** context, char const * context_name)
+ 
+   *context = NULL;
+ 
+-  ctx = new cubeb;
++  ctx = (cubeb *)calloc(1, sizeof(cubeb));
+   assert(ctx);
+-  PodZero(ctx, 1);
++  // Placement new to call the ctors of cubeb members.
++  new (ctx) cubeb();
+ 
+   ctx->ops = &audiounit_ops;
+ 
+-  ctx->mutex = owned_critical_section();
+-
+   ctx->active_streams = 0;
+ 
+   ctx->limit_streams = kCFCoreFoundationVersionNumber < kCFCoreFoundationVersionNumber10_7;
+@@ -833,7 +832,8 @@ audiounit_destroy(cubeb * ctx)
+     audiounit_remove_device_listener(ctx);
+   }
+ 
+-  delete ctx;
++  ctx->~cubeb();
++  free(ctx);
+ }
+ 
+ static void audiounit_stream_destroy(cubeb_stream * stm);
+@@ -1117,9 +1117,10 @@ audiounit_stream_init(cubeb * context,
+     }
+   }
+ 
+-  stm = new cubeb_stream;
++  stm = (cubeb_stream *) calloc(1, sizeof(cubeb_stream));
+   assert(stm);
+-  PodZero(stm, 1);
++  // Placement new to call the ctors of cubeb_stream members.
++  new (stm) cubeb_stream();
+ 
+   /* These could be different in the future if we have both
+    * full-duplex stream and different devices for input vs output. */
+@@ -1130,7 +1131,6 @@ audiounit_stream_init(cubeb * context,
+   stm->state_callback = state_callback;
+   stm->user_ptr = user_ptr;
+   stm->device_changed_callback = NULL;
+-  stm->mutex = owned_critical_section();
+ 
+   /* Init data members where necessary */
+   stm->hw_latency_frames = UINT64_MAX;
+@@ -1443,7 +1443,8 @@ audiounit_stream_destroy(cubeb_stream * stm)
+     stm->context->active_streams -= 1;
+   }
+ 
+-  delete stm;
++  stm->~cubeb_stream();
++  free(stm);
+ }
+ 
+ static int
+diff --git a/src/cubeb_utils_unix.h b/src/cubeb_utils_unix.h
+index 72041f5..72476ac 100644
+--- a/src/cubeb_utils_unix.h
++++ b/src/cubeb_utils_unix.h
+@@ -80,6 +80,10 @@ public:
+ 
+ private:
+   pthread_mutex_t mutex;
++
++  // Disallow copy and assignment because pthread_mutex_t cannot be copied.
++  owned_critical_section(const owned_critical_section&);
++  owned_critical_section& operator=(const owned_critical_section&);
+ };
+ 
+ #endif /* CUBEB_UTILS_UNIX */
+diff --git a/src/cubeb_utils_win.h b/src/cubeb_utils_win.h
+index 90e81c1..322ad24 100644
+--- a/src/cubeb_utils_win.h
++++ b/src/cubeb_utils_win.h
+@@ -62,6 +62,10 @@ private:
+ #ifdef DEBUG
+   DWORD owner;
+ #endif
++
++  // Disallow copy and assignment because CRICICAL_SECTION cannot be copied.
++  owned_critical_section(const owned_critical_section&);
++  owned_critical_section& operator=(const owned_critical_section&);
+ };
+ 
+ #endif /* CUBEB_UTILS_WIN */
+diff --git a/src/cubeb_wasapi.cpp b/src/cubeb_wasapi.cpp
+index b2f28d2..d960dfc 100644
+--- a/src/cubeb_wasapi.cpp
++++ b/src/cubeb_wasapi.cpp
+@@ -1637,7 +1637,8 @@ wasapi_stream_init(cubeb * context, cubeb_stream ** stream,
+   stm->latency = latency_frames;
+   stm->volume = 1.0;
+ 
+-  stm->stream_reset_lock = owned_critical_section();
++  // Placement new to call ctor.
++  new (&stm->stream_reset_lock) owned_critical_section();
+ 
+   stm->reconfigure_event = CreateEvent(NULL, 0, 0, NULL);
+   if (!stm->reconfigure_event) {
+@@ -1734,6 +1735,9 @@ void wasapi_stream_destroy(cubeb_stream * stm)
+     close_wasapi_stream(stm);
+   }
+ 
++  // Need to call dtor to free the resource in owned_critical_section.
++  stm->stream_reset_lock.~owned_critical_section();
++
+   free(stm);
+ }
+ 
--- a/media/libcubeb/src/cubeb_audiounit.cpp
+++ b/media/libcubeb/src/cubeb_audiounit.cpp
@@ -432,24 +432,23 @@ audiounit_output_callback(void * user_pt
 extern "C" {
 int
 audiounit_init(cubeb ** context, char const * context_name)
 {
   cubeb * ctx;
 
   *context = NULL;
 
-  ctx = new cubeb;
+  ctx = (cubeb *)calloc(1, sizeof(cubeb));
   assert(ctx);
-  PodZero(ctx, 1);
+  // Placement new to call the ctors of cubeb members.
+  new (ctx) cubeb();
 
   ctx->ops = &audiounit_ops;
 
-  ctx->mutex = owned_critical_section();
-
   ctx->active_streams = 0;
 
   ctx->limit_streams = kCFCoreFoundationVersionNumber < kCFCoreFoundationVersionNumber10_7;
 #if !TARGET_OS_IPHONE
   cubeb_set_coreaudio_notification_runloop();
 #endif
 
   *context = ctx;
@@ -842,17 +841,18 @@ audiounit_destroy(cubeb * ctx)
   // assert(ctx->active_streams == 0);
 
   /* Unregister the callback if necessary. */
   if(ctx->collection_changed_callback) {
     auto_lock lock(ctx->mutex);
     audiounit_remove_device_listener(ctx);
   }
 
-  delete ctx;
+  ctx->~cubeb();
+  free(ctx);
 }
 
 static void audiounit_stream_destroy(cubeb_stream * stm);
 
 static int
 audio_stream_desc_init(AudioStreamBasicDescription * ss,
                        const cubeb_stream_params * stream_params)
 {
@@ -1126,30 +1126,30 @@ audiounit_stream_init(cubeb * context,
                               output_stream_params,
                               output_device);
     if (r != CUBEB_OK) {
       LOG("AudioUnit creation for output failed.");
       return r;
     }
   }
 
-  stm = new cubeb_stream;
+  stm = (cubeb_stream *) calloc(1, sizeof(cubeb_stream));
   assert(stm);
-  PodZero(stm, 1);
+  // Placement new to call the ctors of cubeb_stream members.
+  new (stm) cubeb_stream();
 
   /* These could be different in the future if we have both
    * full-duplex stream and different devices for input vs output. */
   stm->input_unit  = (input_stream_params != NULL) ? input_unit : NULL;
   stm->output_unit = (output_stream_params != NULL) ? output_unit : NULL;
   stm->context = context;
   stm->data_callback = data_callback;
   stm->state_callback = state_callback;
   stm->user_ptr = user_ptr;
   stm->device_changed_callback = NULL;
-  stm->mutex = owned_critical_section();
 
   /* Init data members where necessary */
   stm->hw_latency_frames = UINT64_MAX;
 
   /* Silently clamp the latency down to the platform default, because we
    * synthetize the clock from the callbacks, and we want the clock to update
    * often. */
   latency_frames = audiounit_clamp_latency(stm, latency_frames);
@@ -1452,17 +1452,18 @@ audiounit_stream_destroy(cubeb_stream * 
 #endif
 
   {
     auto_lock lock(stm->context->mutex);
     assert(stm->context->active_streams >= 1);
     stm->context->active_streams -= 1;
   }
 
-  delete stm;
+  stm->~cubeb_stream();
+  free(stm);
 }
 
 static int
 audiounit_stream_start(cubeb_stream * stm)
 {
   {
     auto_lock lock(stm->mutex);
     stm->shutdown = 0;
--- a/media/libcubeb/src/cubeb_utils_unix.h
+++ b/media/libcubeb/src/cubeb_utils_unix.h
@@ -75,11 +75,15 @@ public:
 #ifdef DEBUG
     int r = pthread_mutex_lock(&mutex);
     assert(r == EDEADLK);
 #endif
   }
 
 private:
   pthread_mutex_t mutex;
+
+  // Disallow copy and assignment because pthread_mutex_t cannot be copied.
+  owned_critical_section(const owned_critical_section&);
+  owned_critical_section& operator=(const owned_critical_section&);
 };
 
 #endif /* CUBEB_UTILS_UNIX */
--- a/media/libcubeb/src/cubeb_utils_win.h
+++ b/media/libcubeb/src/cubeb_utils_win.h
@@ -57,11 +57,15 @@ public:
 #endif
   }
 
 private:
   CRITICAL_SECTION critical_section;
 #ifdef DEBUG
   DWORD owner;
 #endif
+
+  // Disallow copy and assignment because CRICICAL_SECTION cannot be copied.
+  owned_critical_section(const owned_critical_section&);
+  owned_critical_section& operator=(const owned_critical_section&);
 };
 
 #endif /* CUBEB_UTILS_WIN */
--- a/media/libcubeb/src/cubeb_wasapi.cpp
+++ b/media/libcubeb/src/cubeb_wasapi.cpp
@@ -1637,17 +1637,18 @@ wasapi_stream_init(cubeb * context, cube
   if (output_stream_params) {
     stm->output_stream_params = *output_stream_params;
     stm->output_device = output_device;
   }
 
   stm->latency = latency_frames;
   stm->volume = 1.0;
 
-  stm->stream_reset_lock = owned_critical_section();
+  // Placement new to call ctor.
+  new (&stm->stream_reset_lock) owned_critical_section();
 
   stm->reconfigure_event = CreateEvent(NULL, 0, 0, NULL);
   if (!stm->reconfigure_event) {
     LOG("Can't create the reconfigure event, error: %x\n", GetLastError());
     wasapi_stream_destroy(stm);
     return CUBEB_ERROR;
   }
 
@@ -1734,16 +1735,19 @@ void wasapi_stream_destroy(cubeb_stream 
   SafeRelease(stm->refill_event);
   SafeRelease(stm->input_available_event);
 
   {
     auto_lock lock(stm->stream_reset_lock);
     close_wasapi_stream(stm);
   }
 
+  // Need to call dtor to free the resource in owned_critical_section.
+  stm->stream_reset_lock.~owned_critical_section();
+
   free(stm);
 }
 
 enum StreamDirection {
   OUTPUT,
   INPUT
 };
 
--- a/media/libcubeb/update.sh
+++ b/media/libcubeb/update.sh
@@ -52,8 +52,14 @@ if [ -n "$rev" ]; then
     echo "WARNING: updating from a dirty git repository."
   fi
   sed -i.bak -e "/The git commit ID used was/ s/[0-9a-f]\{40\}\(-dirty\)\{0,1\}\./$version./" README_MOZILLA
   rm README_MOZILLA.bak
 else
   echo "Remember to update README_MOZILLA with the version details."
 fi
 
+echo "Applying a patch on top of $version"
+patch -p1 < ./wasapi-drift.patch
+echo "Applying another patch on top of $version"
+patch -p1 < ./wasapi-stereo-mic.patch
+echo "Applying another patch on top of $version"
+patch -p1 < ./bug1308418-mutex-copy-ctor.patch
new file mode 100644
--- /dev/null
+++ b/media/libcubeb/wasapi-drift.patch
@@ -0,0 +1,101 @@
+From 6ae23a6355e35016ad7f01814e92e73b3047d54b Mon Sep 17 00:00:00 2001
+From: Paul Adenot <paul@paul.cx>
+Date: Thu, 29 Sep 2016 02:21:06 +0200
+Subject: [PATCH] Fix drift in WASAPI when having different output and input
+ sample device rate (#155)
+
+* Round instead of ceiling to avoid rounding buildup of buffer size.
+
+* Always consume all the data for an output buffer.
+
+* Add padding only if we don't have enough input data in the linear buffer.
+
+* Define NOMINMAX so that std::max compiles.
+---
+ src/cubeb_resampler.cpp        |  5 +++++
+ src/cubeb_resampler_internal.h |  2 +-
+ src/cubeb_wasapi.cpp           | 11 +++++------
+ 3 files changed, 11 insertions(+), 7 deletions(-)
+
+diff --git a/src/cubeb_resampler.cpp b/src/cubeb_resampler.cpp
+index a476744..7b9326e 100644
+--- a/src/cubeb_resampler.cpp
++++ b/src/cubeb_resampler.cpp
+@@ -4,6 +4,11 @@
+  * This program is made available under an ISC-style license.  See the
+  * accompanying file LICENSE for details.
+  */
++
++#ifndef NOMINMAX
++#define NOMINMAX
++#endif // NOMINMAX
++
+ #include <algorithm>
+ #include <cmath>
+ #include <cassert>
+diff --git a/src/cubeb_resampler_internal.h b/src/cubeb_resampler_internal.h
+index c547991..a605892 100644
+--- a/src/cubeb_resampler_internal.h
++++ b/src/cubeb_resampler_internal.h
+@@ -263,7 +263,7 @@ public:
+    * number of output frames will be exactly equal. */
+   uint32_t input_needed_for_output(uint32_t output_frame_count)
+   {
+-    return (uint32_t)ceilf((output_frame_count - samples_to_frames(resampling_out_buffer.length()))
++    return (uint32_t)roundf((output_frame_count - samples_to_frames(resampling_out_buffer.length()))
+                           * resampling_ratio);
+ 
+   }
+diff --git a/src/cubeb_wasapi.cpp b/src/cubeb_wasapi.cpp
+index 3a7f6fe..b17e7d5 100644
+--- a/src/cubeb_wasapi.cpp
++++ b/src/cubeb_wasapi.cpp
+@@ -613,7 +613,7 @@ bool get_input_buffer(cubeb_stream * stm)
+ 
+ /* Get an output buffer from the render_client. It has to be released before
+  * exiting the callback. */
+-bool get_output_buffer(cubeb_stream * stm, size_t max_frames, float *& buffer, size_t & frame_count)
++bool get_output_buffer(cubeb_stream * stm, float *& buffer, size_t & frame_count)
+ {
+   UINT32 padding_out;
+   HRESULT hr;
+@@ -635,7 +635,7 @@ bool get_output_buffer(cubeb_stream * stm, size_t max_frames, float *& buffer, s
+     return true;
+   }
+ 
+-  frame_count = std::min<size_t>(max_frames, stm->output_buffer_frame_count - padding_out);
++  frame_count = stm->output_buffer_frame_count - padding_out;
+   BYTE * output_buffer;
+ 
+   hr = stm->render_client->GetBuffer(frame_count, &output_buffer);
+@@ -673,7 +673,7 @@ refill_callback_duplex(cubeb_stream * stm)
+     return true;
+   }
+ 
+-  rv = get_output_buffer(stm, input_frames, output_buffer, output_frames);
++  rv = get_output_buffer(stm, output_buffer, output_frames);
+   if (!rv) {
+     hr = stm->render_client->ReleaseBuffer(output_frames, 0);
+     return rv;
+@@ -687,7 +687,7 @@ refill_callback_duplex(cubeb_stream * stm)
+ 
+   // When WASAPI has not filled the input buffer yet, send silence.
+   double output_duration = double(output_frames) / stm->output_mix_params.rate;
+-  double input_duration = double(input_frames) / stm->input_mix_params.rate;
++  double input_duration = double(stm->linear_input_buffer.length() / stm->input_mix_params.channels) / stm->input_mix_params.rate;
+   if (input_duration < output_duration) {
+     size_t padding = size_t(round((output_duration - input_duration) * stm->input_mix_params.rate));
+     LOG("padding silence: out=%f in=%f pad=%u\n", output_duration, input_duration, padding);
+@@ -745,8 +745,7 @@ refill_callback_output(cubeb_stream * stm)
+ 
+   XASSERT(!has_input(stm) && has_output(stm));
+ 
+-  rv = get_output_buffer(stm, std::numeric_limits<size_t>::max(),
+-                         output_buffer, output_frames);
++  rv = get_output_buffer(stm, output_buffer, output_frames);
+   if (!rv) {
+     return rv;
+   }
+-- 
+2.7.4
+
new file mode 100644
--- /dev/null
+++ b/media/libcubeb/wasapi-stereo-mic.patch
@@ -0,0 +1,28 @@
+From 50d92c0eaa05a93f4dd8ff5072e983c6c84d0369 Mon Sep 17 00:00:00 2001
+From: Paul Adenot <paul@paul.cx>
+Date: Tue, 4 Oct 2016 07:18:04 -0700
+Subject: [PATCH] Divide by the number of channel of the stream to compute the
+ duratio of the buffer.
+
+We downmix right before, so this is in frames in with the channel count of the
+stream, not the mix.
+---
+ src/cubeb_wasapi.cpp | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/src/cubeb_wasapi.cpp b/src/cubeb_wasapi.cpp
+index bd2cbed..87aa305 100644
+--- a/src/cubeb_wasapi.cpp
++++ b/src/cubeb_wasapi.cpp
+@@ -687,7 +687,7 @@ refill_callback_duplex(cubeb_stream * stm)
+ 
+   // When WASAPI has not filled the input buffer yet, send silence.
+   double output_duration = double(output_frames) / stm->output_mix_params.rate;
+-  double input_duration = double(stm->linear_input_buffer.length() / stm->input_mix_params.channels) / stm->input_mix_params.rate;
++  double input_duration = double(stm->linear_input_buffer.length() / stm->input_stream_params.channels) / stm->input_mix_params.rate;
+   if (input_duration < output_duration) {
+     size_t padding = size_t(round((output_duration - input_duration) * stm->input_mix_params.rate));
+     LOG("padding silence: out=%f in=%f pad=%u\n", output_duration, input_duration, padding);
+-- 
+2.7.4
+