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 39 matching lines @@
   // found that the best combination is 20ms on the input side and 10ms on the
   // output side.
   // TODO(henrika): It might be possible to reduce the input buffer
   // size and reduce the delay even more.
   buffer_size = 2 * sample_rate / 100;
 #endif
 
   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:
+  scoped_ptr<int16[]> buffer_;

[tommi (sloooow) - chröme, 2013/02/08 16:32:04]

hmm... this doesn't look right.  should just be scoped_ptr<int16>

[phoglund_chromium, 2013/02/08 17:06:00]

The deprecation comment for scoped_array says "replace scoped_array<int16> with
scoped_ptr<int16[]>."

+
+  // Cached values of utilized audio parameters. Platform dependent.

[tommi (sloooow) - chröme, 2013/02/08 16:32:04]

nit: skip the "platform dependent" part.

[phoglund_chromium, 2013/02/08 17:06:00]

Done.

+  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());
   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 15 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;
   }
 
-  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()
   // providing an alternaive media::AudioCapturerSource.
   SetCapturerSource(AudioDeviceFactory::NewInputDevice(),
                     channel_layout,
                     static_cast<float>(sample_rate));
 
   return true;
 }
@@ skipping 32 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;
+  scoped_refptr<ConfiguredBuffer> current_buffer;
   {
     base::AutoLock auto_lock(lock_);
     if (source_ == source)
       return;
 
     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
   // 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_buffer->params().format(),
+                     channel_layout))

[tommi (sloooow) - chröme, 2013/02/08 16:32:04]

use {}

[phoglund_chromium, 2013/02/08 17:06:00]

Done.

       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_);
-    }
   }
 
-  if (source)
-    source->Initialize(params_, this, this);
+  if (source) {
+    // Make sure to grab the new parameters in case they were reconfigured.

[phoglund_chromium, 2013/02/08 16:10:38]

This here was tricky. I initially passed in current_buffer->params(), but that
is NOT what we want if the Reconfigure above happens. By using the accessor I'm
sure to get the latest version of params, at the cost of one more lock grab.

[tommi (sloooow) - chröme, 2013/02/08 16:32:04]

audio_parameters() returns a reference to the parameters owned by buffer_.  Once
you've called that method, you're no longer holding the lock that protects
|buffer_|.  So, if buffer_ is changed during the call to Initialize(), we will
crash.

Instead I suggest this little change to the code above:

if (!Reconfigure(sample_rate, current_buffer->params().format(),
                 channel_layout)) {
  return;
} else {
  // The buffer has been reconfigured.  Update |current_buffer|.
  base::AutoLock auto_lock(lock_);
  current_buffer = buffer_;
}

It's true that this is yet one more lock and we could avoid that but I'll leave
it up to you if you think it's worth it.

But with the above then when you get here, you can use |current_buffer| as you
did before.

[phoglund_chromium, 2013/02/08 17:06:00]

Done.

+    source->Initialize(audio_parameter(), this, this);
+  }
 }
 
 void WebRtcAudioCapturer::SetStopCallback(
     const base::Closure& on_device_stopped_cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) <<  "WebRtcAudioCapturer::SetStopCallback()";
   base::AutoLock auto_lock(lock_);
   on_device_stopped_cb_ = on_device_stopped_cb;
 }
 
 void WebRtcAudioCapturer::PrepareLoopback() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) <<  "WebRtcAudioCapturer::PrepareLoopback()";
   base::AutoLock auto_lock(lock_);
   DCHECK(!loopback_fifo_);
 
   // TODO(henrika): we could add a more dynamic solution here but I prefer
   // a fixed size combined with bad audio at overflow. The alternative is
   // that we start to build up latency and that can be more difficult to
   // detect. Tests have shown that the FIFO never contains more than 2 or 3
   // audio frames but I have selected a max size of ten buffers just
   // 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;
 }
 
 void WebRtcAudioCapturer::CancelLoopback() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) <<  "WebRtcAudioCapturer::CancelLoopback()";
   base::AutoLock auto_lock(lock_);
   buffering_ = false;
   if (loopback_fifo_.get() != NULL) {
     loopback_fifo_->Clear();
@@ skipping 104 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;
+  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.
     // 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 {
         DVLOG(1) << "FIFO is full";
       }
     }
   }
 
+  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);
   }
 }
 
 void WebRtcAudioCapturer::OnCaptureError() {
   NOTIMPLEMENTED();
 }
 
 void WebRtcAudioCapturer::OnDeviceStarted(const std::string& device_id) {
@@ skipping 14 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();
 }
 
+const media::AudioParameters& WebRtcAudioCapturer::audio_parameter() {
+  base::AutoLock auto_lock(lock_);
+  DCHECK(buffer_.get() != NULL);
+  return buffer_->params();
+}
+
 }  // namespace content