[webkit-reviews] review denied: [Bug 46299] Add HRTFPanner files : [Attachment 69117] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 46299: Add HRTFPanner files
https://bugs.webkit.org/show_bug.cgi?id=46299

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69117&action=review

Nearly there, few more small things.  Where is DelayDSPKernel defined? I don't
see it in my tree.

> WebCore/platform/audio/HRTFPanner.cpp:117
> +    // IRCAM HRTF azimuths which we're loading are reversed from the
panner's notion...

Still not technically a sentence.  If this is normalization for the way our
data files are formatted, why is it being done here?

> WebCore/platform/audio/HRTFPanner.cpp:121
> +    if (azimuth < 0)
> +	   azimuth += 360.0; // make positive

Not a sentence.  Why is this needed?

> WebCore/platform/audio/HRTFPanner.cpp:141
> +    bool isAzimuthGood = azimuth >= 0.0 && azimuth <= 360.0;
> +    ASSERT(isAzimuthGood);
> +    if (!isAzimuthGood) {
> +	   outputBus->zero();
> +	   return;
> +    }
> +
> +    int numberOfAzimuths = database->numberOfAzimuths();
> +    const double angleBetweenAzimuths = 360.0 / numberOfAzimuths;
> +
> +    // Calculate the azimuth index and the blend (0 -> 1) for interpolation.

> +    double desiredAzimuthIndexF = azimuth / angleBetweenAzimuths;
> +    int desiredAzimuthIndex = static_cast<int>(desiredAzimuthIndexF);
> +    double azimuthBlend = desiredAzimuthIndexF -
static_cast<double>(desiredAzimuthIndex);
> +    
> +    // We don't immediately start using this azimuth index, but instead
approach this index from the last index we rendered at.
> +    // This minimizes the clicks and graininess for moving sources which
occur otherwise.
> +    desiredAzimuthIndex = max(0, desiredAzimuthIndex);
> +    desiredAzimuthIndex = min(numberOfAzimuths - 1, desiredAzimuthIndex);

We don't typically use suffixes like F for floating point values in WebKit. 
This segment feels like it should be in a helper function (since this function
is a bit on the long side).  Are the min/max needed given that we've already
checked that azimuth is in the range [0.0,360.0] ?

> WebCore/platform/audio/HRTFPanner.cpp:174
> +	   // Get the HRTFKernels and interpolated delays.
> +    

nit: don't need the extra newline here

> WebCore/platform/audio/HRTFPanner.cpp:198
> +	   // Normally, we'll just be dealing with mono sources.
> +	   // If we have a stereo input, implement a strange kind of panning
with left source processed by left HRTF, and right source by right HRTF.
> +	   AudioChannel* channelL =
inputBus->channelByType(AudioBus::ChannelLeft);
> +	   AudioChannel* channelR = numInputChannels > 1 ?
inputBus->channelByType(AudioBus::ChannelRight) : 0;
> +
> +	   // Get source and destination pointers.
> +	   float* sourceL = channelL->data();
> +	   float* sourceR = numInputChannels > 1 ? channelR->data() : sourceL;
> +	   float* destinationL =
outputBus->channelByType(AudioBus::ChannelLeft)->data();
> +	   float* destinationR =
outputBus->channelByType(AudioBus::ChannelRight)->data();

Should some of this be outside the loop?

> WebCore/platform/audio/HRTFPanner.h:32
> +#include "DelayProcessor.h"

I don't see this header in my checkout.  Does it define DelayDSPKernel?  Should
you include DelayDSPKernel.h directly (once it exists)?

> WebCore/platform/audio/HRTFPanner.h:39
> +class FFTFrame;
> +class HRTFDatabase;

This forward declarations aren't needed.