Bug 46505 - Add AudioPannerNode files
Summary: Add AudioPannerNode files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-24 13:30 PDT by Chris Rogers
Modified: 2010-10-05 01:13 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.63 KB, patch)
2010-09-24 13:31 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (22.65 KB, patch)
2010-10-01 14:42 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (22.76 KB, patch)
2010-10-04 16:25 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (22.89 KB, patch)
2010-10-04 16:51 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-09-24 13:30:30 PDT
Add AudioPannerNode files
Comment 1 Chris Rogers 2010-09-24 13:31:39 PDT
Created attachment 68738 [details]
Patch
Comment 2 Chris Rogers 2010-09-24 13:33:15 PDT
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
Comment 3 Chris Rogers 2010-10-01 14:42:43 PDT
Created attachment 69522 [details]
Patch
Comment 4 Kenneth Russell 2010-10-04 15:43:28 PDT
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.
Comment 5 Chris Rogers 2010-10-04 16:25:33 PDT
Created attachment 69712 [details]
Patch
Comment 6 Chris Rogers 2010-10-04 16:31:22 PDT
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
Comment 7 Kenneth Russell 2010-10-04 16:47:44 PDT
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.
Comment 8 Chris Rogers 2010-10-04 16:51:37 PDT
Created attachment 69721 [details]
Patch
Comment 9 Kenneth Russell 2010-10-04 16:52:47 PDT
Comment on attachment 69721 [details]
Patch

Thanks for cleaning up that last nit. Looks good.
Comment 10 WebKit Commit Bot 2010-10-05 01:13:35 PDT
Comment on attachment 69721 [details]
Patch

Clearing flags on attachment: 69721

Committed r69093: <http://trac.webkit.org/changeset/69093>
Comment 11 WebKit Commit Bot 2010-10-05 01:13:41 PDT
All reviewed patches have been landed.  Closing bug.