Bug 78035 – exponentialRampToValue doesn't use starting value

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

exponentialRampToValue currently does:


   float numSampleFrames = deltaTime * sampleRate;
   float multiplier = powf(value2 / value1, 1 / numSampleFrames);
   for (; writeIndex < fillToFrame; ++writeIndex) {
       values[writeIndex] = value;
       value *= multiplier;
       currentTime += sampleFrameTimeIncr;
   }

Note that value has no relationship to value1 or value2, so the ramp never actually starts at the correct value and if it reaches value2, it is coincidental.

I think that before the loop we need to do something like

value = value1 * pow(multiplier, AudioUtilities::timeToSampleFrame(currentTime - time1, sampleRate)

to set the correct starting value.

[Chris Rogers]

(In reply to comment #0)
> exponentialRampToValue currently does:
> 
> 
>    float numSampleFrames = deltaTime * sampleRate;
>    float multiplier = powf(value2 / value1, 1 / numSampleFrames);
>    for (; writeIndex < fillToFrame; ++writeIndex) {
>        values[writeIndex] = value;
>        value *= multiplier;
>        currentTime += sampleFrameTimeIncr;
>    }
> 
> Note that value has no relationship to value1 or value2, so the ramp never actually starts at the correct value and if it reaches value2, it is coincidental.
> 
> I think that before the loop we need to do something like
> 
> value = value1 * pow(multiplier, AudioUtilities::timeToSampleFrame(currentTime - time1, sampleRate)
> 
> to set the correct starting value.

Are you also finding that your layout test is failing because of this, even if you call setValueAtTime() first before exponentialRampToValue()?

[Raymond Toy]

(In reply to comment #1)
> (In reply to comment #0)
> > exponentialRampToValue currently does:
> > 
> > 
> >    float numSampleFrames = deltaTime * sampleRate;
> >    float multiplier = powf(value2 / value1, 1 / numSampleFrames);
> >    for (; writeIndex < fillToFrame; ++writeIndex) {
> >        values[writeIndex] = value;
> >        value *= multiplier;
> >        currentTime += sampleFrameTimeIncr;
> >    }
> > 
> > Note that value has no relationship to value1 or value2, so the ramp never actually starts at the correct value and if it reaches value2, it is coincidental.
> > 
> > I think that before the loop we need to do something like
> > 
> > value = value1 * pow(multiplier, AudioUtilities::timeToSampleFrame(currentTime - time1, sampleRate)
> > 
> > to set the correct starting value.
> 
> Are you also finding that your layout test is failing because of this, even if you call setValueAtTime() first before exponentialRampToValue()?

Yes, I always call setValueAtTime for time n before calling exponentialRampToValue at time n + 1.

[Chris Rogers]

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > exponentialRampToValue currently does:
> > > 
> > > 
> > >    float numSampleFrames = deltaTime * sampleRate;
> > >    float multiplier = powf(value2 / value1, 1 / numSampleFrames);
> > >    for (; writeIndex < fillToFrame; ++writeIndex) {
> > >        values[writeIndex] = value;
> > >        value *= multiplier;
> > >        currentTime += sampleFrameTimeIncr;
> > >    }
> > > 
> > > Note that value has no relationship to value1 or value2, so the ramp never actually starts at the correct value and if it reaches value2, it is coincidental.
> > > 
> > > I think that before the loop we need to do something like
> > > 
> > > value = value1 * pow(multiplier, AudioUtilities::timeToSampleFrame(currentTime - time1, sampleRate)
> > > 
> > > to set the correct starting value.
> > 
> > Are you also finding that your layout test is failing because of this, even if you call setValueAtTime() first before exponentialRampToValue()?
> 
> Yes, I always call setValueAtTime for time n before calling exponentialRampToValue at time n + 1.

Ok, that makes sense.  Something like your proposed fix seems right.  I would suggest making the fix in this bug and adding the layout test here too.

[Raymond Toy]

Created attachment 126158 [details]
Patch

[Chris Rogers]

Comment on attachment 126158 [details]
Patch

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

> Source/WebCore/webaudio/AudioParamTimeline.cpp:247
> +                // the same as multiplier^(currentTime-time1), but

nit: spacing for math operators

actually looking at this equation more closely -- what you mean to say (I think) is that it's the same as multiplier ^ AudioUtilities::timeToSampleFrame(currentTime - time1)
WebKit style: can we get line wrapping on these comments to be more consistent  with the rest of this file (eg. line 242) and with WebKit style in general which doesn't favor such short line wrapping.

> LayoutTests/webaudio/audioparam-exponential.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">

audioparam-exponential seems a little terse.  How about "audioparam-exponentialRampToValueAtTime"

I've noticed some of the canvas2D layout tests use this style of naming (camel-casing) and seems good here too:
(for example:  "linearGradient-infinite-values.html")

> LayoutTests/webaudio/audioparam-exponential.html:14
> +      description("Test AudioParam exponentialRampToValueAtTime functionality.");

how about:

"Test AudioParam exponentialRampToValueAtTime()"

> LayoutTests/webaudio/audioparam-exponential.html:16
> +      // Play a long square wave out through a gain block, and call

let's be specific: "gain block" -> "AudioGainNode"

nit: it's not really a square wave (which is a periodic signal with duty cycle with +1 portion and symmetric -1 portion)
Let's just call it a DC-offset of value 1 (or some more concise version of that)

> LayoutTests/webaudio/audioparam-exponential.html:17
> +      // setValueAtTime at regular intervals to set the starting gain.

setValueAtTime -> setValueAtTime()

How about re-wording this line:
// setValueAtTime() and exponentialRampToValueAtTime() to establish the start and end points for an exponential ramp.

> LayoutTests/webaudio/audioparam-exponential.html:19
> +      // the rendered signal to see if the value of the square wave

Let's be *very* specific and use precise (but as concise as possible) language about what this test does.

"We generate an exponential ramp"  <---- a "different" exponential ramp each time??  are they different starting and ending values?
And if so, how.

> LayoutTests/webaudio/audioparam-exponential.html:31
> +      // gain.

style nit: strange to wrap a line just for one word

Try to keep comments a bit longer than you are -- it's WebKit style and also is a matter of consistency with the rest of this file (lines 45, etc.)

> LayoutTests/webaudio/audioparam-exponential.html:36
> +      var gainStep = startingGain / (2 * numberOfTests);

can we choose a more descriptive name for this variable such as "startEndGainChange"

> LayoutTests/webaudio/audioparam-exponential.html:38
> +      var squareBuffer;

constantBuffer - or dcBuffer

> LayoutTests/webaudio/audioparam-exponential.html:70
> +          // 2*gainStep.  However, the exponential ramp target gain is

nit: 2 * gainStep

for naming symmetry can we avoid the use of the word "target" and instead use "end" (as compared with "start")
Here and everywhere in the patch

> LayoutTests/webaudio/audioparam-exponential.html:92
> +                                      function(g, startTime, endTime) {

Not sure I understand why we need "startTime" if we're not using it here --- is it an abstraction useful for testing other curves other than the exponential one?

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

Can we use the "create" naming convention that we use in "createSquareBuffer" above?
For example, call the function "createExponentialRampArray()"

> LayoutTests/webaudio/resources/audioparam-testing.js:34
> +{

Let's be consistent with use of braces --- I see you have the brace at the end-of-line on 17, 50, etc.

Actually, WebKit style is to have braces on the following line (for functions) but let's be consistent everywhere

> LayoutTests/webaudio/resources/audioparam-testing.js:52
> +    var expected = expectedFunction(startTime, endTime, gainInfo.gainStart, gainInfo.gainTarget, sampleRate);

See comments below about doAutomation() and compareSignals().

"gainInfo.gainStart, gainInfo.gainTarget" seems like it would be a great deal clearer as "valueInfo.startValue, valueInfo.endValue"

> LayoutTests/webaudio/resources/audioparam-testing.js:107
> +function compareSignals(testName, maxError, renderedData, expectedFunction, timeGainInfo, breakThreshold) {

See my comment about doAutomation() below.  Please lets remove the term "gain" and use "value" instead.  How about "timeValueInfo" since all the AudioParam methods deal in the concepts of time and value.

> LayoutTests/webaudio/resources/audioparam-testing.js:139
> +// Run all the automation tests.

In the doAutomation() function I think it would be a lot more clear if we avoid talking about "gain" and instead use the term "value"
This function doesn't need to know anything at all about gain (it's only the caller of this function that knows that) and furthermore
all the AudioParam methods that we're testing talk about things in terms of "value"
Can you have a run through the comments and variable names for this function.

> LayoutTests/webaudio/resources/audioparam-testing.js:141
> +// gainUpdate - How much to change the gain at beginning of each time

This is pretty confusing -- can we call it gainUpdateFunction since it's a function -- not clear from this comment

> LayoutTests/webaudio/resources/audioparam-testing.js:172
> +        // target gain.

nit: unnecessary line wrapping

[Raymond Toy]

Created attachment 126536 [details]
Patch

[Raymond Toy]

Comment on attachment 126158 [details]
Patch

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

>> Source/WebCore/webaudio/AudioParamTimeline.cpp:247
>> +                // the same as multiplier^(currentTime-time1), but
> 
> nit: spacing for math operators
> 
> actually looking at this equation more closely -- what you mean to say (I think) is that it's the same as multiplier ^ AudioUtilities::timeToSampleFrame(currentTime - time1)
> WebKit style: can we get line wrapping on these comments to be more consistent  with the rest of this file (eg. line 242) and with WebKit style in general which doesn't favor such short line wrapping.

Fixed.  Longer line wrapping added.

>> LayoutTests/webaudio/audioparam-exponential.html:1
>> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
> 
> audioparam-exponential seems a little terse.  How about "audioparam-exponentialRampToValueAtTime"
> 
> I've noticed some of the canvas2D layout tests use this style of naming (camel-casing) and seems good here too:
> (for example:  "linearGradient-infinite-values.html")

Renamed as suggested.

>> LayoutTests/webaudio/audioparam-exponential.html:14
>> +      description("Test AudioParam exponentialRampToValueAtTime functionality.");
> 
> how about:
> 
> "Test AudioParam exponentialRampToValueAtTime()"

Done.

>> LayoutTests/webaudio/audioparam-exponential.html:16
>> +      // Play a long square wave out through a gain block, and call
> 
> let's be specific: "gain block" -> "AudioGainNode"
> 
> nit: it's not really a square wave (which is a periodic signal with duty cycle with +1 portion and symmetric -1 portion)
> Let's just call it a DC-offset of value 1 (or some more concise version of that)

Done.

>> LayoutTests/webaudio/audioparam-exponential.html:17
>> +      // setValueAtTime at regular intervals to set the starting gain.
> 
> setValueAtTime -> setValueAtTime()
> 
> How about re-wording this line:
> // setValueAtTime() and exponentialRampToValueAtTime() to establish the start and end points for an exponential ramp.

Rephrased.  I hope this is better.

>> LayoutTests/webaudio/audioparam-exponential.html:19
>> +      // the rendered signal to see if the value of the square wave
> 
> Let's be *very* specific and use precise (but as concise as possible) language about what this test does.
> 
> "We generate an exponential ramp"  <---- a "different" exponential ramp each time??  are they different starting and ending values?
> And if so, how.

Let me know if the new test is specific, precise, and concise.

>> LayoutTests/webaudio/audioparam-exponential.html:31
>> +      // gain.
> 
> style nit: strange to wrap a line just for one word
> 
> Try to keep comments a bit longer than you are -- it's WebKit style and also is a matter of consistency with the rest of this file (lines 45, etc.)

Done.

>> LayoutTests/webaudio/audioparam-exponential.html:36
>> +      var gainStep = startingGain / (2 * numberOfTests);
> 
> can we choose a more descriptive name for this variable such as "startEndGainChange"

Done.  Renamed everything containing "gain" to "value" as appropriate.

>> LayoutTests/webaudio/audioparam-exponential.html:38
>> +      var squareBuffer;
> 
> constantBuffer - or dcBuffer

Renamed to constantBuffer.

>> LayoutTests/webaudio/audioparam-exponential.html:70
>> +          // 2*gainStep.  However, the exponential ramp target gain is
> 
> nit: 2 * gainStep
> 
> for naming symmetry can we avoid the use of the word "target" and instead use "end" (as compared with "start")
> Here and everywhere in the patch

Done.

>> LayoutTests/webaudio/audioparam-exponential.html:92
>> +                                      function(g, startTime, endTime) {
> 
> Not sure I understand why we need "startTime" if we're not using it here --- is it an abstraction useful for testing other curves other than the exponential one?

Yes, this is an abstraction.  Originally the start time was not included, but for setTargetValueAtTime, the start time is needed.

>> LayoutTests/webaudio/resources/audioparam-testing.js:33
>> +function exponentialRampSignal(startTime, endTime, startValue, endValue, sampleRate)
> 
> Can we use the "create" naming convention that we use in "createSquareBuffer" above?
> For example, call the function "createExponentialRampArray()"

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:34
>> +{
> 
> Let's be consistent with use of braces --- I see you have the brace at the end-of-line on 17, 50, etc.
> 
> Actually, WebKit style is to have braces on the following line (for functions) but let's be consistent everywhere

Fixed.

>> LayoutTests/webaudio/resources/audioparam-testing.js:52
>> +    var expected = expectedFunction(startTime, endTime, gainInfo.gainStart, gainInfo.gainTarget, sampleRate);
> 
> See comments below about doAutomation() and compareSignals().
> 
> "gainInfo.gainStart, gainInfo.gainTarget" seems like it would be a great deal clearer as "valueInfo.startValue, valueInfo.endValue"

Done.  "gain" renamed to "value" everywhere appropriate.

>> LayoutTests/webaudio/resources/audioparam-testing.js:107
>> +function compareSignals(testName, maxError, renderedData, expectedFunction, timeGainInfo, breakThreshold) {
> 
> See my comment about doAutomation() below.  Please lets remove the term "gain" and use "value" instead.  How about "timeValueInfo" since all the AudioParam methods deal in the concepts of time and value.

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:139
>> +// Run all the automation tests.
> 
> In the doAutomation() function I think it would be a lot more clear if we avoid talking about "gain" and instead use the term "value"
> This function doesn't need to know anything at all about gain (it's only the caller of this function that knows that) and furthermore
> all the AudioParam methods that we're testing talk about things in terms of "value"
> Can you have a run through the comments and variable names for this function.

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:141
>> +// gainUpdate - How much to change the gain at beginning of each time
> 
> This is pretty confusing -- can we call it gainUpdateFunction since it's a function -- not clear from this comment

Changed to valueUpdateFunction.

>> LayoutTests/webaudio/resources/audioparam-testing.js:172
>> +        // target gain.
> 
> nit: unnecessary line wrapping

Fixed.

[Chris Rogers]

Comment on attachment 126536 [details]
Patch

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

> Source/WebCore/webaudio/AudioParamTimeline.cpp:246
> +                // Set the starting value of the exponential. This is the same as multiplier ^

"exponential" -> "exponential ramp"

> Source/WebCore/webaudio/AudioParamTimeline.cpp:247
> +                // AudioUtilities::timeToSampleFrame(currentTime - time1, sampleRate), but appears

"but appears to be" -> "but is"

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime-expected.txt:6
> +PASS All 100 tests passed with absolute error below 0.000675

Probably best not to explicitly say "below 0.000675", but instead say "within an acceptable tolerance."

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:24
> +      var maxAllowedError = 6.75e-4;

Can't we just push this threshold into the .js file?

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:31
> +      var startingValue = 100;

See comment below about using "initialValue" instead of "startingValue"

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:35
> +      var startEndValueChange = startingValue / (2 * numberOfTests);

"Each exponential ramp" -> "The value of each exponential ramp"
"increase or decrease" -> "change"

There are several places where we're dividing or multiplying by 2.  Here, line 44 and line  68, 77
This is pretty confusing to me just trying to follow the code from start to finish.

Is there any way we can avoid this and make it simpler?

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:89
> +                                      function(value, startTime, endTime) {

Please add short comment describing that we ignore the "startTime" because exponentialRampToValueAtTime() uses the start time from the setValueAtTime() call above.

> LayoutTests/webaudio/resources/audioparam-testing.js:29
> +// Create a exponential ramp starting at |startValue| and ending at |endValue|.  The ramp starts at

nit: "a exponential" -> "an exponential"

> LayoutTests/webaudio/resources/audioparam-testing.js:70
> +// expected start of the time interval. There is a discontinuity if the difference between

location -> locations
discontinuity -> discontinuities
"with our expected start of the time interval" -> "with the times which define the time intervals"

> LayoutTests/webaudio/resources/audioparam-testing.js:72
> +function verifyDiscontinuities(data, startTime, threshold)

How about using "values" instead of "data" -- seems a little more friendly

startTime -> times

"start" isn't really helpful here because the time value represents the end of one interval and the start of the next, thus "start" has no special meaning or preference as compared with "end".

> LayoutTests/webaudio/resources/audioparam-testing.js:89
> +        var expected = timeToSampleFrame(startTime[k + 1], sampleRate);

expected -> expectedSampleFrame

> LayoutTests/webaudio/resources/audioparam-testing.js:110
> +// maxError - maximum allowed difference between the rendered data and the expected data

Can't we just have this be an internal threshold shared among all the different tests?

> LayoutTests/webaudio/resources/audioparam-testing.js:143
> +        testPassed("All " + n + " tests passed with absolute error below " + maxError);

See comment above (in the expected.txt file)

> LayoutTests/webaudio/resources/audioparam-testing.js:155
> +// startingValue - The starting value.

This naming is a bit confusing, since each time interval has the notion of "start" and "end" times and values.
So, I would suggest calling it "initialValue" instead, since it sets the very initial value for all of the intervals...

> LayoutTests/webaudio/resources/audioparam-testing.js:157
> +// valueUpdateFunction - How much to change the value at beginning of each time interval.

It appears from the code that this is the amount to change at the *end* of each time interval and not the *beginning*.  Please fix here and anywhere else (if any)

Also, since it seems to happen at the end of the loop, it would probably make sense to change the order of the parameters and make this the very last parameter.

> LayoutTests/webaudio/resources/audioparam-testing.js:159
> +// endValueStepFunction - How much the target value differs from the value at the start of the

I'm not very fond of the name "step function", since this has connotations in mathematics (think "unit step function").
So, how about calling it "endValueDeltaFunction"

> LayoutTests/webaudio/resources/audioparam-testing.js:165
> +// the value approaches the target.

I'd rather not use the word "target" since it has very specific meaning in relation to setTargetValueAtTime().
Because of this and also because it's more consistent, let's use "end" instead, here and everywhere else

> LayoutTests/webaudio/resources/audioparam-testing.js:169
> +function doAutomation(startingValue, valueUpdateFunction, endValueStepFunction, setValueFunction, automationFunction)

startingValue -> initialValue

> LayoutTests/webaudio/resources/audioparam-testing.js:171
> +    var startTimeInfo = [0];

startTimeInfo -> timeInfo

> LayoutTests/webaudio/resources/audioparam-testing.js:173
> +    var value = startingValue;

startingValue -> initialValue

> LayoutTests/webaudio/resources/audioparam-testing.js:177
> +        var timeEnd = (k + 1) * timeInterval;

timeStart -> startTime
timeEnd -> endTime

> LayoutTests/webaudio/resources/audioparam-testing.js:188
> +        valueInfo.push({ startValue: value, endValue : endValue});

nit: extra space

> LayoutTests/webaudio/resources/audioparam-testing.js:193
> +    return { startTime : startTimeInfo, value : valueInfo };

We're dealing with arrays of times and values (not a single value)
startTime -> times
value -> values

here and everywhere it's used

[Raymond Toy]

Created attachment 128225 [details]
Patch

[Raymond Toy]

Comment on attachment 126536 [details]
Patch

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

>> Source/WebCore/webaudio/AudioParamTimeline.cpp:246
>> +                // Set the starting value of the exponential. This is the same as multiplier ^
> 
> "exponential" -> "exponential ramp"

Done.

>> Source/WebCore/webaudio/AudioParamTimeline.cpp:247
>> +                // AudioUtilities::timeToSampleFrame(currentTime - time1, sampleRate), but appears
> 
> "but appears to be" -> "but is"

Done.

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime-expected.txt:6
>> +PASS All 100 tests passed with absolute error below 0.000675
> 
> Probably best not to explicitly say "below 0.000675", but instead say "within an acceptable tolerance."

Done.  But "acceptable tolerance" is so vague.

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:24
>> +      var maxAllowedError = 6.75e-4;
> 
> Can't we just push this threshold into the .js file?

It's not clear that we can use exactly the same value for all tests.  In addition, the correct threshold for the setValueCurve test is 0 because the reference is stored in a Float32Array, so the rendered output should be exactly equal to the reference.

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:35
>> +      var startEndValueChange = startingValue / (2 * numberOfTests);
> 
> "Each exponential ramp" -> "The value of each exponential ramp"
> "increase or decrease" -> "change"
> 
> There are several places where we're dividing or multiplying by 2.  Here, line 44 and line  68, 77
> This is pretty confusing to me just trying to follow the code from start to finish.
> 
> Is there any way we can avoid this and make it simpler?

Comments changed.

This has been rewritten a bit, adding a few functions with comments.  I hope this is clearer.

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:89
>> +                                      function(value, startTime, endTime) {
> 
> Please add short comment describing that we ignore the "startTime" because exponentialRampToValueAtTime() uses the start time from the setValueAtTime() call above.

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:29
>> +// Create a exponential ramp starting at |startValue| and ending at |endValue|.  The ramp starts at
> 
> nit: "a exponential" -> "an exponential"

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:70
>> +// expected start of the time interval. There is a discontinuity if the difference between
> 
> location -> locations
> discontinuity -> discontinuities
> "with our expected start of the time interval" -> "with the times which define the time intervals"

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:72
>> +function verifyDiscontinuities(data, startTime, threshold)
> 
> How about using "values" instead of "data" -- seems a little more friendly
> 
> startTime -> times
> 
> "start" isn't really helpful here because the time value represents the end of one interval and the start of the next, thus "start" has no special meaning or preference as compared with "end".

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:89
>> +        var expected = timeToSampleFrame(startTime[k + 1], sampleRate);
> 
> expected -> expectedSampleFrame

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:110
>> +// maxError - maximum allowed difference between the rendered data and the expected data
> 
> Can't we just have this be an internal threshold shared among all the different tests?

It's not clear that all the (upcoming) tests can use the same threshold.  Or we end up with a worst case threshold that is much larger than needed.  In particular, the setValueCurve threshold should actually be 0 because the curve should be exactly what we set it to.

>> LayoutTests/webaudio/resources/audioparam-testing.js:143
>> +        testPassed("All " + n + " tests passed with absolute error below " + maxError);
> 
> See comment above (in the expected.txt file)

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:155
>> +// startingValue - The starting value.
> 
> This naming is a bit confusing, since each time interval has the notion of "start" and "end" times and values.
> So, I would suggest calling it "initialValue" instead, since it sets the very initial value for all of the intervals...

Renamed.

>> LayoutTests/webaudio/resources/audioparam-testing.js:157
>> +// valueUpdateFunction - How much to change the value at beginning of each time interval.
> 
> It appears from the code that this is the amount to change at the *end* of each time interval and not the *beginning*.  Please fix here and anywhere else (if any)
> 
> Also, since it seems to happen at the end of the loop, it would probably make sense to change the order of the parameters and make this the very last parameter.

Clarified comment, and reordered parameters.

>> LayoutTests/webaudio/resources/audioparam-testing.js:159
>> +// endValueStepFunction - How much the target value differs from the value at the start of the
> 
> I'm not very fond of the name "step function", since this has connotations in mathematics (think "unit step function").
> So, how about calling it "endValueDeltaFunction"

Renamed.

>> LayoutTests/webaudio/resources/audioparam-testing.js:165
>> +// the value approaches the target.
> 
> I'd rather not use the word "target" since it has very specific meaning in relation to setTargetValueAtTime().
> Because of this and also because it's more consistent, let's use "end" instead, here and everywhere else

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:169
>> +function doAutomation(startingValue, valueUpdateFunction, endValueStepFunction, setValueFunction, automationFunction)
> 
> startingValue -> initialValue

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:177
>> +        var timeEnd = (k + 1) * timeInterval;
> 
> timeStart -> startTime
> timeEnd -> endTime

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:188
>> +        valueInfo.push({ startValue: value, endValue : endValue});
> 
> nit: extra space

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:193
>> +    return { startTime : startTimeInfo, value : valueInfo };
> 
> We're dealing with arrays of times and values (not a single value)
> startTime -> times
> value -> values
> 
> here and everywhere it's used

Done.

[Chris Rogers]

Comment on attachment 128225 [details]
Patch

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

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:4
> +    <script src="resources/audio-testing.js"></script>

I just noticed that indentation is 2 spaces here and other places in this file.  I see that other layout tests in the webaudio directory also have this issue,
so it's probably more efficient to fix all of these at once in another patch...

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:28
> +      var timeGainInfo;

timeGainInfo -> timeValueInfo

> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:107
> +          gainNode = context.createGainNode();

It would be great to have a short comment here describing that:

// We use an AudioGainNode here simply as a convenient way to test the AudioParam automation, since it's easy to pass a constant value through the node,
// automate the .gain attribute and observe the resulting values.

I know we have a comment on line 16, but I think this could be a point of confusion and is worth making clear

> LayoutTests/webaudio/resources/audioparam-testing.js:66
> +    return { maxError : maxError, index : maxErrorIndex };

nit: spacing

> LayoutTests/webaudio/resources/audioparam-testing.js:124
> +    var startTime = timeValueInfo.times;

startTime -> times

> LayoutTests/webaudio/resources/audioparam-testing.js:125
> +    var gains = timeValueInfo.values;

gains -> values

> LayoutTests/webaudio/resources/audioparam-testing.js:134
> +            testFailed("Incorrect gain for test " + k + ". Max error = " + result.maxError + " at offset " + (result.index + timeToSampleFrame(startTime[k], sampleRate)));

gain -> value

> LayoutTests/webaudio/resources/audioparam-testing.js:157
> +// valueUpdateFunction - The starting value at time interval k is adjusted by valueUpdateFunction(k) to give the new starting value at time interval k + 1.

The comment for valueUpdateFunction is out-of-order compared with the param order in the function itself

> LayoutTests/webaudio/resources/audioparam-testing.js:183
> +        // Specify the target value, and how we should approach the target value.

target -> end

> LayoutTests/webaudio/resources/audioparam-testing.js:186
> +        // Keep track of the start times, and the start and target values for each time interval.

target -> end

> LayoutTests/webaudio/resources/audioparam-testing.js:193
> +    return { times : timeInfo, values : valueInfo };

nit: extra spaces

[Raymond Toy]

Created attachment 128309 [details]
Patch

[Raymond Toy]

Created attachment 128312 [details]
Patch

[Raymond Toy]

Created attachment 128314 [details]
Patch

[Raymond Toy]

Comment on attachment 128225 [details]
Patch

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

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:4
>> +    <script src="resources/audio-testing.js"></script>
> 
> I just noticed that indentation is 2 spaces here and other places in this file.  I see that other layout tests in the webaudio directory also have this issue,
> so it's probably more efficient to fix all of these at once in another patch...

You mean the indentation of the html tags?  Ok. I'll fix these in another patch (by removing the html tag indentation as done in other files.)

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:28
>> +      var timeGainInfo;
> 
> timeGainInfo -> timeValueInfo

Done.

>> LayoutTests/webaudio/audioparam-exponentialRampToValueAtTime.html:107
>> +          gainNode = context.createGainNode();
> 
> It would be great to have a short comment here describing that:
> 
> // We use an AudioGainNode here simply as a convenient way to test the AudioParam automation, since it's easy to pass a constant value through the node,
> // automate the .gain attribute and observe the resulting values.
> 
> I know we have a comment on line 16, but I think this could be a point of confusion and is worth making clear

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:66
>> +    return { maxError : maxError, index : maxErrorIndex };
> 
> nit: spacing

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:124
>> +    var startTime = timeValueInfo.times;
> 
> startTime -> times

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:125
>> +    var gains = timeValueInfo.values;
> 
> gains -> values

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:134
>> +            testFailed("Incorrect gain for test " + k + ". Max error = " + result.maxError + " at offset " + (result.index + timeToSampleFrame(startTime[k], sampleRate)));
> 
> gain -> value

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:157
>> +// valueUpdateFunction - The starting value at time interval k is adjusted by valueUpdateFunction(k) to give the new starting value at time interval k + 1.
> 
> The comment for valueUpdateFunction is out-of-order compared with the param order in the function itself

Reordered to match.

>> LayoutTests/webaudio/resources/audioparam-testing.js:183
>> +        // Specify the target value, and how we should approach the target value.
> 
> target -> end

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:186
>> +        // Keep track of the start times, and the start and target values for each time interval.
> 
> target -> end

Done.

>> LayoutTests/webaudio/resources/audioparam-testing.js:193
>> +    return { times : timeInfo, values : valueInfo };
> 
> nit: extra spaces

Done.

[Chris Rogers]

Comment on attachment 128314 [details]
Patch

Looks good - thanks for the test!

[WebKit Review Bot]

Comment on attachment 128314 [details]
Patch

Clearing flags on attachment: 128314

Committed r108592: <http://trac.webkit.org/changeset/108592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.