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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Chris Rogers <crogers at google.com>  2010-09-17 12:47:33 PST ---
James, thanks for the review.  I've addressed all of your comments except for the one about the post-processing stage (see comment below).

(In reply to comment #7)
> (From update of attachment 67854 [details])
> Looking pretty close.  Left a few comments.
> 
> > 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

FIXED

> 
> > WebCore/platform/audio/HRTFKernel.cpp:58
> > +    *delayFrames = frameDelay;
> 
> Instead of using an out param, could this function just return the delay?

FIXED


> > 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.

I agree with you on this, but I've left this as a FIXME.  It's non-trivial to re-generate the data-sets, requiring very careful verification and testing.  Later on in refining the data-sets I'm planning on reducing their size from their current > 1Mb to 500Kb or less.  At this stage, I plan on baking in this post-processing and this code can be removed.

In the meantime, although keeping the post-processing stage in the code at load time is not ideal, it is not very expensive at all (the Biquad processors are insanely fast).


> > 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.

You're quite right.  I broke this when I made yesterday's change to numberOfFadeOutFrames.   FIXED


> > 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

FIXED


> > WebCore/platform/audio/HRTFKernel.h:65
> > +    const FFTFrame* fftFrame() const { return m_fftFrame.get(); }
> 
> Remove this getter, I doubt it's needed

FIXED


> > 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.

FIXED


> > WebCore/platform/audio/HRTFKernel.h:90
> > +    HRTFKernel()
> > +        : m_fftFrame(0)
> > +        , m_frameDelay(0.0)
> > +    {
> > +    }
> 
> This constructor looks unused, if so please remove it.

FIXED

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