[Webkit-unassigned] [Bug 45863] Add HRTFKernel files

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from James Robinson <jamesr at chromium.org>  2010-09-16 19:36:11 PST ---
(From update of attachment 67854)
Looking pretty close.  Left a few comments.

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

> WebCore/platform/audio/HRTFKernel.cpp:47
> +static void extractAverageGroupDelay(float* impulseP, size_t length, double* delayFrames)

It appears this always operates on an AudioChannel, could it take one of those instead of float*/size_t?  Then I believe can the comment about the length requirement and associated ASSERT

> WebCore/platform/audio/HRTFKernel.cpp:58
> +    *delayFrames = frameDelay;

Instead of using an out param, could this function just return the delay?

> WebCore/platform/audio/HRTFKernel.cpp:77
> +    if (bassBoost) {
> +        // Run through some post-processing to boost the bass a little -- the HRTF's seem to be a little bass-deficient.
> +        Biquad filter;
> +        filter.setLowShelfParams(700.0 / nyquist(), 6.0); // boost 6dB at 700Hz
> +        filter.process(impulseResponse, impulseResponse, responseLength);
> +    }

Do we always want to do this?  If it's a property of the HRTF data, what about doing the processing on the source data instead of doing this at runtime?

This doesn't seem like something the caller will want to change dynamically at runtime, so it shouldn't be a parameter.  WebAudio users could set up a filter node to do this boost (or any other sort of effect) if they want.

> WebCore/platform/audio/HRTFKernel.cpp:87
> +            float x = 1.0f - (i - (truncatedResponseLength - numberOfFadeOutFrames)) / numberOfFadeOutFrames;

I'm not positive, but I think this division will be an integer division and x will end up always being 1.0.

> WebCore/platform/audio/HRTFKernel.h:53
> +    // Note: this is destructive on the passed in impulseResponse array.
> +    // responseLength must be a power of two.

this comment needs updating

> WebCore/platform/audio/HRTFKernel.h:65
> +    const FFTFrame* fftFrame() const { return m_fftFrame.get(); }

Remove this getter, I doubt it's needed

> WebCore/platform/audio/HRTFKernel.h:83
> +#ifndef NDEBUG
> +    // For debugging
> +    void print()
> +    {
> +        printf("frameDelay = %f\n", m_frameDelay);
> +        m_fftFrame->print();
> +    }
> +#endif

Is this still needed?  Remove if not, otherwise at least move it out of the class definition.

> WebCore/platform/audio/HRTFKernel.h:90
> +    HRTFKernel()
> +        : m_fftFrame(0)
> +        , m_frameDelay(0.0)
> +    {
> +    }

This constructor looks unused, if so please remove it.

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