Possible solution to synchronization problems in webrtc audio capturer.

content/renderer/media/webrtc_audio_capturer.cc

Index: content/renderer/media/webrtc_audio_capturer.cc
diff --git a/content/renderer/media/webrtc_audio_capturer.cc b/content/renderer/media/webrtc_audio_capturer.cc
index 976b9edd2ecf3705fc5b2749ed4902b03c9f10bd..16b48dea299f9c77d9181cdd8c5b08bd7d0515f5 100644
--- a/content/renderer/media/webrtc_audio_capturer.cc
+++ b/content/renderer/media/webrtc_audio_capturer.cc
@@ -57,12 +57,56 @@ static int GetBufferSizeForSampleRate(int sample_rate) {
   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());
@@ -98,18 +142,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()
@@ -162,6 +196,8 @@ void WebRtcAudioCapturer::SetCapturerSource(
   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)
@@ -169,10 +205,10 @@ void WebRtcAudioCapturer::SetCapturerSource(
 
     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
@@ -185,26 +221,8 @@ 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_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)
@@ -352,19 +370,25 @@ bool WebRtcAudioCapturer::IsInLoopbackMode() {
   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.
@@ -381,15 +405,14 @@ void WebRtcAudioCapturer::Capture(media::AudioBus* audio_source,
 
   // 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);
   }