Bug 83747

Summary: Add layout test case for JavaScriptAudioNode.
Product: WebKit Reporter: Raymond <rgbbones>
Component: Web AudioAssignee: Raymond <rgbbones>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, eric.carlson, feature-media-reviews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Raymond
Reported 2012-04-11 22:29:40 PDT
Add layout test case for JavaScriptAudioNode.
Attachments
Patch (6.90 KB, patch)
2012-04-11 22:30 PDT, Raymond 		no flags
DetailsFormatted DiffDiff
Patch (5.66 KB, patch)
2012-04-12 18:33 PDT, Raymond 		no flags
DetailsFormatted DiffDiff
Patch (5.99 KB, patch)
2012-04-12 20:15 PDT, Raymond 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Raymond
Comment 1 2012-04-11 22:30:24 PDT
Created attachment 136827 [details] Patch
Raymond
Comment 2 2012-04-11 22:33:37 PDT
Hi Chris Layout test case for JavaScriptAudioNode.Please take a review. Btw, I found JavaScriptAudioNode have some race condition issue when swap buffer. Might need to be fixed.
Chris Rogers
Comment 3 2012-04-12 12:07:33 PDT
Comment on attachment 136827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136827&action=review Raymond, thanks for the test. There will need to be some changes in JavaScriptAudioNode to be able to run properly in an "OfflineAudioContext" since it currently has some timing assumptions that the process() method is being called in "real-time" and not much faster than real-time like the OfflineAudioContext calls JavaScriptAudioNode::process() > LayoutTests/webaudio/javascriptaudionode-expected.txt:13 > +PASS Successfully created JavaScriptNode with bufferSize = 16384. nit: the correct name is JavaScriptAudioNode not JavaScriptNode > LayoutTests/webaudio/javascriptaudionode.html:14 > +description("Tests JavascriptAudioNode."); nit: JavaScriptAudioNode (capitalize the "S") > LayoutTests/webaudio/javascriptaudionode.html:80 > + } It seems unfortunate to copy/paste all of this code. Could you please have a look at the layout test for RealtimeAnalyserNode (where we test the different FFT sizes) for a better approach.
Raymond
Comment 4 2012-04-12 18:33:38 PDT
Created attachment 137017 [details] Patch
Raymond
Comment 5 2012-04-12 18:34:29 PDT
(In reply to comment #3) > It seems unfortunate to copy/paste all of this code. Could you please have a look at the layout test for RealtimeAnalyserNode (where we test the different FFT sizes) for a better approach. Hi Chris Thanks for the review. Patch updated.
Chris Rogers
Comment 6 2012-04-12 19:04:37 PDT
Comment on attachment 137017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137017&action=review > LayoutTests/webaudio/javascriptaudionode-expected.txt:6 > +PASS Exception been thrown for illegal bufferSize. been -> was > LayoutTests/webaudio/javascriptaudionode-expected.txt:14 > +PASS onaudioprocess been triggered with correct data. "been triggered" -> "was called" > LayoutTests/webaudio/javascriptaudionode.html:76 > + layoutTestController.overridePreference("WebKitWebAudioEnabled", "1"); Do we really need this here? Most of the other layout tests use the audio-testing.js script (in <head>) which handles this automatically I think. > LayoutTests/webaudio/javascriptaudionode.html:120 > + context.oncomplete = finishJSTest; I'm assuming that the processAudioData() method gets called before finishJSTest() is called because of the way callOnMainThread() works. In general, the JavaScriptAudioNode is not really ready for use with an OfflineAudioContext (for longer render times), but it looks like you've carefully arranged to have the renderLengthInFrames *exactly* equal the *bufferSize* to avoid this problem? If my understanding of this is correct, then it's probably worth describing this in a comment (just before line 120) to explain the current raciness with JavaScriptAudioNode and how we avoid it in this test...
Raymond
Comment 7 2012-04-12 20:10:32 PDT
(In reply to comment #6) Hi Chris. Thanks for the review. > > LayoutTests/webaudio/javascriptaudionode.html:76 > > + layoutTestController.overridePreference("WebKitWebAudioEnabled", "1"); > > Do we really need this here? Most of the other layout tests use the audio-testing.js script (in <head>) which handles this automatically I think. I just try to not use audio-testing.js ;) But anyway, fixed. > > > LayoutTests/webaudio/javascriptaudionode.html:120 > > + context.oncomplete = finishJSTest; > > I'm assuming that the processAudioData() method gets called before finishJSTest() is called because of the way callOnMainThread() works. > > In general, the JavaScriptAudioNode is not really ready for use with an OfflineAudioContext (for longer render times), but it looks like you've carefully arranged to > have the renderLengthInFrames *exactly* equal the *bufferSize* to avoid this problem? > > If my understanding of this is correct, then it's probably worth describing this in a comment (just before line 120) to explain the current raciness with JavaScriptAudioNode and how we avoid it in this test... Actually, since offlineAudioContext also notify finish event through callOnMainThread(), the oncomplete will be triggered after onaudioprocess. However, I did arrange the renderLengthInFrames and bufferSize to avoid the swap buffer issue in order to not have the event.inputBuffer been overwrite will next batch of data. otherwise, if the renderLengthInFrames is not multi time of the bufferSize, the remaining part of the inputBuffer will be filled with zeros and fail the test case. I will add a comments above renderLengthInFrames and bufferSize
Raymond
Comment 8 2012-04-12 20:15:52 PDT
Created attachment 137032 [details] Patch
Raymond
Comment 9 2012-04-12 20:21:43 PDT
patch updated.
Chris Rogers
Comment 10 2012-04-12 21:01:09 PDT
Raymond, I really appreciate the test.
WebKit Review Bot
Comment 11 2012-04-13 18:30:53 PDT
Comment on attachment 137032 [details] Patch Clearing flags on attachment: 137032 Committed r114196: <http://trac.webkit.org/changeset/114196>
WebKit Review Bot
Comment 12 2012-04-13 18:30:57 PDT
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.