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;
 }
 
+class CaptureBuffer : public base::RefCounted<CaptureBuffer> {
+ public:
+  CaptureBuffer(int16* buffer): buffer_(buffer) {}
+  int16* buffer() const { return buffer_.get(); }
+
+ private:
+  scoped_array<int16> buffer_;

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

we're deprecating scoped_array and use scoped_ptr instead.

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

Done.

+};

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

I don't see the audio parameters here.  I think they need to go with the buffer.

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

We could do that, but in that case you could argue to also put the sinks there.
I guess it depends on how much we think the buffer is related to the parameters.

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

The buffer is allocated based on specs from the parameters and any processing
done on the buffer must be done in accordance with how the parameters specify
the buffer.  As soon as the parameters change, the buffer has to be discarded. 
The parameters themselves are then useless without a buffer to go with them, so
I think they're practically married :)

The sink list on the other hand can outlive multiple parameters just as long as
they're notified of change, so I don't think they could live here.

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

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

I was actually thinking that we'd move much of this logic into the CaptureBuffer
class.  So, this function would look closer to:

scoped_refptr<CaptureBuffer> new_buffer(new CaptureBuffer());
if (!new_buffer->Initialize())  // Initialize() checks the buffer size etc.
  return false;

SinkList sinks;
{
  base::AutoLock auto_lock(lock_);
  buffer_ = new_buffer;
  sinks = sinks_;
}

for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it)
  (*it)->SetCaptureFormat(new_buffer->params());

return true;


Also... is Reconfigure() always called on the same thread?
If so, then it would be good to add this to the top of the function:
DCHECK(thread_checker_.CalledOnValidThread());

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

Done. I think we can here from a different thread when resetting the capturer
source.

+  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.
+  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_);
+
+    params_.Reset(format, channel_layout, 0, sample_rate, bits_per_sample,
+                  buffer_size);
+    buffer_ = new CaptureBuffer(
+        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 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;
+  media::AudioParameters::Format current_format;
+  bool no_default_audio_source_exists;
   {
     base::AutoLock auto_lock(lock_);
     if (source_ == source)
       return;
 
     source_.swap(old_source);
     source_ = source;
+    current_format = params_.format();
+    no_default_audio_source_exists = !buffer_.get()->buffer();

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

.get()-> should not be necessary. instead just use ->

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

Done.

   }
 
-  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,
-                  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);
 }
 
 void WebRtcAudioCapturer::SetStopCallback(
     const base::Closure& on_device_stopped_cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) <<  "WebRtcAudioCapturer::SetStopCallback()";
@@ skipping 127 matching lines @@
   if (source_)
     source_->SetAutomaticGainControl(enable);
 }
 
 bool WebRtcAudioCapturer::IsInLoopbackMode() {
   DCHECK(thread_checker_.CalledOnValidThread());
   base::AutoLock auto_lock(lock_);
   return (loopback_fifo_ != NULL);
 }
 
+// This callback is driven by AudioInputDevice::AudioThreadCallback if
+// |source_| is AudioInputDevice, otherwise it is driven by client's
+// CaptureCallback.

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

We generally keep comments like this inside the implementation (where it was). 
Otherwise the comment would be in the header file.

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

Done.

 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<CaptureBuffer> buffer_ref_while_calling;
+  int bytes_per_sample;
   {
     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.
+    bytes_per_sample = params_.bits_per_sample() / 8;

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

if params_ is owned by the CaptureBuffer, then you can do this outside of the
lock, below.

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

Done.

+    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_) {

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

should this block of code be moved out of the lock scope?

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

I have no idea, so I'll just leave it for now. It probably can.

       if (loopback_fifo_->frames() + audio_source->frames() <=
           loopback_fifo_->max_frames()) {
         loopback_fifo_->Push(audio_source);
       } else {
         DVLOG(1) << "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_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 15 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