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:
+  ~ConfiguredBuffer() {}
+  friend class base::RefCounted<WebRtcAudioCapturer::ConfiguredBuffer>;
+
+  scoped_ptr<int16[]> buffer_;

[tommi (sloooow) - chröme, 2013/02/08 20:00:39]

ah, thanks. I didn't know about the DefaultDeleter specialization we have now. 
Makes sense.

[phoglund_chromium, 2013/02/11 09:18:57]

Done.

+
+  // 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)

[tommi (sloooow) - chröme, 2013/02/08 20:00:39]

No action needed in this CL since I think you've improved the code considerably
and fixed the most serious issues.  Thinking about this pattern some more and
the fact that Reconfigure can be called on multiple threads, it looks like
there's a design flaw in the way the WebRtcAudioCapturer and sinks work
together.

Firstly this puts a requirement on the sinks that they need to be thread safe,
which I'm not sure all implementations are aware of.

Secondly, this particular pattern is not guaranteed to give the sinks the
correct audio parameters.  To explain what I mean, take this example:

Thread A enters Reconfigure, allocates a new ConfiguredBuffer, takes the lock,
assigns the new buffer to buffer_ and releases the lock.  Before entering this
loop though, Thread A runs out of quantum.

Thread B runs, enters Reconfigure() does all the steps that A previously did,
but with different audio parameters.  Thread B continues to run and runs through
the loop, letting all the sinks know that buffer_->params() are the new params. 
This is correct now that buffer_ points to those new parameters.

However, now Thread A wakes up and enters the loop.  Its new_buffer however
still holds a reference to the previous buffer and all the sinks will now be
told that the old audio parameters are the current ones.  Oops.

I guess the morale of the story is that we need to make the threading story of
the class better.  Ideally the buffer and params should always be modifed on the
same thread and then we won't have the above problem.

I don't think we need to address this in this CL, but I figured I'd get someone
else to think about it as well :)

[phoglund_chromium, 2013/02/11 09:18:57]

Yeah, the buffer and parameters really need to be delivered together to the
sinks if we are to preserve that invariant. Now it's like we send updates on the
capture format to our sinks, and they expect the buffer in this class to
correspond to the parameters they last saw, but as you demonstrate that isn't
true.

Yeah, we can either require that the sinks are thread-safe OR that they don't
call us back from within a handler (e.g we call them under our lock).

Before we do this though we need to be really sure that this can indeed be
called from different threads. I'm pretty sure but I could be wrong.

+    (*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)) {
       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(
     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();
 }
 
+media::AudioParameters WebRtcAudioCapturer::audio_parameters() {
+  base::AutoLock auto_lock(lock_);
+  DCHECK(buffer_.get() != NULL);

[tommi (sloooow) - chröme, 2013/02/08 20:00:39]

you can remove this. we avoid having (D)CHECKs followed by dereferencing the
checked pointer anyway.

[phoglund_chromium, 2013/02/11 09:18:57]

Done.

+  return buffer_->params();
+}
+
 }  // namespace content