Bug 77666 – AudioParam needs tests for the parameter automation routines.

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

There should be tests to verify the automation routines for AudioParam, including setValueAtTime, linearRampToValueAtTime, exponentialRampToValueAtTime, setTargetValueAtTime, setValueCurveAtTime, and cancelScheduledValues.

[Raymond Toy]

Created attachment 128538 [details]
Patch

[Chris Rogers]

Comment on attachment 128538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128538&action=review

Overall is looking good - added some comments:

> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
> +                        unsigned curveIndex = static_cast<unsigned>(round(curveVirtualIndex));

I appreciate using common functions in MathExtras such as round(), but this is inside a tight loop that we want as fast as possible so it's not ideal.  Can we define a tighter inline function for this?

> LayoutTests/webaudio/audioparam-setValueAtTime.html:33
> +// For setValueAtTime(), we don't need to do anything for automation.

nit: you say "setValueAtTime()", but the function is called automation()

maybe elaborate a little on this comment, adding that "because the value is set initially by setValue() at the start time, it remains constant for the duration of the interval"

> LayoutTests/webaudio/audioparam-setValueCurveAtTime.html:37
> +// For setValueCurveAtTime, the automation just sets the curve.

how about simplifying the comment like this:

// Sets the curve data for the entire time interval.

> LayoutTests/webaudio/resources/audioparam-testing.js:48
> +// |createConstantBuffer|, but with the parameters to match the other create functions.

I don't understand this function.  It's named createConstantArray(), but it returns a JS object of type AudioBuffer and not Array
These are two very different types of objects
can that be right?

Also, you should note that "endValue" is effectively ignored in this function

> LayoutTests/webaudio/resources/audioparam-testing.js:98
> +    return 1 - Math.pow(1 / 2.718282, 1 / (sampleRate * timeConstant));

Can we use Math.E ?

I know we have a hard-coded constant in the .cpp file, but we should at least see if it's possible here...

> LayoutTests/webaudio/resources/audioparam-testing.js:105
> +function createExponentialApproachArray(startTime, endTime, startValue, endValue, sampleRate, timeConstant)

endValue -> targetValue

"endValue" isn't a good name because it's not the true end value of the array, and may not even be anywhere close to it if the timeConstant is very slow

I know this is trying to be consistent with the other similar functions where it is the true "end" value, but in this case we should be clear that it's just a target

[Raymond Toy]

Comment on attachment 128538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128538&action=review

>> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
>> +                        unsigned curveIndex = static_cast<unsigned>(round(curveVirtualIndex));
> 
> I appreciate using common functions in MathExtras such as round(), but this is inside a tight loop that we want as fast as possible so it's not ideal.  Can we define a tighter inline function for this?

We could do static_cast<unsigned>(0.5 + curveVirtualIndex).  But, as the comments for round() say, this causes loss of precision.  And this will have round-off problems on Windows.  I don't think any tests currently show this effect though.

I know how to do a fast round-to-even (two floating-point adds, two compares).  Don't know how well that will work on windows with the x87 registers.

[Chris Rogers]

(In reply to comment #3)
> (From update of attachment 128538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128538&action=review
> 
> >> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
> >> +                        unsigned curveIndex = static_cast<unsigned>(round(curveVirtualIndex));
> > 
> > I appreciate using common functions in MathExtras such as round(), but this is inside a tight loop that we want as fast as possible so it's not ideal.  Can we define a tighter inline function for this?
> 
> We could do static_cast<unsigned>(0.5 + curveVirtualIndex).  But, as the comments for round() say, this causes loss of precision.  And this will have round-off problems on Windows.  I don't think any tests currently show this effect though.
> 
> I know how to do a fast round-to-even (two floating-point adds, two compares).  Don't know how well that will work on windows with the x87 registers.

I'm not very concerned about small losses of precision here.  Speed is much more important.
So, if at all possible, let's use static_cast<unsigned>(0.5 + curveVirtualIndex)
and include a comment that we avoid round() in the inner-loop and can accept some small loss of precision.

[Raymond Toy]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 128538 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128538&action=review
> > 
> > >> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
> > >> +                        unsigned curveIndex = static_cast<unsigned>(round(curveVirtualIndex));
> > > 
> > > I appreciate using common functions in MathExtras such as round(), but this is inside a tight loop that we want as fast as possible so it's not ideal.  Can we define a tighter inline function for this?
> > 
> > We could do static_cast<unsigned>(0.5 + curveVirtualIndex).  But, as the comments for round() say, this causes loss of precision.  And this will have round-off problems on Windows.  I don't think any tests currently show this effect though.
> > 
> > I know how to do a fast round-to-even (two floating-point adds, two compares).  Don't know how well that will work on windows with the x87 registers.
> 
> I'm not very concerned about small losses of precision here.  Speed is much more important.
> So, if at all possible, let's use static_cast<unsigned>(0.5 + curveVirtualIndex)
> and include a comment that we avoid round() in the inner-loop and can accept some small loss of precision.

Ok, but there may be issues on Windows then.  Curiously, though, this round-off issue was found on linux, not windows, where the setValueCurveAtTime test passes without this rounding fix.  I will need to do a few more tests....

[Raymond Toy]

Comment on attachment 128538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128538&action=review

>>>>> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
>>>>> +                        unsigned curveIndex = static_cast<unsigned>(round(curveVirtualIndex));
>>>> 
>>>> I appreciate using common functions in MathExtras such as round(), but this is inside a tight loop that we want as fast as possible so it's not ideal.  Can we define a tighter inline function for this?
>>> 
>>> We could do static_cast<unsigned>(0.5 + curveVirtualIndex).  But, as the comments for round() say, this causes loss of precision.  And this will have round-off problems on Windows.  I don't think any tests currently show this effect though.
>>> 
>>> I know how to do a fast round-to-even (two floating-point adds, two compares).  Don't know how well that will work on windows with the x87 registers.
>> 
>> I'm not very concerned about small losses of precision here.  Speed is much more important.
>> So, if at all possible, let's use static_cast<unsigned>(0.5 + curveVirtualIndex)
>> and include a comment that we avoid round() in the inner-loop and can accept some small loss of precision.
> 
> Ok, but there may be issues on Windows then.  Curiously, though, this round-off issue was found on linux, not windows, where the setValueCurveAtTime test passes without this rounding fix.  I will need to do a few more tests....

Tested on windows.  No apparent issues.

>> LayoutTests/webaudio/audioparam-setValueAtTime.html:33
>> +// For setValueAtTime(), we don't need to do anything for automation.
> 
> nit: you say "setValueAtTime()", but the function is called automation()
> 
> maybe elaborate a little on this comment, adding that "because the value is set initially by setValue() at the start time, it remains constant for the duration of the interval"

Done.

>> LayoutTests/webaudio/audioparam-setValueCurveAtTime.html:37
>> +// For setValueCurveAtTime, the automation just sets the curve.
> 
> how about simplifying the comment like this:
> 
> // Sets the curve data for the entire time interval.

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:48
>> +// |createConstantBuffer|, but with the parameters to match the other create functions.
> 
> I don't understand this function.  It's named createConstantArray(), but it returns a JS object of type AudioBuffer and not Array
> These are two very different types of objects
> can that be right?
> 
> Also, you should note that "endValue" is effectively ignored in this function

Oops.  createConstantArray used to have the same body as createConstantBuffer, so I decided to reuse createConstantBuffer, but forgot to return the array instead of the buffer.

Comment added for endValue.

>> LayoutTests/webaudio/resources/audioparam-testing.js:98
>> +    return 1 - Math.pow(1 / 2.718282, 1 / (sampleRate * timeConstant));
> 
> Can we use Math.E ?
> 
> I know we have a hard-coded constant in the .cpp file, but we should at least see if it's possible here...

Rewrote to use the (proposed) exp(-1/(sampleRate*timeConstant).

No problems with the tests.

>> LayoutTests/webaudio/resources/audioparam-testing.js:105
>> +function createExponentialApproachArray(startTime, endTime, startValue, endValue, sampleRate, timeConstant)
> 
> endValue -> targetValue
> 
> "endValue" isn't a good name because it's not the true end value of the array, and may not even be anywhere close to it if the timeConstant is very slow
> 
> I know this is trying to be consistent with the other similar functions where it is the true "end" value, but in this case we should be clear that it's just a target

Done.

[Raymond Toy]

Created attachment 128591 [details]
Patch

[Chris Rogers]

> >> LayoutTests/webaudio/resources/audioparam-testing.js:48
> >> +// |createConstantBuffer|, but with the parameters to match the other create functions.
> > 
> > I don't understand this function.  It's named createConstantArray(), but it returns a JS object of type AudioBuffer and not Array
> > These are two very different types of objects
> > can that be right?
> > 
> > Also, you should note that "endValue" is effectively ignored in this function
> 
> Oops.  createConstantArray used to have the same body as createConstantBuffer, so I decided to reuse createConstantBuffer, but forgot to return the array instead of the buffer.

Ok, but then how was the audioparam-setValueAtTime.html layout test actually passing (in your first patch) since you were using that function?

[Raymond Toy]

(In reply to comment #8)
> > >> LayoutTests/webaudio/resources/audioparam-testing.js:48
> > >> +// |createConstantBuffer|, but with the parameters to match the other create functions.
> > > 
> > > I don't understand this function.  It's named createConstantArray(), but it returns a JS object of type AudioBuffer and not Array
> > > These are two very different types of objects
> > > can that be right?
> > > 
> > > Also, you should note that "endValue" is effectively ignored in this function
> > 
> > Oops.  createConstantArray used to have the same body as createConstantBuffer, so I decided to reuse createConstantBuffer, but forgot to return the array instead of the buffer.
> 
> Ok, but then how was the audioparam-setValueAtTime.html layout test actually passing (in your first patch) since you were using that function?

Good question!  It passes because the expected values are undefined, which javascript converts to NaN, so the computed error in comparePartialSignals is also NaN, which is never greater than maxError, so the tests pass.

I've added a check in comparePartialSignals to catch NaN's in either the rendered data or the reference data.  This also caught an error in setTargetValueAtTime test.

Updated patch coming soon.

[Raymond Toy]

Created attachment 128752 [details]
Patch

[Chris Rogers]

Thanks for reminding me.  Have you verified that each of the tests will fail if you introduce a small bug in the code at appropriate points?

[Raymond Toy]

(In reply to comment #11)
> Thanks for reminding me.  Have you verified that each of the tests will fail if you introduce a small bug in the code at appropriate points?

I did before, but let me check again.

[Raymond Toy]

(In reply to comment #12)
> (In reply to comment #11)
> > Thanks for reminding me.  Have you verified that each of the tests will fail if you introduce a small bug in the code at appropriate points?
> 
> I did before, but let me check again.

If small errors are made to the time or to the discrete time constant, then the tests fail.  We already know that canary builds fail on the exponential ramp (bad starting value) and the setValueCurveAtTime (round off error in computing the time values), and these tests fail on a canary build as expected.

[Chris Rogers]

Comment on attachment 128752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128752&action=review

> Source/WebCore/ChangeLog:6
> +        Add tests the AudioParam automation functions.

nit: Add tests "to" the AudioParam...

Actually, this additional comment is probably not even necessary given the title (line 3)

> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
> +                        // Ideally use round() from MathExtras, but we're in a tight loop here and

"Ideally use" -> "Ideally we'd"

> LayoutTests/webaudio/audioparam-linearRampToValueAtTime.html:39
> +    // above.

seems unnecessary to wrap the line for a single word.

> LayoutTests/webaudio/audioparam-setTargetValueAtTime.html:56
> +                            discontinuityThreshold,

I see that discontinuityThreshold is defined in audioparam-testing.js but is not initialized to any value?

Could you please look into why this test was passing/failing as expected even though this seems to be uninitialized (at least in my naive reading of the code).

> LayoutTests/webaudio/audioparam-setValueCurveAtTime.html:29
> +var sineAmplitude = 100;

This is a pretty enormous amplitude +40dB gain to scale the signal by.  Can't we choose a value more in-tune with real-world usage?

> LayoutTests/webaudio/audioparam-setValueCurveAtTime.html:40
> +    // For a value curve, we ignore the |value| and |endTime|.

Does it make sense to compute the "duration" (3rd param) as (endTime - startTime) instead of using the global "timeInterval"?

> LayoutTests/webaudio/resources/audioparam-testing.js:4
> +// interval.

nit: unneeded line wrapping for a single word.

maybe could be simplified to make this easier to read: "starting and ending" -> "starting/ending"

> LayoutTests/webaudio/resources/audioparam-testing.js:29
> +function renderLength(tests)

tests -> numberOfTests

> LayoutTests/webaudio/resources/audioparam-testing.js:69
> +    var buffer = new Array(length);

why is this called "buffer" when it is not type AudioBuffer, but is Array?

buffer -> array : here and in rest of function

Also, is there any reason we're not using Float32Array here?

> LayoutTests/webaudio/resources/audioparam-testing.js:113
> +    var buffer = new Array(length);

buffer -> array : here and in rest of function

Array -> Float32Array?

> LayoutTests/webaudio/resources/audioparam-testing.js:176
> +            maxError = 1e300;

1e300 seems a little dramatic and hard to read - can we use a simpler large value?

> LayoutTests/webaudio/resources/audioparam-testing.js:178
> +            testFailed("Invalid number for rendered data at " + maxErrorIndex);

Instead of "Invalid" can we say NaN or Inf --- Invalid is a bit vague

> LayoutTests/webaudio/resources/audioparam-testing.js:184
> +            testFailed("Invalid number for reference data at " + maxErrorIndex);

ditto

> LayoutTests/webaudio/resources/audioparam-testing.js:216
> +    if (breaks.length >= numberOfTests) {

please add simple comment explaining why we subtract one

> LayoutTests/webaudio/resources/audioparam-testing.js:239
> +        if (breaks.length + 1 == numberOfTests) {

It's hard to follow the voodoo with adding 1 and subtracing 1 in various parts of this function --- can this be made more clear for those poor souls trying to understand the test?

[Raymond Toy]

Comment on attachment 128752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128752&action=review

>> LayoutTests/webaudio/audioparam-setTargetValueAtTime.html:56
>> +                            discontinuityThreshold,
> 
> I see that discontinuityThreshold is defined in audioparam-testing.js but is not initialized to any value?
> 
> Could you please look into why this test was passing/failing as expected even though this seems to be uninitialized (at least in my naive reading of the code).

discontinuityThreshold and timeConstant are optional parameters.  For this test, we don't need the threshold (the default that is computed in createAudioGraphAndTest works), but we need the time constant.

The other tests don't need a time constant and set the threshold as needed.

I will rework this.

[Raymond Toy]

Created attachment 129532 [details]
Patch

[Raymond Toy]

Comment on attachment 128752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128752&action=review

>> Source/WebCore/ChangeLog:6
>> +        Add tests the AudioParam automation functions.
> 
> nit: Add tests "to" the AudioParam...
> 
> Actually, this additional comment is probably not even necessary given the title (line 3)

Removed.

>> Source/WebCore/webaudio/AudioParamTimeline.cpp:332
>> +                        // Ideally use round() from MathExtras, but we're in a tight loop here and
> 
> "Ideally use" -> "Ideally we'd"

Done.

>> LayoutTests/webaudio/audioparam-linearRampToValueAtTime.html:39
>> +    // above.
> 
> seems unnecessary to wrap the line for a single word.

Done.

>>> LayoutTests/webaudio/audioparam-setTargetValueAtTime.html:56
>>> +                            discontinuityThreshold,
>> 
>> I see that discontinuityThreshold is defined in audioparam-testing.js but is not initialized to any value?
>> 
>> Could you please look into why this test was passing/failing as expected even though this seems to be uninitialized (at least in my naive reading of the code).
> 
> discontinuityThreshold and timeConstant are optional parameters.  For this test, we don't need the threshold (the default that is computed in createAudioGraphAndTest works), but we need the time constant.
> 
> The other tests don't need a time constant and set the threshold as needed.
> 
> I will rework this.

Reworked.  I hope it's clearer now.

>> LayoutTests/webaudio/audioparam-setValueCurveAtTime.html:29
>> +var sineAmplitude = 100;
> 
> This is a pretty enormous amplitude +40dB gain to scale the signal by.  Can't we choose a value more in-tune with real-world usage?

Changed to 1.

>> LayoutTests/webaudio/audioparam-setValueCurveAtTime.html:40
>> +    // For a value curve, we ignore the |value| and |endTime|.
> 
> Does it make sense to compute the "duration" (3rd param) as (endTime - startTime) instead of using the global "timeInterval"?

Done.  Should be ok.  Any roundoff in the computation should not matter since it should be way below the sample frame interval.

>> LayoutTests/webaudio/resources/audioparam-testing.js:4
>> +// interval.
> 
> nit: unneeded line wrapping for a single word.
> 
> maybe could be simplified to make this easier to read: "starting and ending" -> "starting/ending"

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:29
>> +function renderLength(tests)
> 
> tests -> numberOfTests

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:69
>> +    var buffer = new Array(length);
> 
> why is this called "buffer" when it is not type AudioBuffer, but is Array?
> 
> buffer -> array : here and in rest of function
> 
> Also, is there any reason we're not using Float32Array here?

Changed to array everywhere.

I think it's nice to have the reference have as much accuracy as possible, even if the rendered data only has float precision.

>> LayoutTests/webaudio/resources/audioparam-testing.js:113
>> +    var buffer = new Array(length);
> 
> buffer -> array : here and in rest of function
> 
> Array -> Float32Array?

Done.  See above comment for line 69.

>> LayoutTests/webaudio/resources/audioparam-testing.js:176
>> +            maxError = 1e300;
> 
> 1e300 seems a little dramatic and hard to read - can we use a simpler large value?

Changed to use javascript Infinity.  That should compare as we expect it to.

>> LayoutTests/webaudio/resources/audioparam-testing.js:178
>> +            testFailed("Invalid number for rendered data at " + maxErrorIndex);
> 
> Instead of "Invalid" can we say NaN or Inf --- Invalid is a bit vague

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:184
>> +            testFailed("Invalid number for reference data at " + maxErrorIndex);
> 
> ditto

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:216
>> +    if (breaks.length >= numberOfTests) {
> 
> please add simple comment explaining why we subtract one

Comment added.  I hope it makes it clearer why we do this here and in the following code.

>> LayoutTests/webaudio/resources/audioparam-testing.js:239
>> +        if (breaks.length + 1 == numberOfTests) {
> 
> It's hard to follow the voodoo with adding 1 and subtracing 1 in various parts of this function --- can this be made more clear for those poor souls trying to understand the test?

Code cleaned up a little and I hope the comment added near line 216 makes it clearer.

[Chris Rogers]

Comment on attachment 129532 [details]
Patch

Looks good - thanks for the tests!

[WebKit Review Bot]

Comment on attachment 129532 [details]
Patch

Rejecting attachment 129532 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Chris Rogers]

Comment on attachment 129532 [details]
Patch

Re-trying commit queue - committers.py seems fine...

[WebKit Review Bot]

Comment on attachment 129532 [details]
Patch

Rejecting attachment 129532 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e conflict in Source/WebKit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [DRT] Remove all PlainTextController usages in existing tests by adding internal API

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/11757592

[Raymond Toy]

Created attachment 129742 [details]
Patch

[Raymond Toy]

(In reply to comment #22)
> Created an attachment (id=129742) [details]
> Patch

Updated patch against current sources.

[Chris Rogers]

Comment on attachment 129742 [details]
Patch

trying commit queue again

[WebKit Review Bot]

Comment on attachment 129742 [details]
Patch

Clearing flags on attachment: 129742

Committed r109518: <http://trac.webkit.org/changeset/109518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.