Possible solution to synchronization problems in webrtc audio capturer.

content/renderer/media/webrtc_audio_capturer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/webrtc_audio_capturer.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
@@ skipping 44 matching lines @@
 #endif
 
   return buffer_size;
 }
 
 // static
 scoped_refptr<WebRtcAudioCapturer> WebRtcAudioCapturer::CreateCapturer() {
   scoped_refptr<WebRtcAudioCapturer> capturer = new  WebRtcAudioCapturer();
   return capturer;
 }
 

[phoglund_chromium, 2013/02/07 14:03:18]

I found two places in the code which was doing roughly the same thing, hence
this new method here.

+bool WebRtcAudioCapturer::Reconfigure(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;
+  }
+
+  // Ignored since the audio stack uses float32.

[tommi (sloooow) - chröme, 2013/02/07 14:57:47]

s/Ignored/bits_per_sample is always 16

+  int bits_per_sample = 16;
+
+  // These snapshots will allow us to run callbacks outside the lock.
+  SinkList sinks;
+  media::AudioParameters params;
+  {
+    base::AutoLock auto_lock(lock_);
+

[phoglund_chromium, 2013/02/07 14:03:18]

So along with copying parameters, this is the addition. The buffer_in_use
parameter will keep track of if the buffer is being used by webrtc.

+    while (buffer_in_use_)
+      buffer_in_use_cv_.Wait();
+

[phoglund_chromium, 2013/02/07 14:03:18]

Once the buffer is not used, we can move ahead with the reconfiguration.

+    params_.Reset(format, channel_layout, sample_rate, bits_per_sample,
+                  buffer_size);
+    buffer_.reset(new int16[params_.frames_per_buffer() * params_.channels()]);
+
+    sinks = sinks_;
+    params = params_;
+  }
+
+  // Tell all sinks which format we use.
+  for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it) {
+    (*it)->SetCaptureFormat(params);
+  }
+
+  return true;
+}
+
 bool WebRtcAudioCapturer::Initialize(media::ChannelLayout channel_layout,
                                      int sample_rate) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!sinks_.empty());
   DVLOG(1) << "WebRtcAudioCapturer::Initialize()";
 
   media::AudioParameters::Format format =
       media::AudioParameters::AUDIO_PCM_LOW_LATENCY;
 
   DVLOG(1) << "Audio input hardware channel layout: " << channel_layout;
@@ skipping 14 matching lines @@
 
   // Verify that the reported input hardware sample rate is supported
   // on the current platform.
   if (std::find(&kValidInputRates[0],
                 &kValidInputRates[0] + arraysize(kValidInputRates),
                 sample_rate) ==
           &kValidInputRates[arraysize(kValidInputRates)]) {
     DLOG(ERROR) << sample_rate << " is not a supported input rate.";
     return false;
   }
 

[phoglund_chromium, 2013/02/07 14:03:18]

This code didn't check if GetBufferSize... returned 0 earlier, but I guess it
should? It will now with the extracted method.

-  int buffer_size = GetBufferSizeForSampleRate(sample_rate);
-
-  // Configure audio parameters for the default source.
-  params_.Reset(format, channel_layout, 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()
   // providing an alternaive media::AudioCapturerSource.
   SetCapturerSource(AudioDeviceFactory::NewInputDevice(),
                     channel_layout,
                     static_cast<float>(sample_rate));
 
   return true;
 }
 
 WebRtcAudioCapturer::WebRtcAudioCapturer()
-    : source_(NULL),
+    : buffer_in_use_cv_(&lock_),
+      buffer_in_use_(false),
+      source_(NULL),
       running_(false),
       buffering_(false) {
   DVLOG(1) << "WebRtcAudioCapturer::WebRtcAudioCapturer()";
 }
 
 WebRtcAudioCapturer::~WebRtcAudioCapturer() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(sinks_.empty());
   DCHECK(!loopback_fifo_);
   DVLOG(1) << "WebRtcAudioCapturer::~WebRtcAudioCapturer()";
@@ skipping 19 matching lines @@
   }
 }
 
 void WebRtcAudioCapturer::SetCapturerSource(
     const scoped_refptr<media::AudioCapturerSource>& source,
     media::ChannelLayout channel_layout,
     float sample_rate) {
   DVLOG(1) << "SetCapturerSource(channel_layout=" << channel_layout << ","
            << "sample_rate=" << sample_rate << ")";
   scoped_refptr<media::AudioCapturerSource> old_source;
+  media::AudioParameters::Format current_format;
   {
     base::AutoLock auto_lock(lock_);
     if (source_ == source)
       return;
 
     source_.swap(old_source);
     source_ = source;

[phoglund_chromium, 2013/02/07 14:03:18]

We copy this here to avoid grabbing the lock again inside the if statement
below.

+    current_format = params_.format();
   }
 
   const bool no_default_audio_source_exists = !buffer_.get();
 
   // Detach the old source from normal recording or perform first-time
   // initialization if Initialize() has never been called. For the second
   // case, the caller is not "taking over an ongoing session" but instead
   // "taking control over a new session".
   if (old_source || no_default_audio_source_exists) {
     DVLOG(1) << "New capture source will now be utilized.";
     if (old_source)
       old_source->Stop();
 
     // 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_format, channel_layout))
       return;
-    }
-
-    params_.Reset(params_.format(),
-                  channel_layout,
-                  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_);
-    }
   }
 
   if (source)
     source->Initialize(params_, this, this);
 }
 
 void WebRtcAudioCapturer::SetStopCallback(
     const base::Closure& on_device_stopped_cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) <<  "WebRtcAudioCapturer::SetStopCallback()";
@@ skipping 134 matching lines @@
   return (loopback_fifo_ != NULL);
 }
 
 void WebRtcAudioCapturer::Capture(media::AudioBus* audio_source,
                                   int audio_delay_milliseconds,
                                   double volume) {
   // This callback is driven by AudioInputDevice::AudioThreadCallback if
   // |source_| is AudioInputDevice, otherwise it is driven by client's
   // CaptureCallback.
   SinkList sinks;
+  int16* buffer;
+  int bytes_per_sample;
   {
     base::AutoLock auto_lock(lock_);
     if (!running_)
       return;
 

[phoglund_chromium, 2013/02/07 14:03:18]

Here we use buffer_in_use variable to protect against reconfigurations when we
are calling into WebRTC below. I assumed this method is called by a single
thread. We will get into trouble if this method is called concurrently.

[tommi (sloooow) - chröme, 2013/02/07 14:57:47]

There's no thread check that ensures that the method is called by a single
thread.  Since there's a lock here already it does suggest that we could get
here from multiple threads.

-    // Copy the sink list to a local variable.
+    // We're assuming here that this method is called only once at a time.
+    buffer_in_use_ = true;
+    buffer_in_use_cv_.Broadcast();
+
+    // Copy the stuff we will need to local variables.
     sinks = sinks_;
+    buffer = buffer_.get();
+    bytes_per_sample = params_.bits_per_sample() / 8;
 
     // Push captured audio to FIFO so it can be read by a local sink.
     // Buffering is only enabled if we are rendering a local media stream.
     if (loopback_fifo_ && buffering_) {
       if (loopback_fifo_->frames() + audio_source->frames() <=
           loopback_fifo_->max_frames()) {
         loopback_fifo_->Push(audio_source);
       } else {
         DLOG(WARNING) << "FIFO is full";
       }
     }
   }
 
   // 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);
 
   // 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(reinterpret_cast<const int16*>(buffer),
                        audio_source->channels(), audio_source->frames(),
                        audio_delay_milliseconds, volume);
   }
+

[phoglund_chromium, 2013/02/07 14:03:18]

If this note isn't true, we need WebRTC to somehow tell us when it's done with
the buffer. Let's hope it's not that bad :)

+  // Note: This assumes that the callbacks are guaranteed to be done with the
+  // data after returning!
+  {
+    base::AutoLock auto_lock(lock_);
+    buffer_in_use_ = false;
+    buffer_in_use_cv_.Broadcast();
+  }
 }
 
 void WebRtcAudioCapturer::OnCaptureError() {
   NOTIMPLEMENTED();
 }
 
 void WebRtcAudioCapturer::OnDeviceStarted(const std::string& device_id) {
   device_id_ = device_id;
 }
 
@@ skipping 12 matching lines @@
   // Inform the local renderer about the stopped device.
   // The renderer can then save resources by not asking for more data from
   // the stopped source. We are on the IO thread but the callback task will
   // be posted on the message loop of the main render thread thanks to
   // usage of BindToLoop() when the callback was initialized.
   if (!on_device_stopped_cb_.is_null())
     on_device_stopped_cb_.Run();
 }
 
 }  // namespace content