[webkit-reviews] review denied: [Bug 44705] Add audio distance effect files : [Attachment 65595] Patch

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
Generally looks good, but a few minor issues.

> Index: WebCore/platform/audio/Distance.cpp
> ===================================================================
> --- WebCore/platform/audio/Distance.cpp	(revision 0)
> +++ WebCore/platform/audio/Distance.cpp	(revision 0)
> @@ -0,0 +1,90 @@
> +/*
> + * 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 "Distance.h"
> +
> +#include <math.h>
> +
> +namespace WebCore {
> +
> +DistanceEffect::DistanceEffect()
> +    : m_model(ModelInverse)
> +    , m_isClamped(true)
> +    , m_refDistance(1.0)
> +    , m_maxDistance(10000.0)
> +    , m_rolloffFactor(1.0)
> +{
> +}
> +
> +double DistanceEffect::gain(double distance)
> +{
> +    // don't go beyond maximum distance
> +    distance = distance < m_maxDistance ? distance : m_maxDistance;

Write this as std::min(distance, m_maxDistance). Requires #include <algorithm>.


> +    // if clamped, don't get closer than reference distance
> +    if (m_isClamped)
> +	   distance = distance > m_refDistance ? distance : m_refDistance;

Similarly, std::max(distance, m_refDistance).

> +    switch (m_model) {
> +    case ModelLinear:
> +	   return linearGain(distance);
> +	   break;
> +    case ModelInverse:
> +	   return inverseGain(distance);
> +	   break;
> +    case ModelExponential:
> +	   return exponentialGain(distance);
> +	   break;
> +
> +    default:
> +	   return 0.0;
> +    }
> +}
> +
> +double DistanceEffect::linearGain(double distance)
> +{
> +    return (1.0 - m_rolloffFactor * (distance - m_refDistance)) /
(m_maxDistance - m_refDistance);
> +}
> +
> +double DistanceEffect::inverseGain(double distance)
> +{
> +    return m_refDistance / (m_refDistance + m_rolloffFactor * (distance -
m_refDistance));
> +}
> +
> +double DistanceEffect::exponentialGain(double distance)
> +{
> +    return pow(distance / m_refDistance, -m_rolloffFactor);
> +}
> +
> +} // namespace WebCore
> +
> +#endif // ENABLE(WEB_AUDIO)
> Index: WebCore/platform/audio/Distance.h
> ===================================================================
> --- WebCore/platform/audio/Distance.h (revision 0)
> +++ WebCore/platform/audio/Distance.h (revision 0)
> @@ -0,0 +1,80 @@
> +/*
> + * 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 Distance_h
> +#define Distance_h
> +
> +namespace WebCore {
> +
> +// Distance models are defined according to the OpenAL specification
> +
> +class DistanceEffect {
> +public:
> +    enum {
> +	   ModelLinear = 0,
> +	   ModelInverse = 1,
> +	   ModelExponential = 2
> +    };

This enum should have a name, for example ModelType.

> +    DistanceEffect();
> +
> +    // Returns scalar gain for the given distance the current distance model
is used
> +    double gain(double distance);
> +
> +    int model() { return m_model; }

Should return enum name (e.g. ModelType).

> +    void setModel(int model, bool clamped)
> +    {
> +	   m_model = model;
> +	   m_isClamped = clamped;
> +    }

Should take e.g. ModelType as first argument.

> +    // Distance params
> +    void setRefDistance(double refDistance) { m_refDistance = refDistance; }

> +    void setMaxDistance(double maxDistance) { m_maxDistance = maxDistance; }

> +    void setRolloffFactor(double rolloffFactor) { m_rolloffFactor =
rolloffFactor; }
> +
> +    double refDistance() const { return m_refDistance; }
> +    double maxDistance() const { return m_maxDistance; }
> +    double rolloffFactor() const { return m_rolloffFactor; }
> +
> +protected:
> +    double linearGain(double distance);
> +    double inverseGain(double distance);
> +    double exponentialGain(double distance);
> +
> +    int m_model;

Should use enum type.

> +    bool m_isClamped;
> +    double m_refDistance;
> +    double m_maxDistance;
> +    double m_rolloffFactor;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // Distance_h