[webkit-reviews] review granted: [Bug 44995] Add AudioParam files : [Attachment 66119] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 2 16:40:48 PDT 2010


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

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
Marking r+ with a few minor comments. Fix upon committing, or upload new patch.


> Index: WebCore/webaudio/AudioParam.h
> ===================================================================
> --- WebCore/webaudio/AudioParam.h	(revision 0)
> +++ WebCore/webaudio/AudioParam.h	(revision 0)
> @@ -0,0 +1,115 @@
> +/*
> + * 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 AudioParam_h
> +#define AudioParam_h
> +
> +#include "PlatformString.h"
> +#include <math.h>
> +#include <sys/types.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +
> +namespace WebCore {
> +
> +class AudioParam : public RefCounted<AudioParam> {
> +public:
> +    static const double DefaultSmoothingConstant = 0.05;
> +    static const double SnapThreshold = 0.001;

I think you're going to have problems getting this to link on all platforms (or
maybe not, since you're developing on the Mac and clearly things are linking
for you). I believe you need the following in an AudioParam.cc for correctness.


// GCC requires these declarations, but MSVC requires they not be present
#ifndef COMPILER_MSVC
const double AudioParam::DefaultSmoothingConstant;
const double AudioParam::SnapThreshold;
#endif

It might only be necessary if these are referenced from another .cpp file; not
sure. It looks like SnapThreshold doesn't need to be accessed from outside this
class in which case it should be private. If the same is true for
DefaultSmoothingConstant then making them both private may solve the problem.

> +
> +    static PassRefPtr<AudioParam> create(const String& name, double
defaultValue, double minValue, double maxValue, unsigned units = 0)
> +    {
> +	   return adoptRef(new AudioParam(name, defaultValue, minValue,
maxValue, units));
> +    }
> +
> +    AudioParam(const String& name, double defaultValue, double minValue,
double maxValue, unsigned units = 0)
> +	   : m_name(name)
> +	   , m_value(defaultValue)
> +	   , m_defaultValue(defaultValue)
> +	   , m_minValue(minValue)
> +	   , m_maxValue(maxValue)
> +	   , m_units(units)
> +	   , m_smoothedValue(defaultValue)
> +	   , m_smoothingConstant(DefaultSmoothingConstant)
> +    {
> +    }
> +
> +    float value() const { return static_cast<float>(m_value); }
> +    void setValue(float value) { m_value = value; }
> +
> +    String name() const { return m_name; }
> +
> +    float minValue() const { return static_cast<float>(m_minValue); }
> +    float maxValue() const { return static_cast<float>(m_maxValue); }
> +    float defaultValue() const { return static_cast<float>(m_defaultValue);
}
> +    unsigned units() const { return m_units; }

I'd add a FIXME here too about defining units.

> +
> +    // Value smoothing:
> +
> +    // When a new value is set with setValue(), in our internal use of the
parameter we don't immediately jump to it.
> +    // Instead we smoothly approach this value to avoid glitching.
> +    float smoothedValue() const { return
static_cast<float>(m_smoothedValue); }

These casts between floats and doubles are verbose. I think you should consider
changing all of these implementing classes to use floats internally rather than
doubles, and pass floats around in the API.

> +
> +    // Smoothly exponentially approaches to (de-zippers) the desired value.
> +    // Returns true if smoothed value has already snapped exactly to value.
> +    bool smooth()
> +    {
> +	   if (m_smoothedValue == m_value) {
> +	       // Smoothed value has already approached and snapped to value.
> +	       return true;
> +	   }
> +
> +	   // Exponential approach
> +	   m_smoothedValue += (m_value - m_smoothedValue) *
m_smoothingConstant;
> +
> +	   // If we get close enough then snap to actual value.
> +	   if (fabs(m_smoothedValue - m_value) < SnapThreshold) // FIXME: the
threshold needs to be adjustable depending on range - but this is OK general
purpose value.
> +	       m_smoothedValue = m_value;
> +
> +	   return false;
> +    }
> +
> +    void resetSmoothedValue() { m_smoothedValue = m_value; }
> +    void setSmoothingConstant(double k) { m_smoothingConstant = k; }
> +
> +private:
> +    String m_name;
> +    double m_value;
> +    double m_defaultValue;
> +    double m_minValue;
> +    double m_maxValue;
> +    unsigned m_units;
> +
> +    // Smoothing (de-zippering)
> +    double m_smoothedValue;
> +    double m_smoothingConstant;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // AudioParam_h
> Index: WebCore/webaudio/AudioParam.idl
> ===================================================================
> --- WebCore/webaudio/AudioParam.idl	(revision 0)
> +++ WebCore/webaudio/AudioParam.idl	(revision 0)
> @@ -0,0 +1,43 @@
> +/*
> + * 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.
> + */
> +
> +module webaudio {
> +    interface [
> +	   Conditional=WEB_AUDIO
> +    ] AudioParam {
> +	   attribute float value;
> +	   readonly attribute float minValue;
> +	   readonly attribute float maxValue;
> +	   readonly attribute float defaultValue;
> +	   
> +	   readonly attribute DOMString name;
> +
> +	   // FIXME: Could define units constants here (seconds, decibels,
cents, etc.)...
> +	   readonly attribute unsigned short units;
> +    };
> +}


More information about the webkit-reviews mailing list