Summary: | Exception should be thrown when connect() creates a cycle without DelayNode | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xingnan Wang <xingnan.wang> | ||||||
Component: | Web Audio | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | UNCONFIRMED --- | ||||||||
Severity: | Normal | CC: | crogers, eric.carlson, feature-media-reviews, haraken, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Xingnan Wang
2012-08-06 19:11:18 PDT
Created attachment 156833 [details]
Patch
Comment on attachment 156833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156833&action=review Thanks for the nice patch to fix this issue. Just a few small comments. > Source/WebCore/Modules/webaudio/AudioNode.cpp:73 > +bool AudioNode::checkCycle(AudioNode* startNode, AudioNode* endNode) Can we make checkCycle more explicit. Maybe checkForDelayFreeCycle? Perhaps a short comment can also be added saying we're looking for a path between startNode and endNode that does not contain a delayNode. Also, have you considered what happens if the delayNode has 0 delay? Convolvers could also have an arbitrarily long delay. (These are probably outside the scope of this fix.) > LayoutTests/webaudio/audionode-connect-cycle-expected.txt:1 > +This tests that we should throw an exception if a cycle is created without DelayNode in it. Typos: "we should throw" -> "we throw" and "without DelayNode" -> "without a DelayNode" > LayoutTests/webaudio/audionode-connect-cycle-expected.txt:5 > +PASS connect() is OK. These aren't very informative. Is there an easy way to provide a little more context? > LayoutTests/webaudio/audionode-connect-cycle.html:43 > + Is there any significance to the fact that the audio context length is 411 but the buffer length is 441? Or did you intend both to be 0.01 sec long? Created attachment 158252 [details]
Patch
(In reply to comment #2) > (From update of attachment 156833 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156833&action=review > > Thanks for the nice patch to fix this issue. Just a few small comments. > > > Source/WebCore/Modules/webaudio/AudioNode.cpp:73 > > +bool AudioNode::checkCycle(AudioNode* startNode, AudioNode* endNode) > > Can we make checkCycle more explicit. Maybe checkForDelayFreeCycle? Done. > > Perhaps a short comment can also be added saying we're looking for a path between startNode and endNode that does not contain a delayNode. > Done. > Also, have you considered what happens if the delayNode has 0 delay? Convolvers could also have an arbitrarily long delay. (These are probably outside the scope of this fix.) > That`s an interesting question. Will the sound grow gradually larger and larger and never end if delayNOde has 0 delay? Not sure what`s the result if a convolver in a cycle, I`d love to have a try. But now in spec only delayNode is allowed in cycle, do we have more use-cases to use a convolver in a cycle? > > LayoutTests/webaudio/audionode-connect-cycle-expected.txt:1 > > +This tests that we should throw an exception if a cycle is created without DelayNode in it. > > Typos: "we should throw" -> "we throw" and "without DelayNode" -> "without a DelayNode" > Done. > > LayoutTests/webaudio/audionode-connect-cycle-expected.txt:5 > > +PASS connect() is OK. > > These aren't very informative. Is there an easy way to provide a little more context? > Find a way to print the node name. > > LayoutTests/webaudio/audionode-connect-cycle.html:43 > > + > > Is there any significance to the fact that the audio context length is 411 but the buffer length is 441? Or did you intend both to be 0.01 sec long? My fault. Both should be 441. Comment on attachment 158252 [details]
Patch
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
|