Bug 34907

Summary: audio engine: add FFTConvolver class
Product: WebKit Reporter: Chris Rogers <crogers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, dino, eric.carlson, eric, jorlow, kbr, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 34452    
Bug Blocks:    
Attachments:
Description Flags
diagram describing operation of FFTConvolver
none
Patch
none
Patch
none
Patch none

Description Chris Rogers 2010-02-12 15:01:39 PST
audio engine: add FFTConvolver class
Comment 1 Chris Rogers 2010-02-12 15:03:42 PST
Created attachment 48667 [details]
diagram describing operation of FFTConvolver
Comment 2 Chris Rogers 2010-02-12 15:05:16 PST
The FFTConvolver is able to do short convolutions with the FFT size N being at least twice as large as the length of the short impulse response. It incurs a latency of N/2 sample-frames. Because of this latency and performance considerations, it is not suitable for long convolutions. Multiple instances of this building block can be used to perform extremely long convolutions.
Comment 3 Chris Rogers 2010-02-12 15:07:20 PST
Created attachment 48668 [details]
Patch
Comment 4 Jeremy Orlow 2010-03-12 04:29:33 PST
Comment on attachment 48668 [details]
Patch

Looking pretty good.


> diff --git a/WebCore/platform/audio/FFTConvolver.cpp b/WebCore/platform/audio/FFTConvolver.cpp
> new file mode 100644
> index 0000000..3c06dcc
> --- /dev/null
> +++ b/WebCore/platform/audio/FFTConvolver.cpp
> @@ -0,0 +1,94 @@
> +/*
> + * 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 "FFTConvolver.h"
> +
> +namespace WebCore {
> +
> +FFTConvolver::FFTConvolver(size_t fftSize)
> +    : m_frame(fftSize)
> +    , m_readWriteIndex(0)
> +    , m_inputBuffer(fftSize) // 2nd half of buffer is always zeroed
> +    , m_outputBuffer(fftSize)
> +    , m_lastOverlapBuffer(fftSize / 2)
> +{
> +}
> +
> +void FFTConvolver::process(FFTFrame* fftKernel,
> +                           float* sourceP,
> +                           float* destP,
> +                           size_t framesToProcess)
> +{
> +    // FIXME : make so |framesToProcess| is not required to fit evenly into fftSize/2

No space between FIXME and :.

> +
> +    // Copy samples to input buffer (note contraint above!)

Can we ASSERT anything there?

> +    float* inputP = m_inputBuffer;
> +    memcpy(inputP + m_readWriteIndex, sourceP, sizeof(float) * framesToProcess);
> +
> +    // Copy samples from output buffer
> +    float* outputP = m_outputBuffer;
> +    memcpy(destP, outputP + m_readWriteIndex, sizeof(float) * framesToProcess);
> +
> +    m_readWriteIndex += framesToProcess;
> +
> +    size_t halfSize = fftSize() / 2;
> +
> +    // Check if it's time to perform the next FFT
> +    if (m_readWriteIndex == halfSize) {
> +        // The input buffer is now filled (get frequency-domain version)
> +        m_frame.doFFT(m_inputBuffer);
> +        m_frame.multiply(*fftKernel);
> +        m_frame.doInverseFFT(m_outputBuffer);
> +
> +        // Overlap-add 1st half from previous time
> +        vadd(m_outputBuffer,
> +             1,
> +             m_lastOverlapBuffer,
> +             1,
> +             m_outputBuffer,
> +             1,
> +             halfSize);

The WebKit standard is to put the whole function call on one line.  I don't see much of a readability improvement by splitting this over multiple lines, so I think it'd probably be best if you just did that.

> +
> +        // Finally, save 2nd half of result
> +        memcpy((float*)m_lastOverlapBuffer,
> +               (float*)m_outputBuffer + halfSize,
> +               sizeof(float) * halfSize);

Ditto.

> +
> +        // Reset index back to start for next time
> +        m_readWriteIndex = 0;
> +    }
> +}
> +
> +void FFTConvolver::reset()
> +{
> +    m_lastOverlapBuffer.zero();
> +    m_readWriteIndex = 0;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/FFTConvolver.h b/WebCore/platform/audio/FFTConvolver.h
> new file mode 100644
> index 0000000..20a4873
> --- /dev/null
> +++ b/WebCore/platform/audio/FFTConvolver.h
> @@ -0,0 +1,74 @@
> +/*
> + * 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 FFTConvolver_h
> +#define FFTConvolver_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTFrame.h"
> +
> +namespace WebCore {
> +
> +class FFTConvolver {
> +public:
> +    // |fftSize| must be a power of two

I think the fact that you're talking about the fftSize variable is clear without the ||'s and they're not really used elsewhere in the code base.  Same goes for other uses in comments.

> +    FFTConvolver(size_t fftSize);
> +
> +    // For now, with multiple calls to Process(), |framesToProcess| MUST add up EXACTLY to |fftSize| / 2
> +    //
> +    // FIXME: Later, we can do more sophisticated buffering to relax this requirement...
> +    //
> +    // The input to output latency is equal to |fftSize| / 2
> +    //
> +    // Processing in-place is allowed...
> +    void process(FFTFrame* fftKernel,
> +                 float* sourceP,
> +                 float* destP,
> +                 size_t framesToProcess);
> +
> +    void reset();
> +
> +    size_t fftSize() const { return m_frame.fftSize(); }
> +
> +private:
> +    FFTFrame m_frame;
> +
> +    // Buffer input until we get fftSize / 2 samples then do an FFT
> +    size_t m_readWriteIndex;
> +    AudioFloatArray m_inputBuffer;
> +
> +    // Stores output which we read a little at a time
> +    AudioFloatArray m_outputBuffer;
> +
> +    // Saves the 2nd half of the FFT buffer, so we can do an overlap-add with the 1st half of the next one
> +    AudioFloatArray m_lastOverlapBuffer;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // FFTConvolver_h
Comment 5 Chris Rogers 2010-03-16 16:24:33 PDT
Created attachment 50849 [details]
Patch
Comment 6 Chris Rogers 2010-03-16 16:25:36 PDT
Hi Jeremy, I think I've taken care of everything - thanks...
Comment 7 Jeremy Orlow 2010-03-17 08:50:15 PDT
Comment on attachment 50849 [details]
Patch

> diff --git a/WebCore/platform/audio/FFTConvolver.cpp b/WebCore/platform/audio/FFTConvolver.cpp
> new file mode 100644
> index 0000000..16a0f48
> --- /dev/null
> +++ b/WebCore/platform/audio/FFTConvolver.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 "FFTConvolver.h"
> +
> +namespace WebCore {
> +
> +FFTConvolver::FFTConvolver(size_t fftSize)
> +    : m_frame(fftSize)
> +    , m_readWriteIndex(0)
> +    , m_inputBuffer(fftSize) // 2nd half of buffer is always zeroed
> +    , m_outputBuffer(fftSize)
> +    , m_lastOverlapBuffer(fftSize / 2)
> +{
> +}
> +
> +void FFTConvolver::process(FFTFrame* fftKernel, float* sourceP, float* destP, size_t framesToProcess)
> +{
> +    // FIXME: make so |framesToProcess| is not required to fit evenly into fftSize/2

Get rid of |'s per earlier comment.


Looks good, but we'll probably want to land this in the branch rather than trunk...we'll know for sure soon, hopefully.
Comment 8 Eric Seidel (no email) 2010-03-24 20:09:19 PDT
Chris isn't a committer.  So it seems that the cq- is effectively the same as an r-.
Comment 9 Jeremy Orlow 2010-03-25 03:24:09 PDT
(In reply to comment #8)
> Chris isn't a committer.  So it seems that the cq- is effectively the same as
> an r-.

He'll be a committer in a couple of days (pending paper work).  Once that's done, he'll land this on a branch.
Comment 10 Eric Seidel (no email) 2010-04-21 18:22:18 PDT
Any update?  Should this still be r+ in the pending-commit list?
Comment 11 Jeremy Orlow 2010-04-21 18:26:57 PDT
Chris is on vacation.
Comment 12 Chris Rogers 2010-04-23 11:32:25 PDT
(In reply to comment #11)
> Chris is on vacation.

I'm back now - I'll soon be checking a bunch of stuff into a branch.  Then we can figure out how to land this and some of the other patches into trunk...
Comment 13 Eric Seidel (no email) 2010-05-02 19:14:44 PDT
Was this landed in the big audio branch landing?  If so, please close so it's not in webkit.org/pending-commit anymore.
Comment 14 Chris Rogers 2010-05-03 12:15:24 PDT
(In reply to comment #13)
> Was this landed in the big audio branch landing?  If so, please close so it's
> not in webkit.org/pending-commit anymore.

This still needs to land in trunk.
Comment 15 Adam Barth 2010-05-14 23:49:11 PDT
> This still needs to land in trunk.

Two weeks have gone by.  Would you like this landed in trunk?  If so, either land it (if you are a committer) or mark it commit-queue? so a committer knows that this is available to be committed.  We don't want patches to live in pending-commit forever.
Comment 16 Chris Rogers 2010-05-17 12:10:39 PDT
Adam, I'm sorry if this hasn't been clear.  This and the other patches all need to land in trunk eventually.  But the code currently lives in a private branch where I'm working so we can determine the proper directory layout, etc. as was discussed in the webkit-dev thread.  This was Maciej's suggestion and it's working out very well for me.  I'm sorry if the r+ patches sitting around have been a problem but I have to get my patches reviewed as the audio engine contains a lot of code.  But it can't be landed until we settle on the directory layout.  In the meantime I'll remove the r+
Comment 17 Chris Rogers 2010-05-17 12:11:41 PDT
Comment on attachment 50849 [details]
Patch

temporarily changing back to r- until we can settle on proper directory layout
Comment 18 Chris Rogers 2010-08-26 13:38:08 PDT
Created attachment 65611 [details]
Patch
Comment 19 Chris Rogers 2010-08-26 13:43:09 PDT
Jeremy Orlow has already R+ the previous patch.  This new patch just introduces the WEB_AUDIO feature enable.
Comment 20 Kenneth Russell 2010-08-30 13:56:59 PDT
Comment on attachment 65611 [details]
Patch

I'm r+'ing this based on earlier review but want to highlight a couple of portability issues.

> Index: WebCore/platform/audio/FFTConvolver.cpp
> ===================================================================
> --- WebCore/platform/audio/FFTConvolver.cpp	(revision 0)
> +++ WebCore/platform/audio/FFTConvolver.cpp	(revision 0)
> @@ -0,0 +1,108 @@
> +/*
> + * 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 "FFTConvolver.h"
> +
> +#include "Accelerate.h"

This #include should be surrounded by some PLATFORM or OS macro for Mac OS X, and a compile-time or run-time failure on other platforms.

> +
> +namespace WebCore {
> +
> +FFTConvolver::FFTConvolver(size_t fftSize)
> +    : m_frame(fftSize)
> +    , m_readWriteIndex(0)
> +    , m_inputBuffer(fftSize) // 2nd half of buffer is always zeroed
> +    , m_outputBuffer(fftSize)
> +    , m_lastOverlapBuffer(fftSize / 2)
> +{
> +}
> +
> +void FFTConvolver::process(FFTFrame* fftKernel, float* sourceP, float* destP, size_t framesToProcess)
> +{
> +    // FIXME: make so framesToProcess is not required to fit evenly into fftSize/2
> +
> +    // Copy samples to input buffer (note contraint above!)
> +    float* inputP = m_inputBuffer.data();
> +
> +    // Sanity check
> +    bool isCopyGood1 = sourceP && inputP && m_readWriteIndex + framesToProcess <= m_inputBuffer.size();
> +    ASSERT(isCopyGood1);
> +    if (!isCopyGood1)
> +        return;
> +    
> +    memcpy(inputP + m_readWriteIndex, sourceP, sizeof(float) * framesToProcess);
> +
> +    // Copy samples from output buffer
> +    float* outputP = m_outputBuffer.data();
> +
> +    // Sanity check
> +    bool isCopyGood2 = destP && outputP && m_readWriteIndex + framesToProcess <= m_outputBuffer.size();
> +    ASSERT(isCopyGood2);
> +    if (!isCopyGood2)
> +        return;
> +
> +    memcpy(destP, outputP + m_readWriteIndex, sizeof(float) * framesToProcess);
> +    m_readWriteIndex += framesToProcess;
> +
> +
> +    // Check if it's time to perform the next FFT
> +    size_t halfSize = fftSize() / 2;
> +    if (m_readWriteIndex == halfSize) {
> +        // The input buffer is now filled (get frequency-domain version)
> +        m_frame.doFFT(m_inputBuffer.data());
> +        m_frame.multiply(*fftKernel);
> +        m_frame.doInverseFFT(m_outputBuffer.data());
> +
> +        // Overlap-add 1st half from previous time
> +        vadd(m_outputBuffer.data(), 1, m_lastOverlapBuffer.data(), 1, m_outputBuffer.data(), 1, halfSize);

This should be similarly covered by a Mac-specific PLATFORM or OS macro.

> +
> +        // Finally, save 2nd half of result
> +        bool isCopyGood3 = m_outputBuffer.size() == 2 * halfSize && m_lastOverlapBuffer.size() == halfSize;
> +        ASSERT(isCopyGood3);
> +        if (!isCopyGood3)
> +            return;
> +        
> +        memcpy(m_lastOverlapBuffer.data(), m_outputBuffer.data() + halfSize, sizeof(float) * halfSize);
> +
> +        // Reset index back to start for next time
> +        m_readWriteIndex = 0;
> +    }
> +}
> +
> +void FFTConvolver::reset()
> +{
> +    m_lastOverlapBuffer.zero();
> +    m_readWriteIndex = 0;
> +}
> +
> +} // namespace WebCore
> +
> +#endif // ENABLE(WEB_AUDIO)
Comment 21 Chris Rogers 2010-08-30 14:09:02 PDT
Hi Ken, thanks for the review.  The include of "Accelerate.h" *is* cross-platform and is the abstraction layer for vector operations (like vadd()) so it doesn't have a platform-specific #if
Comment 22 Kenneth Russell 2010-08-30 16:22:00 PDT
(In reply to comment #21)
> Hi Ken, thanks for the review.  The include of "Accelerate.h" *is* cross-platform and is the abstraction layer for vector operations (like vadd()) so it doesn't have a platform-specific #if

OK. I didn't see the creation of Accelerate.h until looking at https://bugs.webkit.org/show_bug.cgi?id=34452 .
Comment 23 Chris Rogers 2010-08-31 14:02:25 PDT
Comment on attachment 65611 [details]
Patch

Clearing flags on attachment: 65611

Committed r66529: <http://trac.webkit.org/changeset/66529>
Comment 24 Chris Rogers 2010-08-31 14:02:34 PDT
All reviewed patches have been landed.  Closing bug.