[Webkit-unassigned] [Bug 56692] Add high-quality band-limited audio resampling algorithm

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56692


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86240|review?                     |review-
               Flag|                            |




--- Comment #3 from Kenneth Russell <kbr at google.com>  2011-03-21 15:13:51 PST ---
(From update of attachment 86240)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.