Bug 56692 – Add high-quality band-limited audio resampling algorithm

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add high-quality band-limited audio resampling algorithm

[Chris Rogers]

Created attachment 86240 [details]
Patch

[Chris Rogers]

The code has been tested a fair bit by processing exponentially swept sine waves and diffing against the ideal generated result.  The code can be optimized further, but runs well enough to get us started.

Note: this code will not be used in the mac port, since CoreAudio's resampler is very good and better optimized.

[Kenneth Russell]

Comment on attachment 86240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86240&action=review

There are a bunch of signed/unsigned mismatches that need to be dealt with because I'm pretty sure they'll be flagged as errors on some platforms (I think Windows). Some assertions are also needed to ensure no out-of-bounds accesses.

> Source/WebCore/platform/audio/SincResampler.cpp:98
> +    unsigned n = m_kernelSize;
> +    int halfSize = n / 2;

Mixing of signed/unsigned; bad idea.

> Source/WebCore/platform/audio/SincResampler.cpp:102
> +    for (int offsetIndex = 0; offsetIndex <= m_numberOfKernelOffsets; ++offsetIndex) {

Mixing of signed/unsigned.

> Source/WebCore/platform/audio/SincResampler.cpp:105
> +        for (int i = 0; i < n; ++i) {

Signed/unsigned mismatch. I think you should consider just using signed ints everywhere.

> Source/WebCore/platform/audio/SincResampler.cpp:141
> +    // Setup various region pointers in the buffer (see diagram above).

Please add some assertions to prevent out-of-bounds accesses. As far as I can tell the following are requirements:
  m_blockSize >= m_kernelSize / 2
  numberOfSourceFrames >= m_blockSize + m_kernelSize / 2
  m_inputBuffer.size() >= m_blockSize + m_kernelSize
  m_kernelSize % 2 == 0

> Source/WebCore/platform/audio/SincResampler.cpp:152
> +    unsigned numberOfDestinationFrames = numberOfSourceFrames / m_scaleFactor;

Is there a warning about loss of precision because the denominator is a double?

> Source/WebCore/platform/audio/SincResampler.cpp:178
> +            float sum1 = 0.0;
> +            float sum2 = 0.0;

The .0's here are unnecessary; see "Floating point literals" in http://www.webkit.org/coding/coding-style.html .

> Source/WebCore/platform/audio/SincResampler.cpp:198
> +                // Optimize size 32 and size 64 kernels by unrolling the while loop.

Might be worth mentioning the percentage speedup you get by doing this.

> Source/WebCore/platform/audio/SincResampler.cpp:325
> +        memcpy(r1, r3, sizeof(float) * (m_kernelSize / 2) );
> +        memcpy(r2, r4, sizeof(float) * (m_kernelSize / 2) );

Extra spaces before closing paren.

[Chris Rogers]

Created attachment 86490 [details]
Patch

[Chris Rogers]

Comment on attachment 86240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86240&action=review

>> Source/WebCore/platform/audio/SincResampler.cpp:98
>> +    int halfSize = n / 2;
> 
> Mixing of signed/unsigned; bad idea.

FIXED

>> Source/WebCore/platform/audio/SincResampler.cpp:102
>> +    for (int offsetIndex = 0; offsetIndex <= m_numberOfKernelOffsets; ++offsetIndex) {
> 
> Mixing of signed/unsigned.

FIXED

>> Source/WebCore/platform/audio/SincResampler.cpp:105
>> +        for (int i = 0; i < n; ++i) {
> 
> Signed/unsigned mismatch. I think you should consider just using signed ints everywhere.

n is now int instead of unsigned

>> Source/WebCore/platform/audio/SincResampler.cpp:141
>> +    // Setup various region pointers in the buffer (see diagram above).
> 
> Please add some assertions to prevent out-of-bounds accesses. As far as I can tell the following are requirements:
>   m_blockSize >= m_kernelSize / 2
>   numberOfSourceFrames >= m_blockSize + m_kernelSize / 2
>   m_inputBuffer.size() >= m_blockSize + m_kernelSize
>   m_kernelSize % 2 == 0

Added your suggested assertions, except the one about numberOfSourceFrames.  It can be smaller than you suggest because consumeSource() will zero-pad if there aren't enough source frames.

>> Source/WebCore/platform/audio/SincResampler.cpp:152
>> +    unsigned numberOfDestinationFrames = numberOfSourceFrames / m_scaleFactor;
> 
> Is there a warning about loss of precision because the denominator is a double?

I added a static_cast<> here

>> Source/WebCore/platform/audio/SincResampler.cpp:178
>> +            float sum2 = 0.0;
> 
> The .0's here are unnecessary; see "Floating point literals" in http://www.webkit.org/coding/coding-style.html .

FIXED

>> Source/WebCore/platform/audio/SincResampler.cpp:198
>> +                // Optimize size 32 and size 64 kernels by unrolling the while loop.
> 
> Might be worth mentioning the percentage speedup you get by doing this.

DONE

>> Source/WebCore/platform/audio/SincResampler.cpp:325
>> +        memcpy(r2, r4, sizeof(float) * (m_kernelSize / 2) );
> 
> Extra spaces before closing paren.

FIXED

[Kenneth Russell]

Comment on attachment 86490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86490&action=review

Looks better, thanks. Couple of additional minor issues you can fix upon landing.

> Source/WebCore/platform/audio/SincResampler.cpp:165
> +    while (numberOfDestinationFrames > 0) {

numberOfDestinationFrames is unsigned, so this test is not well written. Would be better as "while (numberOfDestinationFrames)".

> Source/WebCore/platform/audio/SincResampler.cpp:316
> +            numberOfDestinationFrames--;

In WebKit, prefer predecrement (--numberOfDestinationFrames).

[Chris Rogers]

Committed r81702: <http://trac.webkit.org/changeset/81702>