| Summary: | AudioParam must support connections from audio-rate signals | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||||
| Component: | New Bugs | Assignee: | Chris Rogers <crogers> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, eric.carlson, feature-media-reviews, kbr, ojan, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Chris Rogers
2012-04-09 16:40:11 PDT
Created attachment 136342 [details]
Patch
Comment on attachment 136342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136342&action=review > Source/WebCore/Modules/webaudio/AudioNode.cpp:163 > + // Sanity check input and output indices. > + if (outputIndex >= numberOfOutputs()) { Nit: Input index? In any case, I am not sure that this comment adds much value. > Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:143 > + ASSERT(context()->isGraphOwner()); > + return m_inputs.size(); The header comment notes that this must be called with the context's graph lock. Is it possible to ASSERT that this is true? > Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:201 > + ASSERT(context()->isGraphOwner()); > + > + ASSERT(param); > + if (!param) > + return; Can an AudioParam have a different context from the AudioNodeOutput? > Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:210 > + ASSERT(context()->isGraphOwner()); > + > + ASSERT(param); Ditto. > Source/WebCore/Modules/webaudio/AudioParam.cpp:112 > + // FIXME: support fan-in (multiple audio connections to this parameter with unity-gain summing). Nit: Is there a bug for this FIXME? > Source/WebCore/Modules/webaudio/AudioParam.cpp:169 > + // FIXME: support fan-in (multiple audio connections to this parameter with unity-gain summing). Ditto. Comment on attachment 136342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136342&action=review Eric, thanks for having a look. I think I've addressed your comments and have also added some details in the ChangeLog. >> Source/WebCore/Modules/webaudio/AudioNode.cpp:163 >> + if (outputIndex >= numberOfOutputs()) { > > Nit: Input index? In any case, I am not sure that this comment adds much value. Fixed: removed comment >> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:143 >> + return m_inputs.size(); > > The header comment notes that this must be called with the context's graph lock. Is it possible to ASSERT that this is true? Already have it on line 142 >> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:201 >> + return; > > Can an AudioParam have a different context from the AudioNodeOutput? Yes, it's possible if there are several AudioContexts which have been created. We have a sanity check in the newer AudioNode::connect(AudioParam*) method which nips this in the bud very early on: if (context() != param->context()) { ec = SYNTAX_ERR; return; } By the way, we already have a similar sanity check for AudioNode::connect(AudioNode*) (connecting an AudioNode -> AudioNode) since the contexts may be different. >> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:210 >> + ASSERT(param); > > Ditto. See response for addParam() above >> Source/WebCore/Modules/webaudio/AudioParam.cpp:112 >> + // FIXME: support fan-in (multiple audio connections to this parameter with unity-gain summing). > > Nit: Is there a bug for this FIXME? Created bug and added link in the comment. >> Source/WebCore/Modules/webaudio/AudioParam.cpp:169 >> + // FIXME: support fan-in (multiple audio connections to this parameter with unity-gain summing). > > Ditto. Added bug link. Created attachment 136527 [details]
Patch
Comment on attachment 136342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136342&action=review >>> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:143 >>> + return m_inputs.size(); >> >> The header comment notes that this must be called with the context's graph lock. Is it possible to ASSERT that this is true? > > Already have it on line 142 Oops, so *that* is what "isGraphOwner" checks :-) >>> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:201 >>> + return; >> >> Can an AudioParam have a different context from the AudioNodeOutput? > > Yes, it's possible if there are several AudioContexts which have been created. We have a sanity check in the newer AudioNode::connect(AudioParam*) method which nips this in the bud very early on: > > if (context() != param->context()) { > ec = SYNTAX_ERR; > return; > } > > By the way, we already have a similar sanity check for AudioNode::connect(AudioNode*) (connecting an AudioNode -> AudioNode) since the contexts may be different. I saw that, which is why I wondered about this. So you are saying that an AudioParam will always be connected to an AudioNode before it is added to a node output? Committed r113769: <http://trac.webkit.org/changeset/113769> (In reply to comment #5) > (From update of attachment 136342 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136342&action=review > > >>> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:143 > >>> + return m_inputs.size(); > >> > >> The header comment notes that this must be called with the context's graph lock. Is it possible to ASSERT that this is true? > > > > Already have it on line 142 > > Oops, so *that* is what "isGraphOwner" checks :-) > > >>> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:201 > >>> + return; > >> > >> Can an AudioParam have a different context from the AudioNodeOutput? > > > > Yes, it's possible if there are several AudioContexts which have been created. We have a sanity check in the newer AudioNode::connect(AudioParam*) method which nips this in the bud very early on: > > > > if (context() != param->context()) { > > ec = SYNTAX_ERR; > > return; > > } > > > > By the way, we already have a similar sanity check for AudioNode::connect(AudioNode*) (connecting an AudioNode -> AudioNode) since the contexts may be different. > > I saw that, which is why I wondered about this. So you are saying that an AudioParam will always be connected to an AudioNode before it is added to a node output? Eric, thanks for the review! Yes, the whole process is started with a JavaScript call to connect() which is implemented in the AudioNode::connect() method, so we're catching mis-matching contexts very early before any other of this lower-level code gets executed. |