[webkit-reviews] review denied: [Bug 56692] Add high-quality band-limited audio resampling algorithm : [Attachment 86240] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 56692: Add high-quality band-limited audio resampling algorithm
https://bugs.webkit.org/show_bug.cgi?id=56692

Attachment 86240: Patch
https://bugs.webkit.org/attachment.cgi?id=86240&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.