Bug 75996

Summary: Typo in sample-accurate-scheduling layout test?
Product: WebKit Reporter: Raymond Toy <rtoy>
Component: Web AudioAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, crogers, dglazkov, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Raymond Toy
Reported 2012-01-10 14:54:51 PST
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.
Attachments
Patch (39.47 KB, patch)
2012-01-10 15:52 PST, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Patch (39.17 KB, patch)
2012-01-11 08:53 PST, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Patch (3.35 KB, patch)
2012-01-13 10:56 PST, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Patch (3.29 KB, patch)
2012-01-30 15:50 PST, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Raymond Toy
Comment 1 2012-01-10 15:52:06 PST
Created attachment 121925 [details] Patch
Adam Barth
Comment 2 2012-01-10 16:22:07 PST
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...
Raymond Toy
Comment 3 2012-01-10 16:25:39 PST
(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.
Raymond Toy
Comment 4 2012-01-11 08:53:00 PST
Created attachment 122037 [details] Patch
Raymond Toy
Comment 5 2012-01-11 10:09:36 PST
(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.
Chris Rogers
Comment 6 2012-01-12 20:35:44 PST
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.
Adam Barth
Comment 7 2012-01-12 20:48:37 PST
> 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.
Chris Rogers
Comment 8 2012-01-12 21:08:33 PST
(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.
Raymond Toy
Comment 9 2012-01-13 10:56:22 PST
Created attachment 122459 [details] Patch
Raymond Toy
Comment 10 2012-01-13 10:57:55 PST
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.)
WebKit Review Bot
Comment 11 2012-01-13 12:48:25 PST
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
Raymond Toy
Comment 12 2012-01-30 15:50:40 PST
Created attachment 124618 [details] Patch
Raymond Toy
Comment 13 2012-01-30 15:53:09 PST
Forgot to include correct expected results file. This test passes on linux and windows now, after bug 76659 landed.
Chris Rogers
Comment 14 2012-01-31 18:25:08 PST
Looks good.
Kenneth Russell
Comment 15 2012-02-02 17:51:33 PST
Comment on attachment 124618 [details] Patch rs=me
WebKit Review Bot
Comment 16 2012-02-02 18:28:31 PST
Comment on attachment 124618 [details] Patch Clearing flags on attachment: 124618 Committed r106611: <http://trac.webkit.org/changeset/106611>
WebKit Review Bot
Comment 17 2012-02-02 18:28:38 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.