Bug 46505 – Add AudioPannerNode files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add AudioPannerNode files

[Chris Rogers]

Created attachment 68738 [details]
Patch

[Chris Rogers]

This is the implementation for AudioPannerNode as described in the web audio API:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioPannerNode-section

[Chris Rogers]

Created attachment 69522 [details]
Patch

[Kenneth Russell]

Comment on attachment 69522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69522&action=review

This basically looks good to me. Please update the license text and take a look at the few highlighted issues. It's OK with me if you fix those upon landing the patch.

> WebCore/webaudio/AudioPannerNode.cpp:47
> +static void fixNANS(double &x)

Naming convention: fixNANS -> fixNANs

> WebCore/webaudio/AudioPannerNode.cpp:95
> +    AudioBus* destination = output(0)->bus();

Out of curiosity, why does AudioPannerNode::process not need the same sort of locking as AudioBasicProcessorNode::process? Is it because the number of channels of the input to AudioPannerNode can't change at run time?

> WebCore/webaudio/AudioPannerNode.cpp:191
> +    listenerFrontNorm.normalize();

Since listenerFront isn't used after this point you could just rename listenerFront to listenerFrontNorm and normalize it earlier.

> WebCore/webaudio/AudioPannerNode.cpp:220
> +    if (elevation < -90.0)

Presumably this should be "else if".

> WebCore/webaudio/AudioPannerNode.cpp:284
> +    m_distanceGain->setValue((float)distanceGain);

Avoid C-style casts.

> WebCore/webaudio/AudioPannerNode.cpp:289
> +    m_coneGain->setValue((float)coneGain);

Avoid C-style casts.

> WebCore/webaudio/AudioPannerNode.h:80
> +    void setPanningModel(unsigned model);

These should take and return unsigned short at a minimum. Ideally the above enum would be named and these return and take the enum name, but I'm not sure that will compile given that the IDL uses unsigned short (try it).

> WebCore/webaudio/AudioPannerNode.h:96
> +    void setDistanceModel(unsigned model) { m_distanceEffect.setModel(static_cast<DistanceEffect::ModelType>(model), true); }

It looks like DistanceEffect hasn't landed yet but at a minimum these should return and take unsigned short, if not DistanceEffect::ModelType (not sure whether that will compile while still having the IDL return unsigned short).

> WebCore/webaudio/AudioPannerNode.h:139
> +    Vector3 m_velocity;

These should change to use FloatPoint3D, if not now then soon.

> WebCore/webaudio/AudioPannerNode.idl:42
> +        attribute long panningModel;

Shouldn't this be "unsigned short" to match the types of the Panning Model values above? The spec would also need to change.

> WebCore/webaudio/AudioPannerNode.idl:50
> +        attribute unsigned long distanceModel;

Presumably this should be "unsigned short" both here and in the spec to match panningModel, even though the distance model constants aren't defined yet.

[Chris Rogers]

Created attachment 69712 [details]
Patch

[Chris Rogers]

Comment on attachment 69522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69522&action=review

>> WebCore/webaudio/AudioPannerNode.cpp:47
>> +static void fixNANS(double &x)
> 
> Naming convention: fixNANS -> fixNANs

FIXED

>> WebCore/webaudio/AudioPannerNode.cpp:95
>> +    AudioBus* destination = output(0)->bus();
> 
> Out of curiosity, why does AudioPannerNode::process not need the same sort of locking as AudioBasicProcessorNode::process? Is it because the number of channels of the input to AudioPannerNode can't change at run time?

Actually the number of channels *can* change, but the panning algorithms do not require any complex re-initialization to handle the different cases since they can simply call different code paths.

>> WebCore/webaudio/AudioPannerNode.cpp:220
>> +    if (elevation < -90.0)
> 
> Presumably this should be "else if".

FIXED

>> WebCore/webaudio/AudioPannerNode.cpp:284
>> +    m_distanceGain->setValue((float)distanceGain);
> 
> Avoid C-style casts.

FIXED

>> WebCore/webaudio/AudioPannerNode.cpp:289
>> +    m_coneGain->setValue((float)coneGain);
> 
> Avoid C-style casts.

FIXED

>> WebCore/webaudio/AudioPannerNode.h:139
>> +    Vector3 m_velocity;
> 
> These should change to use FloatPoint3D, if not now then soon.

added a FIXME

[Kenneth Russell]

Comment on attachment 69712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69712&action=review

> WebCore/webaudio/AudioPannerNode.h:96
> +    void setDistanceModel(unsigned model) { m_distanceEffect.setModel(static_cast<DistanceEffect::ModelType>(model), true); }

Per earlier review please change the arguments and return values to unsigned short.

> WebCore/webaudio/AudioPannerNode.idl:42
> +        attribute unsigned long panningModel;

I gather from our offline discussion that the generated glue code doesn't work if these attributes are changed to "unsigned short". It's worth filing a low-priority follow-up bug against the code generators (I'm not sure which would be the best component; "WebCore JavaScript" is a guess) about that.

[Chris Rogers]

Created attachment 69721 [details]
Patch

[Kenneth Russell]

Comment on attachment 69721 [details]
Patch

Thanks for cleaning up that last nit. Looks good.

[WebKit Commit Bot]

Comment on attachment 69721 [details]
Patch

Clearing flags on attachment: 69721

Committed r69093: <http://trac.webkit.org/changeset/69093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.