Allowing a custom audio buffer size in WebRtcAudioCapturer

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 c59695a72f6a97f7651004220eabdc9e801a3061..2f8b52ddab7963f875266a8cefa36a06851d2c52 100644
--- a/content/renderer/media/webrtc_audio_capturer.cc
+++ b/content/renderer/media/webrtc_audio_capturer.cc
@@ -9,11 +9,13 @@
 #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"
@@ -23,6 +25,12 @@ 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) {
@@ -89,7 +97,7 @@ class WebRtcAudioCapturer::TrackOwner
   // 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_);
@@ -188,21 +196,32 @@ bool WebRtcAudioCapturer::Initialize() {
     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.
@@ -287,7 +306,8 @@ void WebRtcAudioCapturer::RemoveTrack(WebRtcLocalAudioTrack* track) {
 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 << ")";
@@ -313,10 +333,13 @@ void WebRtcAudioCapturer::SetCapturerSourceInternal(
   // 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);
 
   {
@@ -365,7 +388,8 @@ void WebRtcAudioCapturer::EnablePeerConnectionMode() {
   // 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() {
@@ -588,8 +612,10 @@ 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