[webkit-reviews] review denied: [Bug 44921] audio engine: add AudioChannel files : [Attachment 65996] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 31 18:30:54 PDT 2010
Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 44921: audio engine: add AudioChannel files
https://bugs.webkit.org/show_bug.cgi?id=44921
Attachment 65996: Patch
https://bugs.webkit.org/attachment.cgi?id=65996&action=review
------- Additional Comments from Kenneth Russell <kbr at google.com>
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 @@
> +2010-08-30 Chris Rogers <crogers at google.com>
> +
> + 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 <abarth at webkit.org>
>
> 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
More information about the webkit-reviews
mailing list