Typo in sample-accurate-scheduling layout test?

[Raymond Toy]

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.

[Raymond Toy]

Created attachment 121925 [details]
Patch

[Adam Barth]

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]

(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]

Created attachment 122037 [details]
Patch

[Raymond Toy]

(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]

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]

> 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]

(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]

Created attachment 122459 [details]
Patch

[Raymond Toy]

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 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]

Created attachment 124618 [details]
Patch

[Raymond Toy]

Forgot to include correct expected results file.  This test passes on linux and windows now, after bug 76659 landed.

[Chris Rogers]

Looks good.

[Kenneth Russell]

Comment on attachment 124618 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 124618 [details]
Patch

Clearing flags on attachment: 124618

Committed r106611: <http://trac.webkit.org/changeset/106611>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.