Bug 44921 – audio engine: add AudioChannel files

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

audio engine: add AudioChannel files

[Chris Rogers]

Created attachment 65996 [details]
Patch

[Kenneth Russell]

Comment on attachment 65996 [details]
Patch

Looks good overall, but r- because there's one place where there aren't enough assertions to catch a potential memory stomp.


> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 66445)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> [phone]  Chris Rogers  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        audio engine: add AudioChannel files
> +        https://bugs.webkit.org/show_bug.cgi?id=44921
> +
> +        No new tests since audio API is not yet implemented.
> +
> +        * platform/audio/AudioChannel.cpp: Added.
> +        (WebCore::AudioChannel::scale):
> +        (WebCore::AudioChannel::copyFrom):
> +        (WebCore::AudioChannel::copyFromRange):
> +        (WebCore::AudioChannel::sumFrom):
> +        (WebCore::AudioChannel::maxAbsValue):
> +        * platform/audio/AudioChannel.h: Added.
> +        (WebCore::AudioChannel::AudioChannel):
> +        (WebCore::AudioChannel::set):
> +        (WebCore::AudioChannel::length):
> +        (WebCore::AudioChannel::data):
> +        (WebCore::AudioChannel::zero):
> +
>  2010-08-30  Adam Barth  <[e-mail]>
>  
>          Reviewed by Eric Seidel.
> Index: WebCore/platform/audio/AudioChannel.cpp
> ===================================================================
> --- WebCore/platform/audio/AudioChannel.cpp	(revision 0)
> +++ WebCore/platform/audio/AudioChannel.cpp	(revision 0)
> @@ -0,0 +1,100 @@
> +/*
> + * 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 "AudioChannel.h"
> +
> +#include "Accelerate.h"
> +#include <math.h>
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +void AudioChannel::scale(double scale)
> +{
> +    float s = static_cast<float>(scale);
> +    vsmul(data(), 1, &s, data(), 1, length());
> +}
> +
> +void AudioChannel::copyFrom(const AudioChannel* sourceChannel)
> +{
> +    bool isSafe = (sourceChannel && sourceChannel->length() >= length());
> +    ASSERT(isSafe);
> +    if (!isSafe)
> +        return;
> +
> +    memcpy(data(), sourceChannel->data(), sizeof(float) * length());
> +}
> +
> +void AudioChannel::copyFromRange(const AudioChannel* sourceChannel, unsigned startFrame, unsigned endFrame)
> +{
> +    // Sanity checking
> +    bool isRangeSafe = sourceChannel && startFrame < endFrame && endFrame <= sourceChannel->length();
> +    ASSERT(isRangeSafe);
> +    if (!isRangeSafe)
> +        return;
> +
> +    size_t rangeLength = endFrame - startFrame;

Where are the guarantees that this AudioChannel has enough storage to cover rangeLength * sizeof(float) bytes?

> +
> +    const float* source = sourceChannel->data();
> +    float* destination = data();
> +    memcpy(destination, source + startFrame, sizeof(float) * rangeLength);
> +}
> +
> +void AudioChannel::sumFrom(const AudioChannel* sourceChannel)
> +{
> +    bool isSafe = sourceChannel && sourceChannel->length() >= length();
> +    ASSERT(isSafe);
> +    if (!isSafe)
> +        return;
> +
> +    vadd(data(), 1, sourceChannel->data(), 1, data(), 1, length());
> +}
> +
> +float AudioChannel::maxAbsValue() const
> +{
> +    const float* p = data();
> +    int n = length();
> +
> +    float max = 0.0f;
> +    while (n--) {
> +        float value = *p++;
> +        float absValue = fabsf(value);
> +        if (absValue > max)
> +            max = absValue;

Could just write "max = std::max(max, fabsf(*p++))".

> +    }
> +
> +    return max;
> +}
> +
> +} // WebCore
> +
> +#endif // ENABLE(WEB_AUDIO)
> Index: WebCore/platform/audio/AudioChannel.h
> ===================================================================
> --- WebCore/platform/audio/AudioChannel.h	(revision 0)
> +++ WebCore/platform/audio/AudioChannel.h	(revision 0)
> @@ -0,0 +1,113 @@
> +/*
> + * 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 AudioChannel_h
> +#define AudioChannel_h
> +
> +#include "AudioFloatArray.h"
> +#include <wtf/Noncopyable.h>
> +#include <wtf/PassOwnPtr.h>
> +
> +namespace WebCore {
> +
> +// An AudioChannel represents a buffer of non-interleaved floating-point audio samples.
> +// The PCM samples are normally assumed to be in a nominal range -1.0 -> +1.0
> +class AudioChannel : public Noncopyable {
> +public:
> +    // Memory can be externally referenced, or can be internally allocated with an AudioFloatArray.
> +
> +    // Reference an external buffer.
> +    AudioChannel(float* storage, size_t length)
> +        : m_length(length), m_rawPointer(storage), m_memBuffer(0) { }

It's unnecessary to explicitly initialize m_memBuffer.

> +
> +    // Manage storage for us.
> +    explicit AudioChannel(size_t length)
> +        : m_length(length)
> +        , m_rawPointer(0)
> +    {
> +        m_memBuffer = new AudioFloatArray(length);

adoptPtr(new AudioFloatArray(...))

> +    }
> +
> +    // A "blank" audio channel -- must call set() before it's useful...
> +    AudioChannel()
> +        : m_length(0)
> +        , m_rawPointer(0)
> +        , m_memBuffer(0)

Unnecessary to initialize m_memBuffer explicitly.

> +    {
> +    }
> +
> +    // Redefine the memory for this channel.
> +    // storage represents external memory not managed by this object.
> +    void set(float* storage, size_t length)
> +    {
> +        m_memBuffer.clear(); // cleanup managed storage
> +        m_rawPointer = storage;
> +        m_length = length;
> +    }
> +
> +    // How many sample-frames do we contain?
> +    size_t length() const { return m_length; }
> +
> +    // Direct access to PCM sample data
> +    float* data() { return m_rawPointer ? m_rawPointer : m_memBuffer->data(); }
> +    const float* data() const { return m_rawPointer ? m_rawPointer : m_memBuffer->data(); }
> +
> +    // Zeroes out all sample values in buffer.
> +    void zero()
> +    {
> +        if (m_memBuffer.get())
> +            m_memBuffer->zero();
> +        else
> +            memset(m_rawPointer, 0, sizeof(float) * m_length);
> +    }
> +
> +    // Scales all samples by the same amount.
> +    void scale(double scale);
> +
> +    // A simple memcpy() from the source channel
> +    void copyFrom(const AudioChannel* sourceChannel);
> +
> +    // Copies the given range from the source channel.
> +    void copyFromRange(const AudioChannel* sourceChannel, unsigned startFrame, unsigned endFrame);
> +
> +    // Sums (with unity gain) from the source channel.
> +    void sumFrom(const AudioChannel* sourceChannel);
> +
> +    // Returns maximum absolute value (useful for normalization).
> +    float maxAbsValue() const;
> +
> +private:
> +    size_t m_length;
> +
> +    float* m_rawPointer;
> +    OwnPtr<AudioFloatArray> m_memBuffer;
> +};
> +
> +} // WebCore
> +
> +#endif // AudioChannel_h

[Chris Rogers]

Created attachment 66243 [details]
Patch

[Chris Rogers]

Created attachment 66311 [details]
Patch

[Kenneth Russell]

Comment on attachment 66311 [details]
Patch

Looks good. r=me

[WebKit Commit Bot]

Comment on attachment 66311 [details]
Patch

Rejecting patch 66311 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Chris Rogers]

Comment on attachment 66311 [details]
Patch

Clearing flags on attachment: 66311

Committed r66758: <http://trac.webkit.org/changeset/66758>

[Chris Rogers]

All reviewed patches have been landed.  Closing bug.