Bug 75996 - Typo in sample-accurate-scheduling layout test?
Summary: Typo in sample-accurate-scheduling layout test?
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-10 14:54 PST by Raymond Toy
Modified: 2012-02-02 18:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (39.47 KB, patch)
2012-01-10 15:52 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (39.17 KB, patch)
2012-01-11 08:53 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (3.35 KB, patch)
2012-01-13 10:56 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (3.29 KB, patch)
2012-01-30 15:50 PST, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond Toy 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.
Comment 1 Raymond Toy 2012-01-10 15:52:06 PST
Created attachment 121925 [details]
Patch
Comment 2 Adam Barth 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...
Comment 3 Raymond Toy 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.
Comment 4 Raymond Toy 2012-01-11 08:53:00 PST
Created attachment 122037 [details]
Patch
Comment 5 Raymond Toy 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.
Comment 6 Chris Rogers 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.
Comment 7 Adam Barth 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.
Comment 8 Chris Rogers 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.
Comment 9 Raymond Toy 2012-01-13 10:56:22 PST
Created attachment 122459 [details]
Patch
Comment 10 Raymond Toy 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.)
Comment 11 WebKit Review Bot 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
Comment 12 Raymond Toy 2012-01-30 15:50:40 PST
Created attachment 124618 [details]
Patch
Comment 13 Raymond Toy 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.
Comment 14 Chris Rogers 2012-01-31 18:25:08 PST
Looks good.
Comment 15 Kenneth Russell 2012-02-02 17:51:33 PST
Comment on attachment 124618 [details]
Patch

rs=me
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2012-02-02 18:28:38 PST
All reviewed patches have been landed.  Closing bug.