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.
Created attachment 124590 [details] Patch
Created attachment 124597 [details] Patch
Forgot to add expected results for the new test to the patch.
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.
Created attachment 124838 [details] Patch
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.
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
Created attachment 124963 [details] Patch
I think I've addressed all of the review comments and updated accordingly.
Looks good.
Comment on attachment 124963 [details] Patch rs=me
Comment on attachment 124963 [details] Patch Clearing flags on attachment: 124963 Committed r106612: <http://trac.webkit.org/changeset/106612>
All reviewed patches have been landed. Closing bug.