UMA stats for browser/renderer audio rendering buffer size mismatch.

media/audio/audio_output_resampler.cc

Index: media/audio/audio_output_resampler.cc
diff --git a/media/audio/audio_output_resampler.cc b/media/audio/audio_output_resampler.cc
index 54afbe99fde86847835977ddbc4cebd6180c8013..2b70abf5aac63ecf1a03fe5ee990a5ff01127242 100644
--- a/media/audio/audio_output_resampler.cc
+++ b/media/audio/audio_output_resampler.cc
@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/single_thread_task_runner.h"
 #include "base/trace_event/trace_event.h"
@@ -135,6 +136,39 @@ static void RecordFallbackStats(const AudioParameters& output_params) {
   }
 }
 
+// Record UMA statistics for input/output rebuffering.
+static void RecordRebufferingStats(const AudioParameters& input_params,
+                                   const AudioParameters& output_params) {
+  const int& input_buffer_size = input_params.frames_per_buffer();
+  const int& output_buffer_size = output_params.frames_per_buffer();

[Ilya Sherman, 2016/08/24 22:20:23]

nit: An int is generally going to be smaller or equally sized to a pointer, and
have one less level of indirection than a reference.  I'll defer to owners of
this code, but IMO it's generally preferable to copy ints than to have const
references to them ;-)

[o1ka, 2016/08/25 11:48:34]

It's extending lifetime of a temporary rather than having a reference (aka
pointer), so it should not require any additional memory, AFAIK. On the other
hand, "const int" should be optimized by copy elision => does not cause any
copy. I'll leave out references, just to make the code clearer.

+  DCHECK(input_buffer_size);
+  DCHECK(output_buffer_size);
+  int value = (output_buffer_size - input_buffer_size) * 1000 /
+              std::min(input_buffer_size, output_buffer_size);

[Ilya Sherman, 2016/08/24 22:20:23]

What is the possible range for these values?  It's not appropriate to use a
sparse histogram to record a value that might take on many different for a
single client.

[o1ka, 2016/08/25 11:48:34]

For both buffer sizes the range is between 128 and 4096.
A buffer size is usually: (a) a power of 2, or (b) a sample rate
(https://en.wikipedia.org/wiki/Sampling_(signal_processing)#Sampling_rate)
divided by 100 (10 ms buffers) or 50 (20 ms buffers).

So, the range of the values we want to record is [-31, 31] (before scaling), but
we'll see only a subset of integers in that range, and for some of those
integers a very limited subset of non-round values.

I'm not sure what is considered to be a client in this context. We expect that
each (audio hardware + drivers) combination will produce 1 value per histogram
(might be 2-3 in some cases if code paths for the specific audio latency diverge
on renderer side)

[Ilya Sherman, 2016/08/26 00:35:34]

I think you are trying to pack too much into a single histogram.  It would be
great if we could record 2D histograms -- and, actually, if you know the precise
set of inputs, you could just flatten the cross-product.

OTOH, if you're not sure that you know the exact set of inputs, then I'd
recommend recording two histograms: One indicating whether the sizes are
compatible, and another indicating the precise periodicity if they do not match
(or, in all cases, if you prefer).  That would both (a) give you clearer data,
and (b) make me more confident that I can correctly reason about the structure
of the histogram.

+
+  switch (input_params.latency_tag()) {
+    case AudioLatency::LATENCY_EXACT_MS:
+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "Media.Audio.Render.BufferSizeMismatch.LatencyExactMs", value);
+      return;
+    case AudioLatency::LATENCY_INTERACTIVE:
+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "Media.Audio.Render.BufferSizeMismatch.LatencyInteractive", value);
+      return;
+    case AudioLatency::LATENCY_RTC:
+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "Media.Audio.Render.BufferSizeMismatch.LatencyRtc", value);
+      return;
+    case AudioLatency::LATENCY_PLAYBACK:
+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "Media.Audio.Render.BufferSizeMismatch.LatencyPlayback", value);
+      return;
+    default:

[Ilya Sherman, 2016/08/24 22:20:23]

Please explicitly list all possible cases.  AFAICT there are no other valid
cases, so there is no reason to have an additional histogram.

[o1ka, 2016/08/25 11:48:34]

Agree. Done

+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "Media.Audio.Render.BufferSizeMismatch.LatencyOther", value);
+  }
+}
+
 // Converts low latency based |output_params| into high latency appropriate
 // output parameters in error situations.
 void AudioOutputResampler::SetupFallbackParams() {
@@ -354,7 +388,9 @@ OnMoreDataConverter::OnMoreDataConverter(const AudioParameters& input_params,
       audio_converter_(input_params, output_params, false),
       error_occurred_(false),
       input_buffer_size_(input_params.frames_per_buffer()),
-      output_buffer_size_(output_params.frames_per_buffer()) {}
+      output_buffer_size_(output_params.frames_per_buffer()) {
+  RecordRebufferingStats(input_params, output_params);
+}
 
 OnMoreDataConverter::~OnMoreDataConverter() {
   // Ensure Stop() has been called so we don't end up with an AudioOutputStream