Bug 83524 – AudioParam must support connections from audio-rate signals

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

AudioParam must support connections from audio-rate signals

[Chris Rogers]

Created attachment 136342 [details]
Patch

[Eric Carlson]

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.

[Chris Rogers]

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.

[Chris Rogers]

Created attachment 136527 [details]
Patch

[Eric Carlson]

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?

[Chris Rogers]

Committed r113769: <http://trac.webkit.org/changeset/113769>

[Chris Rogers]

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