[webkit-reviews] review granted: [Bug 44712] Add audio cone effect files : [Attachment 65608] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Chris Rogers
<crogers at google.com>'s request for review:
Bug 44712: Add audio cone effect files
https://bugs.webkit.org/show_bug.cgi?id=44712

Attachment 65608: Patch
https://bugs.webkit.org/attachment.cgi?id=65608&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
Looks good; r=me. One minor issue and one comment.

> Index: WebCore/platform/audio/Cone.cpp
> ===================================================================
> --- WebCore/platform/audio/Cone.cpp	(revision 0)
> +++ WebCore/platform/audio/Cone.cpp	(revision 0)
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *	  notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *	  notice, this list of conditions and the following disclaimer in the
> + *	  documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *	  its contributors may be used to endorse or promote products derived
> + *	  from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +
> +#if ENABLE(WEB_AUDIO)
> +
> +#include "Cone.h"

Even though it must be being pulled in through some other header, you should
#include <math.h> here since you're referencing fabs and acos below.

> +
> +namespace WebCore {
> +
> +ConeEffect::ConeEffect()
> +    : m_innerAngle(360.0)
> +    , m_outerAngle(360.0)
> +    , m_outerGain(0.0)
> +{
> +}
> +
> +double ConeEffect::gain(Vector3 sourcePosition, Vector3 sourceOrientation,
Vector3 listenerPosition)
> +{
> +    if (sourceOrientation.isZero() || ((m_innerAngle == 360.0) &&
(m_outerAngle == 360.0)))
> +	   return 1.0; // no cone specified - unity gain
> +
> +    // Normalized source-listener vector
> +    Vector3 sourceToListener = listenerPosition - sourcePosition;
> +    sourceToListener.normalize();
> +
> +    Vector3 normalizedSourceOrientation = sourceOrientation;
> +    normalizedSourceOrientation.normalize();
> +
> +    // Angle between the source orientation vector and the source-listener
vector
> +    double dotProduct = dot(sourceToListener, normalizedSourceOrientation);
> +    double angle = 180.0 * acos(dotProduct) / M_PI;
> +    double absAngle = fabs(angle);
> +
> +    // Divide by 2.0 here since API is entire angle (not half-angle)
> +    double absInnerAngle = fabs(m_innerAngle) / 2.0;
> +    double absOuterAngle = fabs(m_outerAngle) / 2.0;
> +    double gain = 1.0;
> +
> +    if (absAngle <= absInnerAngle)
> +	   // No attenuation
> +	   gain = 1.0;
> +    else if (absAngle >= absOuterAngle)
> +	   // Max attenuation
> +	   gain = m_outerGain;
> +    else {
> +	   // Between inner and outer cones
> +	   // inner -> outer, x goes from 0 -> 1
> +	   double x = (absAngle - absInnerAngle) / (absOuterAngle -
absInnerAngle);
> +	   gain = (1.0 - x) + m_outerGain * x;
> +    }
> +
> +    return gain;
> +}
> +
> +} // namespace WebCore
> +
> +#endif // ENABLE(WEB_AUDIO)
> Index: WebCore/platform/audio/Cone.h
> ===================================================================
> --- WebCore/platform/audio/Cone.h	(revision 0)
> +++ WebCore/platform/audio/Cone.h	(revision 0)
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *	  notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *	  notice, this list of conditions and the following disclaimer in the
> + *	  documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *	  its contributors may be used to endorse or promote products derived
> + *	  from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef Cone_h
> +#define Cone_h
> +
> +#include <wtf/Vector3.h>

We've discussed offline the fact that Vector3 is in the wrong namespace. You'll
need to fix up references to it. It looks like a couple of options are adding a
"using WTF::Vector3;" at the end of wtf/Vector3.h, or adding "using
WTF::Vector3" at the top of your class declarations in your headers.

> +namespace WebCore {
> +
> +// Cone gain is defined according to the OpenAL specification
> +
> +class ConeEffect {
> +public:
> +    ConeEffect();
> +
> +    // Returns scalar gain for the given source/listener
positions/orientations
> +    double gain(Vector3 sourcePosition, Vector3 sourceOrientation, Vector3
listenerPosition);
> +
> +    // Angles in degrees
> +    void setInnerAngle(double innerAngle) { m_innerAngle = innerAngle; }
> +    double innerAngle() const { return m_innerAngle; }
> +
> +    void setOuterAngle(double outerAngle) { m_outerAngle = outerAngle; }
> +    double outerAngle() const { return m_outerAngle; }
> +
> +    void setOuterGain(double outerGain) { m_outerGain = outerGain; }
> +    double outerGain() const { return m_outerGain; }
> +
> +protected:
> +    double m_innerAngle;
> +    double m_outerAngle;
> +    double m_outerGain;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // Cone_h