Add histograms for Biquad lowpass and highpass Q values

third_party/WebKit/Source/modules/webaudio/AudioParam.cpp

Index: third_party/WebKit/Source/modules/webaudio/AudioParam.cpp
diff --git a/third_party/WebKit/Source/modules/webaudio/AudioParam.cpp b/third_party/WebKit/Source/modules/webaudio/AudioParam.cpp
index ab6894a96fabaae33bd7554d04093c22a1489831..f5486db7a87f438dcfb8313be680cc3e2fea86e2 100644
--- a/third_party/WebKit/Source/modules/webaudio/AudioParam.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/AudioParam.cpp
@@ -27,6 +27,7 @@
 #include "modules/webaudio/AudioNode.h"
 #include "modules/webaudio/AudioNodeOutput.h"
 #include "platform/FloatConversion.h"
+#include "platform/Histogram.h"
 #include "platform/audio/AudioUtilities.h"
 #include "wtf/MathExtras.h"
 
@@ -40,6 +41,11 @@ AudioDestinationHandler& AudioParamHandler::destinationHandler() const
     return *m_destinationHandler;
 }
 
+void AudioParamHandler::setParamType(AudioParamType paramType)
+{
+    m_paramType = paramType;
+}
+
 String AudioParamHandler::getParamName() const
 {
     // The returned string should be the name of the node and the name of the AudioParam for
@@ -52,6 +58,10 @@ String AudioParamHandler::getParamName() const
     case ParamTypeBiquadFilterFrequency:
         return "BiquadFilter.frequency";
     case ParamTypeBiquadFilterQ:
+    case ParamTypeBiquadFilterQLowpass:
+    case ParamTypeBiquadFilterQHighpass:
+        // We don't really need separate names for the Q parameter for lowpass and highpass filters.
+        // The difference is only for the histograms.
         return "BiquadFilter.Q";
     case ParamTypeBiquadFilterGain:
         return "BiquadFilter.gain";
@@ -102,6 +112,7 @@ float AudioParamHandler::value()
 void AudioParamHandler::setValue(float value)
 {
     setIntrinsicValue(value);
+    updateHistograms(value);
 }
 
 float AudioParamHandler::smoothedValue()
@@ -234,6 +245,41 @@ void AudioParamHandler::disconnect(AudioNodeOutput& output)
     }
 }
 
+void AudioParamHandler::updateHistograms(float newValue)
+{
+    switch (m_paramType) {
+    case ParamTypeBiquadFilterQLowpass:
+        {
+            // The Q parameter of a lowpass and highpass BiquadFilter is in dB. Let's assume a
+            // useful range is [0, 50].  And also assume 0.01 dB resolution is good enough.  Then,
+            // we can map the floating point Q value to an integer just by multipling by 100.  So
+            // the max value is 5000.
+            DEFINE_STATIC_LOCAL(
+                EnumerationHistogram, lowpassQHistogram,
+                ("WebAudio.BiquadFilter.Q.Lowpass", 5000));

[Raymond Toy, 2016/05/06 17:07:34]

Should the limit here be 5000 or 5001?

Is EnumerationHistogram the right thing here?  The histograms are updated on the
main thread so speed is probably not required.  We also don't have a feel for
the Q values, but the default Q of 1 is probably quite common?

[Mark P, 2016/05/06 23:58:34]

Probably neither, see below.
I doubt it.  This creates a histogram with 5000 buckets.  Each bucket takes up
memory.  Mostly we try to get people to use histograms with <=100 buckets.

Consider how much representation you want.  Can you simply round Q to an int and
record that?

If you think Q values don't change that much for an individual user (i.e., each
user will only see a handful of them over the time between browser restarts),
you can use a sparse histogram.  A sparse histogram will allow more buckets
(created on demand) so you can do something like 100 * Q.

[Raymond Toy, 2016/05/09 17:17:09]

Rounding Q to an int is probably ok.  Perhaps I'll change the range from [0, 50]
to [0, 25] (50 dB is HUGE).  100 entries would give 0.25 dB steps, which is more
than adequate.
I don't know what the actual range of Q values are; and we don't know how often
users change Q.  

+            newValue = clampTo(newValue, 0, 50);
+            int histogramValue = static_cast<int>(100 * newValue);
+            lowpassQHistogram.count(histogramValue);
+        }
+        break;
+    case ParamTypeBiquadFilterQHighpass:
+        {
+            // The histogram for the highpass filter has the same constraints as for the lowpass
+            // filter above.
+            DEFINE_STATIC_LOCAL(
+                EnumerationHistogram, highpassQHistogram,
+                ("WebAudio.BiquadFilter.Q.Highpass", 5000));
+            newValue = clampTo(newValue, 0, 50);
+            int histogramValue = static_cast<int>(100 * newValue);
+            highpassQHistogram.count(histogramValue);
+        }
+        break;
+    default:
+        // Nothing to do for all other types.
+        break;
+    }
+}
+
 // ----------------------------------------------------------------
 
 AudioParam::AudioParam(AbstractAudioContext& context, AudioParamType paramType, double defaultValue)
@@ -267,9 +313,15 @@ float AudioParam::defaultValue() const
     return handler().defaultValue();
 }
 
+void AudioParam::setParamType(AudioParamType paramType)
+{
+    handler().setParamType(paramType);
+}
+
 AudioParam* AudioParam::setValueAtTime(float value, double time, ExceptionState& exceptionState)
 {
     handler().timeline().setValueAtTime(value, time, exceptionState);
+    handler().updateHistograms(value);
     return this;
 }
 
@@ -277,24 +329,41 @@ AudioParam* AudioParam::linearRampToValueAtTime(float value, double time, Except
 {
     handler().timeline().linearRampToValueAtTime(
         value, time, handler().intrinsicValue(), context()->currentTime(), exceptionState);
+
+    // This is probably the best we can do for the histogram.  We don't want to run the automation
+    // to get all the values and use them to update the histogram.
+    handler().updateHistograms(value);
+
     return this;
 }
 
 AudioParam* AudioParam::exponentialRampToValueAtTime(float value, double time, ExceptionState& exceptionState)
 {
     handler().timeline().exponentialRampToValueAtTime(value, time, handler().intrinsicValue(), context()->currentTime(), exceptionState);
+
+    // This is probably the best we can do for the histogram.  We don't want to run the automation
+    // to get all the values and use them to update the histogram.
+    handler().updateHistograms(value);
+
     return this;
 }
 
 AudioParam* AudioParam::setTargetAtTime(float target, double time, double timeConstant, ExceptionState& exceptionState)
 {
     handler().timeline().setTargetAtTime(target, time, timeConstant, exceptionState);
+
+    // Don't update the histogram here.  It's not clear in normal usage if the parameter value will
+    // actually reach |target|.
     return this;
 }
 
 AudioParam* AudioParam::setValueCurveAtTime(DOMFloat32Array* curve, double time, double duration, ExceptionState& exceptionState)
 {
     handler().timeline().setValueCurveAtTime(curve, time, duration, exceptionState);
+
+    // We could update the histogram with every value in the curve, due to interpolation, we'll
+    // probably be missing many values.  So we don't update the histogram.  setValueCurveAtTime is
+    // probably a fairly rare method anyway.
     return this;
 }