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..db9817ddd259933b47a3de67b9b3291f1ca2d52b 100644
--- a/content/renderer/media/webrtc_audio_capturer.cc
+++ b/content/renderer/media/webrtc_audio_capturer.cc
@@ -57,12 +57,73 @@ static int GetBufferSizeForSampleRate(int sample_rate) {
   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());
@@ -98,18 +159,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 +213,7 @@ void WebRtcAudioCapturer::SetCapturerSource(
   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)
@@ -169,9 +221,10 @@ void WebRtcAudioCapturer::SetCapturerSource(
 
     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
@@ -185,30 +238,20 @@ 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_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(
@@ -233,8 +276,9 @@ void WebRtcAudioCapturer::PrepareLoopback() {
   // 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;
 }
 
@@ -359,12 +403,16 @@ void WebRtcAudioCapturer::Capture(media::AudioBus* audio_source,
   // |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.
@@ -379,17 +427,19 @@ void WebRtcAudioCapturer::Capture(media::AudioBus* audio_source,
     }
   }
 
+  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);
   }
@@ -424,4 +474,10 @@ void WebRtcAudioCapturer::OnDeviceStopped() {
     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