[Webkit-unassigned] [Bug 46505] Add AudioPannerNode files
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 4 15:43:28 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=46505
Kenneth Russell <kbr at google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #69522|review? |review+
Flag| |
--- Comment #4 from Kenneth Russell <kbr at google.com> 2010-10-04 15:43:28 PST ---
(From update of attachment 69522)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list