Allowing a custom audio buffer size in WebRtcAudioCapturer

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/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "cc/base/math_util.h"
 #include "content/child/child_process.h"
 #include "content/renderer/media/audio_device_factory.h"
 #include "content/renderer/media/media_stream_audio_processor.h"
 #include "content/renderer/media/media_stream_audio_processor_options.h"
 #include "content/renderer/media/media_stream_audio_source.h"
+#include "content/renderer/media/media_stream_constraints_util.h"
 #include "content/renderer/media/webrtc_audio_device_impl.h"
 #include "content/renderer/media/webrtc_local_audio_track.h"
 #include "content/renderer/media/webrtc_logging.h"
 #include "media/audio/sample_rates.h"
 
 namespace content {
 
 namespace {
 
+const char kAudioLatency[] = "latency";

[henrika (OOO until Aug 14), 2015/05/14 15:19:51]

Is it clear from this name that this is for the capture side? Also, note that
the actual latency will be larger since more parts are added in the complete
audio path. You are in fact setting the native buffer size in milliseconds and
not the capture latency. Just FYI. As an example, using 10ms will lead to
different actual delays and the end result will also very between platforms. 

[Charlie, 2015/05/14 16:46:10]

The JS will look like this:

navigator.webkitGetUserMedia(
    {
      'audio' : {
        'optional' : [
          { 'latency' : 0.3 }
        ]
      }
    },
    onAudioReady,
    onAudioError);

We are requesting a audio input MediaStream and specifying the latency we can
tolerate. This is an optional constraint, and since it's processed directly by
the WebRtcAudioCapturer, it will be ignored if specified elsewhere. The w3c
thread seems to be converging on calling this simply "latency":

https://lists.w3.org/Archives/Public/public-media-capture/2015May/0008.html

As for the total latency, it seems that this would depend rather unpredictably
on the client's processing speed. Once the buffer size is set, the rest of the
stream should hand the audio along with the same buffer size unless instructed
otherwise. (Do you know whether this is the case?) So the main source of delay
should be how long the driver accumulates audio before passing it along.

+const double kMinAudioLatency = 0;

[henrika (OOO until Aug 14), 2015/05/14 15:19:51]

Add unit.

[tommi (sloooow) - chröme, 2015/05/14 16:29:26]

It's not clear if this is a buffer size or time value.

[Charlie, 2015/05/14 17:30:52]

Done.

+const double kMaxAudioLatency = 60;
+
+const int kDontUseBufferSizeParameter = 0;

[henrika (OOO until Aug 14), 2015/05/14 15:19:51]

Is it used?

[tommi (sloooow) - chröme, 2015/05/14 16:29:26]

From the name, it sounds like this should be a bool but the type is int, which
is confusing.  What about kInvalidBufferSize?  Or perhaps this constant isn't
necessary?  I think I would prefer to see a 0 in the code in order to understand
what's being compared instead of a constant name since I would probably always
go and check what the magical value would be.

[Charlie, 2015/05/14 16:46:10]

I think Henrik was suggesting that we "tag" the zero with the constant name to
explain what is going on. But I agree that an explicit zero with a comment seems
better.

[Charlie, 2015/05/14 17:30:52]

Done.

+
 // Method to check if any of the data in |audio_source| has energy.
 bool HasDataEnergy(const media::AudioBus& audio_source) {
   for (int ch = 0; ch < audio_source.channels(); ++ch) {
     const float* channel_ptr = audio_source.channel(ch);
     for (int frame = 0; frame < audio_source.frames(); ++frame) {
       if (channel_ptr[frame] != 0)
         return true;
     }
   }
 
@@ skipping 46 matching lines @@
 
     // This can be reentrant so reset |delegate_| before calling out.
     WebRtcLocalAudioTrack* temp = delegate_;
     delegate_ = NULL;
     temp->Stop();
   }
 
   // Wrapper which allows to use std::find_if() when adding and removing
   // sinks to/from the list.
   struct TrackWrapper {
-    TrackWrapper(WebRtcLocalAudioTrack* track) : track_(track) {}
+    explicit TrackWrapper(WebRtcLocalAudioTrack* track) : track_(track) {}
     bool operator()(
         const scoped_refptr<WebRtcAudioCapturer::TrackOwner>& owner) const {
       return owner->IsEqual(track_);
     }
     WebRtcLocalAudioTrack* track_;
   };
 
  protected:
   virtual ~TrackOwner() {}
 
@@ skipping 78 matching lines @@
 
   // Verify that the reported input channel configuration is supported.
   if (channel_layout != media::CHANNEL_LAYOUT_MONO &&
       channel_layout != media::CHANNEL_LAYOUT_STEREO &&
       channel_layout != media::CHANNEL_LAYOUT_STEREO_AND_KEYBOARD_MIC) {
     DLOG(ERROR) << channel_layout
                 << " is not a supported input channel configuration.";
     return false;
   }
 
+  const int& sample_rate = device_info_.device.input.sample_rate;
   DVLOG(1) << "Audio input hardware sample rate: "
            << device_info_.device.input.sample_rate;
   media::AudioSampleRate asr;
-  if (media::ToAudioSampleRate(device_info_.device.input.sample_rate, &asr)) {
+  if (media::ToAudioSampleRate(sample_rate, &asr)) {

[tommi (sloooow) - chröme, 2015/05/14 16:29:26]

nit: to keep the change simple, can you revert the cosmetic changes?

[Charlie, 2015/05/14 17:30:52]

Done.

     UMA_HISTOGRAM_ENUMERATION(
         "WebRTC.AudioInputSampleRate", asr, media::kAudioSampleRateMax + 1);
   } else {
-    UMA_HISTOGRAM_COUNTS("WebRTC.AudioInputSampleRateUnexpected",
-                         device_info_.device.input.sample_rate);
+    UMA_HISTOGRAM_COUNTS("WebRTC.AudioInputSampleRateUnexpected", sample_rate);
   }
 
+  double buffer_size_seconds = 0;

[henrika (OOO until Aug 14), 2015/05/14 15:19:51]

is it really in seconds!? Should it not be in milliseconds or do you allow a 60
seconds long delay?

[Charlie, 2015/05/14 16:46:10]

I can drop the max to 10 seconds if you want. I don't see much harm in allowing
that.

+  GetConstraintValueAsDouble(
+      constraints_, kAudioLatency, &buffer_size_seconds);

[tommi (sloooow) - chröme, 2015/05/14 16:29:26]

seconds doesn't feel right. also I think you need to check the return value of
this call.  If the call succeeds and the buffer size is not within the valid
range, I would prefer to treat that as the buffer size not being set instead of
clamping.

[Charlie, 2015/05/14 16:46:10]

On the w3c thread it was pointed out that milliseconds aren't really used
anywhere else. I also don't like seconds as a unit here, though.

Actually the logic already does what you want if this value isn't set. Since
it's initialized to zero, the value will stay there. And zero is our magic value
that says we need to figure out the buffer size for ourselves. Setting an
explicit minimum (like 10ms) wouldn't preserve the current behavior, since right
now we use the hardware buffer size if it is smaller.

[Charlie, 2015/05/14 17:30:52]

Fixed to ignore out-of-range values.

+  buffer_size_seconds = cc::MathUtil::ClampToRange(

[tommi (sloooow) - chröme, 2015/05/14 16:29:25]

with the above, I think that would also remove the dependency on MathUtil?

[Charlie, 2015/05/14 16:46:10]

Well, we still need to clamp the value if the user specifies something out of
range. Also I think this isn't a binary dependency, since the function is a
template that just calls std::min and std::max.

+      buffer_size_seconds, kMinAudioLatency, kMaxAudioLatency);
+  int buffer_size = sample_rate * buffer_size_seconds;
+  if (buffer_size > 0)

[Charlie, 2015/05/14 17:30:52]

Done.

+    DVLOG(1) << "Custom audio buffer size: " << buffer_size;
+
   // Create and configure the default audio capturing source.
   SetCapturerSourceInternal(
-      AudioDeviceFactory::NewInputDevice(render_frame_id_), channel_layout,
-      static_cast<float>(device_info_.device.input.sample_rate));
+      AudioDeviceFactory::NewInputDevice(render_frame_id_),
+      channel_layout,
+      sample_rate,
+      buffer_size);
 
   // Add the capturer to the WebRtcAudioDeviceImpl since it needs some hardware
   // information from the capturer.
   if (audio_device_)
     audio_device_->AddAudioCapturer(this);
 
   return true;
 }
 
 WebRtcAudioCapturer::WebRtcAudioCapturer(
@@ skipping 64 matching lines @@
     // we have to call StopSource on the MediaStreamSource. This will call
     // MediaStreamAudioSource::DoStopSource which in turn call
     // WebRtcAudioCapturerer::Stop();
     audio_source_->StopSource();
   }
 }
 
 void WebRtcAudioCapturer::SetCapturerSourceInternal(
     const scoped_refptr<media::AudioCapturerSource>& source,
     media::ChannelLayout channel_layout,
-    float sample_rate) {
+    int sample_rate,
+    int buffer_size) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) << "SetCapturerSource(channel_layout=" << channel_layout << ","
            << "sample_rate=" << sample_rate << ")";
   scoped_refptr<media::AudioCapturerSource> old_source;
   {
     base::AutoLock auto_lock(lock_);
     if (source_.get() == source.get())
       return;
 
     source_.swap(old_source);
     source_ = source;
 
     // Reset the flag to allow starting the new source.
     running_ = false;
   }
 
   DVLOG(1) << "Switching to a new capture source.";
   if (old_source.get())
     old_source->Stop();
 
   // Dispatch the new parameters both to the sink(s) and to the new source,
   // also apply the new |constraints|.
   // The idea is to get rid of any dependency of the microphone parameters
   // which would normally be used by default.
   // bits_per_sample is always 16 for now.
-  int buffer_size = GetBufferSize(sample_rate);
+  if (buffer_size == kDontUseBufferSizeParameter)

[henrika (OOO until Aug 14), 2015/05/14 15:19:51]

Make a clear comment about what happens here and what controls this change.

[Charlie, 2015/05/14 17:30:53]

Done.

+    buffer_size = GetBufferSize(sample_rate);
   media::AudioParameters params(media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
-                                channel_layout, sample_rate,
-                                16, buffer_size,
+                                channel_layout,
+                                sample_rate,
+                                16,
+                                buffer_size,
                                 device_info_.device.input.effects);
 
   {
     base::AutoLock auto_lock(lock_);
     // Notify the |audio_processor_| of the new format.
     audio_processor_->OnCaptureFormatChanged(params);
 
     // Notify all tracks about the new format.
     tracks_.TagAll();
   }
@@ skipping 28 matching lines @@
   // Do nothing if the current buffer size is the WebRtc native buffer size.
   if (GetBufferSize(input_params.sample_rate()) ==
           input_params.frames_per_buffer()) {
     return;
   }
 
   // Create a new audio stream as source which will open the hardware using
   // WebRtc native buffer size.
   SetCapturerSourceInternal(AudioDeviceFactory::NewInputDevice(render_frame_id),
                             input_params.channel_layout(),
-                            static_cast<float>(input_params.sample_rate()));
+                            input_params.sample_rate(),
+                            0);
 }
 
 void WebRtcAudioCapturer::Start() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) << "WebRtcAudioCapturer::Start()";
   base::AutoLock auto_lock(lock_);
   if (running_ || !source_.get())
     return;
 
   // Start the data source, i.e., start capturing data from the current source.
@@ skipping 202 matching lines @@
     return hardware_buffer_size;
   }
 
   return (sample_rate / 100);
 }
 
 void WebRtcAudioCapturer::SetCapturerSource(
     const scoped_refptr<media::AudioCapturerSource>& source,
     media::AudioParameters params) {
   // Create a new audio stream as source which uses the new source.
-  SetCapturerSourceInternal(source, params.channel_layout(),
-                            static_cast<float>(params.sample_rate()));
+  SetCapturerSourceInternal(source,
+                            params.channel_layout(),
+                            params.sample_rate(),
+                            0);
 }
 
 }  // namespace content