[Webkit-unassigned] [Bug 34912] audio engine: add ReverbConvolver class
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 22 08:31:33 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=34912
--- Comment #16 from Jeremy Orlow <jorlow at chromium.org> 2010-03-22 08:31:33 PST ---
(From update of attachment 50957)
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbAccumulationBuffer.cpp
> @@ -0,0 +1,101 @@
> +/*
> + * 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"
> +#include "ReverbAccumulationBuffer.h"
> +
> +#include "Accelerate.h"
> +
> +namespace WebCore {
> +
> +ReverbAccumulationBuffer::ReverbAccumulationBuffer(size_t length)
> + : m_buffer(length)
> + , m_length(length)
> + , m_readIndex(0)
> + , m_readTimeFrame(0)
> +{
> +}
> +
> +void ReverbAccumulationBuffer::readAndClear(float* destP, size_t nframes)
destP and nframes are not the best variables names. Same goes for nframes1 and
2. I don't think it gets too much in the way of readability here, but it's a
good practice to get into...even if it means a bit more typing and thinking
about what to name stuff.
> +{
> + ASSERT(m_readIndex <= m_length);
> + size_t framesAvailable = m_length - m_readIndex;
> + size_t nframes1 = (nframes <= framesAvailable) ? nframes : framesAvailable;
min(nframes, framseAvailable)
> + size_t nframes2 = nframes - nframes1;
> +
> + float* sourceP = m_buffer;
> + memcpy(destP, sourceP + m_readIndex, sizeof(float) * nframes1);
> + memset(sourceP + m_readIndex, 0, sizeof(float) * nframes1);
> +
> + // Handle wrap-around if necessary
> + if (nframes2 > 0) {
> + memcpy(destP + nframes1, sourceP, sizeof(float) * nframes2);
> + memset(sourceP, 0, sizeof(float) * nframes2);
> + }
> +
> + m_readIndex = (m_readIndex + nframes) % m_length;
You could avoid the division by adding |m_readIndex = nframes2| in the if and
adding an else to it that sets |m_readIndex += nframes1|. Its probably over
optimizing, but... (Your choice.)
> + m_readTimeFrame += nframes;
> +}
> +
> +void ReverbAccumulationBuffer::updateReadIndex(int* readIndex, size_t nframes)
> +{
First of all, this could be const.
Seconly, I'm not sure whether this is better or it's better to do this logic in
the caller. If you had a length getter method, then you could just do this up
one level. On the other hand, I see this is a weird case of a more general
pattern that you follow. If you still think this is best, it's OK with me
though.
> + // Update caller's readIndex
> + *readIndex = (*readIndex + nframes) % m_length;
> +}
> +
> +int ReverbAccumulationBuffer::accumulate(float* sourceP, size_t nframes, int* readIndex, size_t delayFrames)
> +{
> + size_t writeIndex = (*readIndex + delayFrames) % m_length;
> +
> + // Update caller's readIndex
> + *readIndex = (*readIndex + nframes) % m_length;
If you change above, change this.
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbInputBuffer.cpp
> @@ -0,0 +1,70 @@
> +/*
> + * 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"
> +#include "ReverbInputBuffer.h"
> +
> +namespace WebCore {
> +
> +ReverbInputBuffer::ReverbInputBuffer(size_t length)
> + : m_buffer(length)
> + , m_length(length)
> + , m_writeIndex(0)
> +{
> +}
> +
> +void ReverbInputBuffer::write(float* sourceP, size_t nframes)
> +{
> + memcpy(m_buffer.data() + m_writeIndex, sourceP, sizeof(float) * nframes);
> +
> + m_writeIndex += nframes;
> + ASSERT(m_writeIndex <= m_length);
Do this check before we potentially corrupt memory. If it's over, that's
really bad...right? Maybe even use CRASH() here? Otherwise you probably
should handle this case more gracefully.
> +
> + if (m_writeIndex >= m_length)
> + m_writeIndex = 0;
> +}
> +
> +float* ReverbInputBuffer::directReadFrom(int* index, size_t nframes)
> +{
> + ASSERT(*index >= 0 && *index + nframes <= m_length);
ditto
> + float* sourceP = m_buffer;
> + float* p = sourceP + *index;
> +
> + // Update index
> + *index = (*index + nframes) % m_length;
> +
> + return p;
> +}
> +
> +void ReverbInputBuffer::reset()
> +{
> + m_buffer.zero();
> + m_writeIndex = 0;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/ReverbInputBuffer.h b/WebCore/platform/audio/ReverbInputBuffer.h
> new file mode 100644
> index 0000000..0b80f42
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbInputBuffer.h
> @@ -0,0 +1,66 @@
> +/*
> + * 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 ReverbInputBuffer_h
> +#define ReverbInputBuffer_h
> +
> +#include "AudioFloatArray.h"
> +
> +namespace WebCore {
> +
> +// ReverbInputBuffer is used to buffer input samples for deferred processing by the background threads
Period. Here and elsewhere please.
> +class ReverbInputBuffer {
> +public:
> + ReverbInputBuffer(size_t length);
> +
> + // The realtime audio thread keeps writing samples here.
> + // The assumption is that the buffer's length is evenly divisible by nframes (for nearly all cases this will be fine)
> + // FIXME: remove nframes restriction...
> + void write(float* sourceP, size_t nframes);
> +
> + // Background threads can call this to check if there's anything to read...
> + // FIXME: create better system to check for buffer overruns / error conditions...
I'm not sure if this fixme is actually useful.
> + size_t writeIndex() const { return m_writeIndex; }
> +
> + // The individual background threads read here (and hope that they can keep up with the buffer writing)
> + // index is updated with the next index to read from...
> + // The assumption is that the buffer's length is evenly divisible by nframes
> + // FIXME: remove nframes restriction...
> + float* directReadFrom(int* index, size_t nframes);
> +
> + void reset();
> +
> +private:
> + AudioFloatArray m_buffer;
> + size_t m_length;
Doesn't the buffer itself know its length?
> + size_t m_writeIndex;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbInputBuffer_h
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list