Possible solution to synchronization problems in webrtc audio capturer.

Re-land of https://codereview.chromium.org/12220063/.

This will hold on to a reference to the buffer while the buffer is being used by WebRTC. I also tried to fix the places where synchronization was missing (mostly for the params_ instance).

BUG=173987
TBR=tommi@chromium.org


Review URL: https://chromiumcodereview.appspot.com/12261003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182227 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 109 insertions, 48 deletions

diff --git a/content/renderer/media/webrtc_audio_capturer.cc b/content/renderer/media/webrtc_audio_capturer.cc
index 8aad07e3..b4d349a 100644
--- a/content/renderer/media/webrtc_audio_capturer.cc
+++ b/content/renderer/media/webrtc_audio_capturer.cc
@@ -60,12 +60,73 @@ static int GetBufferSizeForSampleRate(int sample_rate) {
   return buffer_size;
 }
 
+// This is a temporary audio buffer with parameters used to send data to
+// callbacks.
+class WebRtcAudioCapturer::ConfiguredBuffer :
+    public base::RefCounted<WebRtcAudioCapturer::ConfiguredBuffer> {
+ public:
+  ConfiguredBuffer() {}
+
+  bool Initialize(int sample_rate,
+                  media::AudioParameters::Format format,
+                  media::ChannelLayout channel_layout) {
+    int buffer_size = GetBufferSizeForSampleRate(sample_rate);
+    if (!buffer_size) {
+      DLOG(ERROR) << "Unsupported sample-rate: " << sample_rate;
+      return false;
+    }
+
+    // bits_per_sample is always 16 for now.
+    int bits_per_sample = 16;
+
+    params_.Reset(format, channel_layout, 0, sample_rate, bits_per_sample,
+                  buffer_size);
+    buffer_.reset(new int16[params_.frames_per_buffer() * params_.channels()]);
+
+    return true;
+  }
+
+  int16* buffer() const { return buffer_.get(); }
+  const media::AudioParameters& params() const { return params_; }
+
+ private:
+  ~ConfiguredBuffer() {}
+  friend class base::RefCounted<WebRtcAudioCapturer::ConfiguredBuffer>;
+
+  scoped_ptr<int16[]> buffer_;
+
+  // Cached values of utilized audio parameters.
+  media::AudioParameters params_;
+};
+
 // static
 scoped_refptr<WebRtcAudioCapturer> WebRtcAudioCapturer::CreateCapturer() {
   scoped_refptr<WebRtcAudioCapturer> capturer = new  WebRtcAudioCapturer();
   return capturer;
 }
 
+bool WebRtcAudioCapturer::Reconfigure(int sample_rate,
+                                      media::AudioParameters::Format format,
+                                      media::ChannelLayout channel_layout) {
+  scoped_refptr<ConfiguredBuffer> new_buffer(new ConfiguredBuffer());
+  if (!new_buffer->Initialize(sample_rate, format, channel_layout))
+    return false;
+
+  SinkList sinks;
+  {
+    base::AutoLock auto_lock(lock_);
+
+    buffer_ = new_buffer;
+    sinks = sinks_;
+  }
+
+  // Tell all sinks which format we use.
+  for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it)
+    (*it)->SetCaptureFormat(new_buffer->params());
+
+  return true;
+}
+
 bool WebRtcAudioCapturer::Initialize(media::ChannelLayout channel_layout,
                                      int sample_rate) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -101,18 +162,8 @@ bool WebRtcAudioCapturer::Initialize(media::ChannelLayout channel_layout,
     return false;
   }
 
-  int buffer_size = GetBufferSizeForSampleRate(sample_rate);
-
-  // Configure audio parameters for the default source.
-  params_.Reset(format, channel_layout, 0, sample_rate, 16, buffer_size);
-
-  // Tell all sinks which format we use.
-  for (SinkList::const_iterator it = sinks_.begin();
-    it != sinks_.end(); ++it) {
-      (*it)->SetCaptureFormat(params_);
-  }
-
-  buffer_.reset(new int16[params_.frames_per_buffer() * params_.channels()]);
+  if (!Reconfigure(sample_rate, format, channel_layout))
+    return false;
 
   // Create and configure the default audio capturing source. The |source_|
   // will be overwritten if an external client later calls SetCapturerSource()
@@ -165,6 +216,7 @@ void WebRtcAudioCapturer::SetCapturerSource(
   DVLOG(1) << "SetCapturerSource(channel_layout=" << channel_layout << ","
            << "sample_rate=" << sample_rate << ")";
   scoped_refptr<media::AudioCapturerSource> old_source;
+  scoped_refptr<ConfiguredBuffer> current_buffer;
   {
     base::AutoLock auto_lock(lock_);
     if (source_ == source)
@@ -172,9 +224,10 @@ void WebRtcAudioCapturer::SetCapturerSource(
 
     source_.swap(old_source);
     source_ = source;
+    current_buffer = buffer_;
   }
 
-  const bool no_default_audio_source_exists = !buffer_.get();
+  const bool no_default_audio_source_exists = !current_buffer->buffer();
 
   // Detach the old source from normal recording or perform first-time
   // initialization if Initialize() has never been called. For the second
@@ -188,30 +241,20 @@ void WebRtcAudioCapturer::SetCapturerSource(
     // Dispatch the new parameters both to the sink(s) and to the new source.
     // The idea is to get rid of any dependency of the microphone parameters
     // which would normally be used by default.
-
-    int buffer_size = GetBufferSizeForSampleRate(sample_rate);
-    if (!buffer_size) {
-      DLOG(ERROR) << "Unsupported sample-rate: " << sample_rate;
+    if (!Reconfigure(sample_rate, current_buffer->params().format(),
+                     channel_layout)) {
       return;
-    }
-
-    params_.Reset(params_.format(),
-                  channel_layout,
-                  0,
-                  sample_rate,
-                  16,  // ignored since the audio stack uses float32.
-                  buffer_size);
-
-    buffer_.reset(new int16[params_.frames_per_buffer() * params_.channels()]);
-
-    for (SinkList::const_iterator it = sinks_.begin();
-         it != sinks_.end(); ++it) {
-      (*it)->SetCaptureFormat(params_);
+    } else {
+      // The buffer has been reconfigured.  Update |current_buffer|.
+      base::AutoLock auto_lock(lock_);
+      current_buffer = buffer_;
     }
   }
 
-  if (source)
-    source->Initialize(params_, this, this);
+  if (source) {
+    // Make sure to grab the new parameters in case they were reconfigured.
+    source->Initialize(current_buffer->params(), this, this);
+  }
 }
 
 void WebRtcAudioCapturer::SetStopCallback(
@@ -236,8 +279,9 @@ void WebRtcAudioCapturer::PrepareLoopback() {
   // in case since these tests were performed on a 16 core, 64GB Win 7
   // machine. We could also add some sort of error notifier in this area if
   // the FIFO overflows.
-  loopback_fifo_.reset(new media::AudioFifo(params_.channels(),
-                       10 * params_.frames_per_buffer()));
+  loopback_fifo_.reset(new media::AudioFifo(
+      buffer_->params().channels(),
+      10 * buffer_->params().frames_per_buffer()));
   buffering_ = true;
 }
 
@@ -362,12 +406,16 @@ void WebRtcAudioCapturer::Capture(media::AudioBus* audio_source,
   // |source_| is AudioInputDevice, otherwise it is driven by client's
   // CaptureCallback.
   SinkList sinks;
+  scoped_refptr<ConfiguredBuffer> buffer_ref_while_calling;
   {
     base::AutoLock auto_lock(lock_);
     if (!running_)
       return;
 
-    // Copy the sink list to a local variable.
+    // Copy the stuff we will need to local variables. In particular, we grab
+    // a reference to the buffer so we can ensure it stays alive even if the
+    // buffer is reconfigured while we are calling back.
+    buffer_ref_while_calling = buffer_;
     sinks = sinks_;
 
     // Push captured audio to FIFO so it can be read by a local sink.
@@ -382,17 +430,19 @@ void WebRtcAudioCapturer::Capture(media::AudioBus* audio_source,
     }
   }
 
+  int bytes_per_sample =
+      buffer_ref_while_calling->params().bits_per_sample() / 8;
+
   // Interleave, scale, and clip input to int and store result in
   // a local byte buffer.
-  audio_source->ToInterleaved(audio_source->frames(),
-                              params_.bits_per_sample() / 8,
-                              buffer_.get());
+  audio_source->ToInterleaved(audio_source->frames(), bytes_per_sample,
+                              buffer_ref_while_calling->buffer());
 
   // Feed the data to the sinks.
   for (SinkList::const_iterator it = sinks.begin();
        it != sinks.end();
        ++it) {
-    (*it)->CaptureData(reinterpret_cast<const int16*>(buffer_.get()),
+    (*it)->CaptureData(buffer_ref_while_calling->buffer(),
                        audio_source->channels(), audio_source->frames(),
                        audio_delay_milliseconds, volume);
   }
@@ -427,4 +477,9 @@ void WebRtcAudioCapturer::OnDeviceStopped() {
     on_device_stopped_cb_.Run();
 }
 
+media::AudioParameters WebRtcAudioCapturer::audio_parameters() const {
+  base::AutoLock auto_lock(lock_);
+  return buffer_->params();
+}
+
 }  // namespace content
diff --git a/content/renderer/media/webrtc_audio_capturer.h b/content/renderer/media/webrtc_audio_capturer.h
index afb072f..24eea03 100644
--- a/content/renderer/media/webrtc_audio_capturer.h
+++ b/content/renderer/media/webrtc_audio_capturer.h
@@ -136,7 +136,10 @@ class CONTENT_EXPORT WebRtcAudioCapturer
   // Audio parameters utilized by the audio capturer. Can be utilized by
   // a local renderer to set up a renderer using identical parameters as the
   // capturer.
-  const media::AudioParameters& audio_parameter() const { return params_; }
+  // TODO(phoglund): This accessor is inherently unsafe since the returned
+  // parameters can become outdated at any time. Think over the implications
+  // of this accessor and if we can remove it.
+  media::AudioParameters audio_parameters() const;
 
   // AudioCapturerSource::CaptureCallback implementation.
   // Called on the AudioInputDevice audio thread.
@@ -165,12 +168,17 @@ class CONTENT_EXPORT WebRtcAudioCapturer
 
   WebRtcAudioCapturer();
 
+  // Reconfigures the capturer with a new buffer size and capture parameters.
+  // Must be called without holding the lock. Returns true on success.
+  bool Reconfigure(int sample_rate, media::AudioParameters::Format format,
+                   media::ChannelLayout channel_layout);
+
   // Used to DCHECK that we are called on the correct thread.
   base::ThreadChecker thread_checker_;
 
   // Protects |source_|, |sinks_|, |running_|, |on_device_stopped_cb_|,
-  // |loopback_fifo_| and |buffering_|.
-  base::Lock lock_;
+  // |loopback_fifo_|, |params_| and |buffering_|.
+  mutable base::Lock lock_;
 
   // A list of sinks that the audio data is fed to.
   SinkList sinks_;
@@ -178,12 +186,10 @@ class CONTENT_EXPORT WebRtcAudioCapturer
   // The audio data source from the browser process.
   scoped_refptr<media::AudioCapturerSource> source_;
 
-  // Cached values of utilized audio parameters. Platform dependent.
-  media::AudioParameters params_;
-
   // Buffers used for temporary storage during capture callbacks.
   // Allocated during initialization.
-  scoped_array<int16> buffer_;
+  class ConfiguredBuffer;
+  scoped_refptr<ConfiguredBuffer> buffer_;
   std::string device_id_;
   bool running_;
 
diff --git a/content/renderer/media/webrtc_local_audio_renderer.cc b/content/renderer/media/webrtc_local_audio_renderer.cc
index d6efff6..f24045a 100644
--- a/content/renderer/media/webrtc_local_audio_renderer.cc
+++ b/content/renderer/media/webrtc_local_audio_renderer.cc
@@ -163,7 +163,7 @@ void WebRtcLocalAudioRenderer::Start() {
   // cases where resampling is needed on the output side.
   // TODO(henrika): verify this scheme on as many different devices and
   // combinations of sample rates as possible
-  media::AudioParameters source_params = source_->audio_parameter();
+  media::AudioParameters source_params = source_->audio_parameters();
   media::AudioParameters sink_params(source_params.format(),
                                      source_params.channel_layout(),
                                      source_params.sample_rate(),