Bug 76073 - noteOn, noteGrainOn and noteOff idl should take doubles
Summary: noteOn, noteGrainOn and noteOff idl should take doubles
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-11 10:12 PST by Raymond Toy
Modified: 2012-02-13 11:26 PST (History)
5 users (show)

See Also:


Attachments
Patch (36.58 KB, patch)
2012-01-13 09:44 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.
Comment 1 Raymond Toy 2012-01-13 09:44:46 PST
Created attachment 122443 [details]
Patch
Comment 2 Chris Rogers 2012-01-13 10:30:34 PST
This looks fine to me.  I assume it fixes the off-by-one sample-accurate-scheduling issues you were seeing?
Comment 3 Raymond Toy 2012-01-13 10:37:58 PST
(In reply to comment #2)
> This looks fine to me.  I assume it fixes the off-by-one sample-accurate-scheduling issues you were seeing?

Yes, it does, (once the typo is fixed).  I also verified a couple of values in gain.wav that the signal now starts at the expected place instead of being off by one.   The audiobuffersource-playbackrate tests also tests noteOff (which I did verify for one case).  noteGrainOn is untested.
Comment 4 Chris Rogers 2012-01-13 12:30:00 PST
Adam Barth, can you please provide final review?

Looks good, but please check on why "mac" EWS bot is purple.
Comment 5 Chris Rogers 2012-01-13 12:31:34 PST
(In reply to comment #3)
> (In reply to comment #2)
> > This looks fine to me.  I assume it fixes the off-by-one sample-accurate-scheduling issues you were seeing?
> 
> Yes, it does, (once the typo is fixed).  I also verified a couple of values in gain.wav that the signal now starts at the expected place instead of being off by one. 

Does that mean you have to re-baseline "gain.wav"?  If so, then you should include that as part of this patch.
Comment 6 Raymond Toy 2012-01-13 12:37:50 PST
(In reply to comment #4)
> Adam Barth, can you please provide final review?
> 
> Looks good, but please check on why "mac" EWS bot is purple.

Because non-committers can't submit patches.
Comment 7 Raymond Toy 2012-01-13 12:39:54 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > This looks fine to me.  I assume it fixes the off-by-one sample-accurate-scheduling issues you were seeing?
> > 
> > Yes, it does, (once the typo is fixed).  I also verified a couple of values in gain.wav that the signal now starts at the expected place instead of being off by one. 
> 
> Does that mean you have to re-baseline "gain.wav"?  If so, then you should include that as part of this patch.

The formatted diff shows a patch against gain-expected.wav.  Is that what you mean?  audiobuffersource-playbackrate-expected.wav was also updated.
Comment 8 Chris Rogers 2012-01-13 13:04:44 PST
Sorry, I missed that :)

Looks good - you can ping abarth for final review
Comment 9 Adam Barth 2012-01-13 15:17:07 PST
Comment on attachment 122443 [details]
Patch

The purple Mac EWS is expected for non-committer.
Comment 10 WebKit Review Bot 2012-01-13 16:02:09 PST
Comment on attachment 122443 [details]
Patch

Clearing flags on attachment: 122443

Committed r105007: <http://trac.webkit.org/changeset/105007>
Comment 11 WebKit Review Bot 2012-01-13 16:02:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 James Robinson 2012-01-14 00:58:43 PST
webaudio/gain.html is failing on the chromium win / linux bots since this patch landed. I don't know how to "view" a diff or how to rebaseline it.
Comment 13 Raymond Toy 2012-01-15 18:01:05 PST
(In reply to comment #12)
> webaudio/gain.html is failing on the chromium win / linux bots since this patch landed. I don't know how to "view" a diff or how to rebaseline it.

Sorry for the trouble.  I cannot reproduce this on my linux or mac box.  If you can get the actual gain-actual.wav and send it to me, that would be very helpful.
Comment 14 James Robinson 2012-01-15 22:11:17 PST
It's failing on every windows and linux bot. The bots upload a .zip containing all the actual files, here's one with a failing gain.html:
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win__dbg__2_/117740/layout-test-results.zip

I don't know what to make of gain-actual.wav.
Comment 15 Raymond Toy 2012-01-16 10:08:15 PST
(In reply to comment #14)
> It's failing on every windows and linux bot. The bots upload a .zip containing all the actual files, here's one with a failing gain.html:
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win__dbg__2_/117740/layout-test-results.zip
> 
> I don't know what to make of gain-actual.wav.

Thanks for the link.  One section of the actual wav file is one sample off compared to the expected wav file.  I will have to test this on a windows box later, but I think I know reason.  The reason is that windows still uses x87 instructions instead of sse2, so arithmetic is done with 80-bit precision.  (See http://code.google.com/p/chromium/issues/detail?id=106147#c6).  This messes up some comparisons because some have been rounded to double float precision, but some are still in 80-bit precision.

It will take a little bit to fix, but I won't be able to until I get to my windows box.

Thanks.
Comment 16 Raymond Toy 2012-01-30 09:21:09 PST
The test failures on windows should be fixed with bug 76659 (https://bugs.webkit.org/show_bug.cgi?id=76659), which has landed.
Comment 17 Raymond Toy 2012-01-31 16:05:39 PST
75996 is not blocked on this anymore.
Comment 18 Raymond Toy 2012-02-13 11:26:25 PST
Closing.  This issue fixed when bug 76659 landed.