[Webkit-unassigned] [Bug 46299] Add HRTFPanner files

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

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




--- Comment #10 from James Robinson <jamesr at chromium.org>  2010-09-28 16:15:22 PST ---
(From update of attachment 69117)
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.

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