Bug 93321 – Exception should be thrown when connect() creates a cycle without DelayNode

• Component: Web Audio
• Status: UNCONFIRMED

[Xingnan Wang]

https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#methodsandparams-AudioNode

[Xingnan Wang]

Created attachment 156833 [details]
Patch

[Raymond Toy]

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?

[Xingnan Wang]

Created attachment 158252 [details]
Patch

[Xingnan Wang]

(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.

[Anders Carlsson]

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.