In the sample-accurate-scheduling test line 106 is: var timeInSeconds = sampleOffsets[i] * sampleRate; I think this should probably be divide instead of multiply if sampleOffsets[i] really means a sample offset. If we change this to divide, then the test fails. I think the problem is that noteOn takes a float, so we convert the double from javascript to a float for the idl and then to a double inside the webaudio code. This conversion loses precision and the sample offset is off by one. We should change noteOn to take a double or maybe change the code that converts the time to sample to round instead of truncate.
Created attachment 121925 [details] Patch
Comment on attachment 121925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121925&action=review > Source/WebCore/ChangeLog:12 > + * webaudio/AudioBufferSourceNode.idl: Change parameters for > + noteOn, noteGrainOn, and noteOff to take doubles instead of > + floats. This seems unrelated to the typo in the layout test...
(In reply to comment #2) > (From update of attachment 121925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121925&action=review > > > Source/WebCore/ChangeLog:12 > > + * webaudio/AudioBufferSourceNode.idl: Change parameters for > > + noteOn, noteGrainOn, and noteOff to take doubles instead of > > + floats. > > This seems unrelated to the typo in the layout test... Ok. I'll only change noteOn and write another bug report that noteGrainOn and noteOff should take doubles instead of floats.
Created attachment 122037 [details] Patch
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 121925 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=121925&action=review > > > > > Source/WebCore/ChangeLog:12 > > > + * webaudio/AudioBufferSourceNode.idl: Change parameters for > > > + noteOn, noteGrainOn, and noteOff to take doubles instead of > > > + floats. > > > > This seems unrelated to the typo in the layout test... > > Ok. I'll only change noteOn and write another bug report that noteGrainOn and noteOff should take doubles instead of floats. See http://code.google.com/p/chromium/issues/detail?id=106147.
I think Adam was suggesting fixing the layout test in a separate patch from any change to the IDL? So two patches: 1) change IDL to doubles (new patch) 2) fix layout test (this patch) Is that correct? I think the changes from float->double in the IDL should happen in noteOn(), noteOff(), noteGrainOn() as a unit. It doesn't make much sense to *only* change noteOn() since these APIs are so closely related.
> Is that correct? Yeah, although it's a minor point. It's just a bit unusual to change a web-facing API in a bug about a typo in a layout test.
(In reply to comment #7) > > Is that correct? > > Yeah, although it's a minor point. It's just a bit unusual to change a web-facing API in a bug about a typo in a layout test. True. The name of this bug really should be something like "Improve precision of note scheduling" which makes it a little less strange.
Created attachment 122459 [details] Patch
Patch corrected with just the fix for the typo, and one additional test to make sure the expected number of events occurs. (This new test would have made the typo apparent.)
Comment on attachment 122459 [details] Patch Attachment 122459 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11237086 New failing tests: webaudio/sample-accurate-scheduling.html
Created attachment 124618 [details] Patch
Forgot to include correct expected results file. This test passes on linux and windows now, after bug 76659 landed.
Looks good.
Comment on attachment 124618 [details] Patch rs=me
Comment on attachment 124618 [details] Patch Clearing flags on attachment: 124618 Committed r106611: <http://trac.webkit.org/changeset/106611>
All reviewed patches have been landed. Closing bug.