Add histograms for Biquad lowpass and highpass Q values

Add histograms for Biquad lowpass and highpass Q values

This is to measure the Q values in common use so we can determine how
much impact there will be if the BiquadFilter changes the internal
formulas for the lowpass and highpass filters.

BUG=607733
TEST=none

Committed: https://crrev.com/2d01806ab92e8a360d5dd468cec1d4e91a79d6c6
Cr-Commit-Position: refs/heads/master@{#392643}

[Raymond Toy, 2016-04-29 19:45:45]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2016-04-29 19:45:46]

PTAL at the general approach.  Of course, this depends on getting the names into
the AudioParams to begin with, which is a different CL that you've been copied
on.  Thus, lots of duplicated code here.

[Raymond Toy, 2016-05-04 15:43:54]

On 2016/04/29 19:45:46, Raymond Toy wrote:
> PTAL at the general approach.  Of course, this depends on getting the names
into
> the AudioParams to begin with, which is a different CL that you've been copied
> on.  Thus, lots of duplicated code here.

Ping

[hongchan, 2016-05-04 16:38:32]

It seems a bit weird to me that you have LowPass/HighPass after Q. Also we're
making special cases only for high/lowpass.

I hope we could have a dedicated tracker class somewhere, but I don't know if
you're willing to do that. I am just worried we are building a special case on
top of special cases.

[Raymond Toy, 2016-05-04 17:09:23]

On 2016/05/04 16:38:32, hoch wrote:
> It seems a bit weird to me that you have LowPass/HighPass after Q. Also we're
> making special cases only for high/lowpass.

I originally had BiquadFilterLowpassQ, but it's really the Q audioparam so I
renamed it to BiquadFilterQLowpass to make it clearer that this is the Q param
for the lowpass filter.

> 
> I hope we could have a dedicated tracker class somewhere, but I don't know if
> you're willing to do that. I am just worried we are building a special case on
> top of special cases.

For AudioParams I don't know how else to do this.  And I don't ever expect to do
metrics on any other AudioParam.  So I don't want to think about a general
approach that will never get used, and I fully expect these metrics to go away
after some time (if possible?)

[hongchan, 2016-05-06 16:50:13]

On 2016/05/04 17:09:23, Raymond Toy wrote:
> On 2016/05/04 16:38:32, hoch wrote:
> > It seems a bit weird to me that you have LowPass/HighPass after Q. Also
we're
> > making special cases only for high/lowpass.
> 
> I originally had BiquadFilterLowpassQ, but it's really the Q audioparam so I
> renamed it to BiquadFilterQLowpass to make it clearer that this is the Q param
> for the lowpass filter.
> 
> > 
> > I hope we could have a dedicated tracker class somewhere, but I don't know
if
> > you're willing to do that. I am just worried we are building a special case
on
> > top of special cases.
> 
> For AudioParams I don't know how else to do this.  And I don't ever expect to
do
> metrics on any other AudioParam.  So I don't want to think about a general
> approach that will never get used, and I fully expect these metrics to go away
> after some time (if possible?)

So, what's our decision on this? Stamp and ship this out first and refine later?

[Raymond Toy, 2016-05-06 16:51:03]

On 2016/05/06 16:50:13, hoch wrote:
> On 2016/05/04 17:09:23, Raymond Toy wrote:
> > On 2016/05/04 16:38:32, hoch wrote:
> > > It seems a bit weird to me that you have LowPass/HighPass after Q. Also
> we're
> > > making special cases only for high/lowpass.
> > 
> > I originally had BiquadFilterLowpassQ, but it's really the Q audioparam so I
> > renamed it to BiquadFilterQLowpass to make it clearer that this is the Q
param
> > for the lowpass filter.
> > 
> > > 
> > > I hope we could have a dedicated tracker class somewhere, but I don't know
> if
> > > you're willing to do that. I am just worried we are building a special
case
> on
> > > top of special cases.
> > 
> > For AudioParams I don't know how else to do this.  And I don't ever expect
to
> do
> > metrics on any other AudioParam.  So I don't want to think about a general
> > approach that will never get used, and I fully expect these metrics to go
away
> > after some time (if possible?)
> 
> So, what's our decision on this? Stamp and ship this out first and refine
later?

As the reviewer, you have final say.

[hongchan, 2016-05-06 16:52:41]

I guess we collect information for a period of time (say, 3 month) with this
patch, but we should work on a more comprehensive way of data collection.

lgtm

[hongchan, 2016-05-06 16:53:15]

On 2016/05/06 16:52:41, hoch wrote:
> I guess we collect information for a period of time (say, 3 month) with this
> patch, but we should work on a more comprehensive way of data collection.
> 
> lgtm

Could you please add TODO comment or a crbug entry?

[Raymond Toy, 2016-05-06 16:56:39]

On 2016/05/06 16:53:15, hoch wrote:
> On 2016/05/06 16:52:41, hoch wrote:
> > I guess we collect information for a period of time (say, 3 month) with this
> > patch, but we should work on a more comprehensive way of data collection.
> > 
> > lgtm
> 
> Could you please add TODO comment or a crbug entry?

crbug.com/609859

[blink-reviews, 2016-05-06 17:05:23]

Thanks!

On Fri, May 6, 2016 at 9:56 AM rtoy@chromium.org via codereview.chromium.org
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/06 16:53:15, hoch wrote:
> > On 2016/05/06 16:52:41, hoch wrote:
> > > I guess we collect information for a period of time (say, 3 month)
> with this
> > > patch, but we should work on a more comprehensive way of data
> collection.
> > >
> > > lgtm
> >
> > Could you please add TODO comment or a crbug entry?
>
> crbug.com/609859
>
> https://codereview.chromium.org/1934683002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-05-06 17:05:24]

Thanks!

On Fri, May 6, 2016 at 9:56 AM rtoy@chromium.org via codereview.chromium.org
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/06 16:53:15, hoch wrote:
> > On 2016/05/06 16:52:41, hoch wrote:
> > > I guess we collect information for a period of time (say, 3 month)
> with this
> > > patch, but we should work on a more comprehensive way of data
> collection.
> > >
> > > lgtm
> >
> > Could you please add TODO comment or a crbug entry?
>
> crbug.com/609859
>
> https://codereview.chromium.org/1934683002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Raymond Toy, 2016-05-06 17:07:33]

rtoy@chromium.org changed reviewers:
+ mpearson@chromium.org

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

mpearson: PTAL at the histograms.

https://codereview.chromium.org/1934683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/1934683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:259:
("WebAudio.BiquadFilter.Q.Lowpass", 5000));
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]

https://codereview.chromium.org/1934683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/1934683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:259:
("WebAudio.BiquadFilter.Q.Lowpass", 5000));
On 2016/05/06 17:07:34, Raymond Toy wrote:
> Should the limit here be 5000 or 5001?

Probably neither, see below.
> 
> Is EnumerationHistogram the right thing here?

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.

> 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?

https://codereview.chromium.org/1934683002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1934683002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58393: +    linearRampToValueAtTime,
exponentialValueAtTime, and setValueAtTime.
Is it appropriate to mix these automated methods with the .value setter method? 
i.e., is this mix of data from conceptually different sources useful?

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

https://codereview.chromium.org/1934683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/1934683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:259:
("WebAudio.BiquadFilter.Q.Lowpass", 5000));
On 2016/05/06 23:58:34, Mark P wrote:
> On 2016/05/06 17:07:34, Raymond Toy wrote:
> > Should the limit here be 5000 or 5001?
> 
> Probably neither, see below.
> > 
> > Is EnumerationHistogram the right thing here?
> 
> 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?

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.

> 
> 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.

I don't know what the actual range of Q values are; and we don't know how often
users change Q.  
> 
> > 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?

https://codereview.chromium.org/1934683002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1934683002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58393: +    linearRampToValueAtTime,
exponentialValueAtTime, and setValueAtTime.
On 2016/05/06 23:58:34, Mark P wrote:
> Is it appropriate to mix these automated methods with the .value setter
method? 
> i.e., is this mix of data from conceptually different sources useful?
> 

I believe so.  These methods just automate what the user would have to do
manually to set Q values.  Ideally, we would want all Q values, but I think that
is too much data.  An automation method would produce 128 values every 3 ms or
so.  As a compromise, we just take the value given when the automation method is
called.  At least the user intended to use it at some time, even if it isn't
actually. (Automations are scheduled in the future and can be overridden or
cancelled.)

[Mark P, 2016-05-09 19:37:17]

You have replied to my comments.  It sounds like you meant to update a new
patchset but appears you forgot to do so.

Let me know if I'm wrong.

--mark

[Raymond Toy, 2016-05-09 21:23:33]

On 2016/05/09 19:37:17, Mark P wrote:
> You have replied to my comments.  It sounds like you meant to update a new
> patchset but appears you forgot to do so.
> 
> Let me know if I'm wrong.
> 
> --mark

Yeah, I didn't upload anything because I was in the middle of a different CL,
but there's a new  patch now.  I changed things to use a SparseHistogram using
at most 100 buckets.

[Mark P, 2016-05-09 21:36:25]

On 2016/05/09 21:23:33, Raymond Toy wrote:
> On 2016/05/09 19:37:17, Mark P wrote:
> > You have replied to my comments.  It sounds like you meant to update a new
> > patchset but appears you forgot to do so.
> > 
> > Let me know if I'm wrong.
> > 
> > --mark
> 
> Yeah, I didn't upload anything because I was in the middle of a different CL,
> but there's a new  patch now.  I changed things to use a SparseHistogram using
> at most 100 buckets.

I know you said
"The histograms are updated on the main thread so speed is probably not
required."
Yet you also say,
"An automation method would produce 128 values every 3 ms or so."
This sounds like a lot of emits to the histogram, which makes me think speed is
important.

Sparse histograms are slow.  (They acquire a lock each time.)

You may want to reconsider.

(It looks like your histogram would have 100 buckets at your desired resolution.
 100 buckets is fine.)

--mark

[Raymond Toy, 2016-05-09 21:40:10]

On 2016/05/09 21:36:25, Mark P wrote:
> On 2016/05/09 21:23:33, Raymond Toy wrote:
> > On 2016/05/09 19:37:17, Mark P wrote:
> > > You have replied to my comments.  It sounds like you meant to update a new
> > > patchset but appears you forgot to do so.
> > > 
> > > Let me know if I'm wrong.
> > > 
> > > --mark
> > 
> > Yeah, I didn't upload anything because I was in the middle of a different
CL,
> > but there's a new  patch now.  I changed things to use a SparseHistogram
using
> > at most 100 buckets.
> 
> I know you said
> "The histograms are updated on the main thread so speed is probably not
> required."
> Yet you also say,
> "An automation method would produce 128 values every 3 ms or so."
> This sounds like a lot of emits to the histogram, which makes me think speed
is
> important.

I think I was not clear on this.  I meant if we did want to record Q values for
all of the automations, we would have 128 values every 3 ms.  But the code is
speed critical so we don't want to do that.  Instead, at the call to
linearRampToValueAtTime(), we'll just grab the specified Q value.  This happens
only on the main thread and only when the user calls it.

It's a trade-off between getting all of the data, and just some indication.

> 
> Sparse histograms are slow.  (They acquire a lock each time.)
> 
> You may want to reconsider.
> 
> (It looks like your histogram would have 100 buckets at your desired
resolution.
>  100 buckets is fine.)
> 
> --mark

[Mark P, 2016-05-09 21:48:33]

histograms.xml lgtm

https://codereview.chromium.org/1934683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/1934683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:267: int
histogramValue = static_cast<int>(4 * newValue + 0.5);
I don't know WebKit style but, for the record, this kind of thing looks odd to
me, where you declare a variable to only use it once in the next line (ditto
elsewhere).  Plus comment line length.

[Raymond Toy, 2016-05-09 21:51:55]

On 2016/05/09 21:48:33, Mark P wrote:
> histograms.xml lgtm
> 
>
https://codereview.chromium.org/1934683002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):
> 
>
https://codereview.chromium.org/1934683002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:267: int
> histogramValue = static_cast<int>(4 * newValue + 0.5);
> I don't know WebKit style but, for the record, this kind of thing looks odd to
> me, where you declare a variable to only use it once in the next line (ditto
> elsewhere).  Plus comment line length.

Yeah, I can fix the single-use issue.  Don't think blink style mentions this. 
But blink style currently allows infinite line length for code and comments.  I
usually stop at about 120 or so.

[Raymond Toy, 2016-05-10 16:33:46]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2016-05-10 16:33:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1934683002/#ps80001
(title: "Remove variables used only once.")

[commit-bot: I haz the power, 2016-05-10 16:34:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1934683002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1934683002/80001

[commit-bot: I haz the power, 2016-05-10 18:00:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-10 18:01:43]

Description was changed from

==========
Add histograms for Biquad lowpass and highpass Q values

This is to measure the Q values in common use so we can determine how
much impact there will be if the BiquadFilter changes the internal
formulas for the lowpass and highpass filters.

BUG=607733
TEST=none
==========

to

==========
Add histograms for Biquad lowpass and highpass Q values

This is to measure the Q values in common use so we can determine how
much impact there will be if the BiquadFilter changes the internal
formulas for the lowpass and highpass filters.

BUG=607733
TEST=none

Committed: https://crrev.com/2d01806ab92e8a360d5dd468cec1d4e91a79d6c6
Cr-Commit-Position: refs/heads/master@{#392643}
==========

[commit-bot: I haz the power, 2016-05-10 18:01:44]

Patchset 5 (id:??) landed as
https://crrev.com/2d01806ab92e8a360d5dd468cec1d4e91a79d6c6
Cr-Commit-Position: refs/heads/master@{#392643}