noteGrainOn needs more tests

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

Bug 76659 adds a timing test for noteGrainOn, but only tests the case where the offset is 0 and the duration is fixed.  That test also does not test that the output is correct, except for the position of the signal.

Additional tests need to be written to cover the case where the offset and duration change.  We also need to test whether the correct part of the signal is produced.

[Raymond Toy]

Created attachment 124590 [details]
Patch

[Raymond Toy]

Created attachment 124597 [details]
Patch

[Raymond Toy]

Forgot to add expected results for the new test to the patch.

[Chris Rogers]

Comment on attachment 124597 [details]
Patch

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

Great test! I have a few nit comments:

> LayoutTests/webaudio/note-grain-on-play.html:14
> +      description("Test noteGrainOn.");

Can we have a *slightly* more detailed description of the test here -- could even be as simple as:

"Test noteGrainOn offset rendering."

> LayoutTests/webaudio/note-grain-on-play.html:17
> +      // Various sections of the ramp are played out at different

"played out" -> "rendered with noteGrainOn()"

> LayoutTests/webaudio/note-grain-on-play.html:25
> +      var trueGrainOffset = [];

can we call this either "trueGrainOffsetTime" or "grainOffsetTime"

> LayoutTests/webaudio/note-grain-on-play.html:30
> +          var rampOffset = timeToSampleFrame(trueGrainOffset[rampNumber], sampleRate);

"rampOffset" -> "rampOffsetFrame"

> LayoutTests/webaudio/note-grain-on-play.html:31
> +          var rampLength = endFrame - startFrame;

rampLength -> rampFrameLength

> LayoutTests/webaudio/note-grain-on-play.html:72
> +          // section of our ramp signal.

It seems like this comment really belongs just inside the loop, but we want a comment here describing that we're looping through each of the rendered grains, one at a time...

> LayoutTests/webaudio/note-grain-on-play.html:76
> +                  testFailed("Ramp " + k + " incorrect.  Expected " + result.expected + " but received " + result.actual + " at sample frame " + result.frame);

I'm wondering if "Grain" might be a better word than "Ramp", since I think of the "ramp" as being the entire "linearRampBuffer", while the "grain" is a single rendering of a portion of the ramp.  It's true that a sub-section of the ramp is itself a ramp, but that doesn't seem helpful in trying to distinguish the concepts.

So: "verifyRamp" -> "verifyGrain", and other uses of word ramp...

> LayoutTests/webaudio/note-grain-on-play.html:108
> +          linearRampBuffer = createSignalBuffer(context, function (k) { return k + 1; });    

nit: no space after function -- should be "function(k)"

Also, please add comment explaining that we use "k + 1" instead of "k" so that the ramp starts at a non-zero value.

[Raymond Toy]

Created attachment 124838 [details]
Patch

[Raymond Toy]

Comment on attachment 124597 [details]
Patch

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

>> LayoutTests/webaudio/note-grain-on-play.html:14
>> +      description("Test noteGrainOn.");
> 
> Can we have a *slightly* more detailed description of the test here -- could even be as simple as:
> 
> "Test noteGrainOn offset rendering."

Oops.  Better description added.

>> LayoutTests/webaudio/note-grain-on-play.html:17
>> +      // Various sections of the ramp are played out at different
> 
> "played out" -> "rendered with noteGrainOn()"

Done.

>> LayoutTests/webaudio/note-grain-on-play.html:25
>> +      var trueGrainOffset = [];
> 
> can we call this either "trueGrainOffsetTime" or "grainOffsetTime"

Done.

>> LayoutTests/webaudio/note-grain-on-play.html:30
>> +          var rampOffset = timeToSampleFrame(trueGrainOffset[rampNumber], sampleRate);
> 
> "rampOffset" -> "rampOffsetFrame"

Done.

>> LayoutTests/webaudio/note-grain-on-play.html:31
>> +          var rampLength = endFrame - startFrame;
> 
> rampLength -> rampFrameLength

Done.

I've also made similar changes in other places to make it clear if we're doing time or frames.

>> LayoutTests/webaudio/note-grain-on-play.html:72
>> +          // section of our ramp signal.
> 
> It seems like this comment really belongs just inside the loop, but we want a comment here describing that we're looping through each of the rendered grains, one at a time...

Done.

>> LayoutTests/webaudio/note-grain-on-play.html:76
>> +                  testFailed("Ramp " + k + " incorrect.  Expected " + result.expected + " but received " + result.actual + " at sample frame " + result.frame);
> 
> I'm wondering if "Grain" might be a better word than "Ramp", since I think of the "ramp" as being the entire "linearRampBuffer", while the "grain" is a single rendering of a portion of the ramp.  It's true that a sub-section of the ramp is itself a ramp, but that doesn't seem helpful in trying to distinguish the concepts.
> 
> So: "verifyRamp" -> "verifyGrain", and other uses of word ramp...

Yes, I agree.  I think I've made this change everywhere.

>> LayoutTests/webaudio/note-grain-on-play.html:108
>> +          linearRampBuffer = createSignalBuffer(context, function (k) { return k + 1; });    
> 
> nit: no space after function -- should be "function(k)"
> 
> Also, please add comment explaining that we use "k + 1" instead of "k" so that the ramp starts at a non-zero value.

Done.

[Chris Rogers]

Comment on attachment 124838 [details]
Patch

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

> LayoutTests/webaudio/note-grain-on-play-expected.txt:7
> +PASS All 100 pulses ended at the correct time.

Lines 5:7 use the word "pulse" but line 8 uses "grain" -- it seems like "grain" should be used everywhere since it's no longer a square pulse

> LayoutTests/webaudio/note-grain-on-play.html:18
> +      // out at different times, and we verify that the actual output

nit: can remove the word "out"

> LayoutTests/webaudio/note-grain-on-play.html:19
> +      // matches our expectations.

"verify that the actual output matches our expectations" is pretty generic and vague.  It could be used to describe any test.  Can we have a sentence that's much more explicit?

> LayoutTests/webaudio/note-grain-on-play.html:29
> +      function verifyGrain(renderedData, startFrame, endFrame, rampNumber) {

rampNumber -> grainIndex (or grainNumber)

> LayoutTests/webaudio/note-grain-on-play.html:30
> +          var rampOffsetFrame = timeToSampleFrame(grainOffsetTime[rampNumber], sampleRate);

rampOffsetFrame -> grainOffsetFrame

> LayoutTests/webaudio/note-grain-on-play.html:31
> +          var rampFrameLength = endFrame - startFrame;

rampFrameLength -> grainFrameLength

> LayoutTests/webaudio/note-grain-on-play.html:64
> +          var startEndFrames = findStartAndEndSamples(renderedData);

naming nit: findStartAndEndSamples -> findStartEndFrames

(down below you have a function called "verifyStartEndFrames" without an "And")

either both should have "And" or neither

[Raymond Toy]

Created attachment 124963 [details]
Patch

[Raymond Toy]

I think I've addressed all of the review comments and updated accordingly.

[Chris Rogers]

Looks good.

[Kenneth Russell]

Comment on attachment 124963 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 124963 [details]
Patch

Clearing flags on attachment: 124963

Committed r106612: <http://trac.webkit.org/changeset/106612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.