WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
75996
Typo in sample-accurate-scheduling layout test?
https://bugs.webkit.org/show_bug.cgi?id=75996
Summary
Typo in sample-accurate-scheduling layout test?
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
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
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.
Top of Page
Format For Printing
XML
Clone This Bug