Bug 93321 - Exception should be thrown when connect() creates a cycle without DelayNode
Summary: Exception should be thrown when connect() creates a cycle without DelayNode
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-06 19:11 PDT by Xingnan Wang
Modified: 2014-02-05 11:11 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.28 KB, patch)
2012-08-06 19:19 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2012-08-14 01:14 PDT, Xingnan Wang
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 Xingnan Wang 2012-08-06 19:19:30 PDT
Created attachment 156833 [details]
Patch
Comment 2 Raymond Toy 2012-08-13 15:59:51 PDT
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?
Comment 3 Xingnan Wang 2012-08-14 01:14:15 PDT
Created attachment 158252 [details]
Patch
Comment 4 Xingnan Wang 2012-08-14 01:47:04 PDT
(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 5 Anders Carlsson 2014-02-05 11:11:35 PST
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.