Bug 45863 – Add HRTFKernel files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add HRTFKernel files

[Chris Rogers]

Created attachment 67758 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 67758 [details]
Patch

You should say what HRTF stands for somewhere.

[James Robinson]

Comment on attachment 67758 [details]
Patch

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

> WebCore/platform/audio/HRTFKernel.cpp:73
> +    size_t truncatedResponseLength = std::min(responseLength, fftSize / 2); // truncate if necessary to max impulse response length allowed by FFT

WebKit style is to add a 'using namespace std;' declaration and then call undecorated min().

> WebCore/platform/audio/HRTFKernel.cpp:76
> +    float numberOfFadeOutFrames = sampleRate / 4410; // 10 sample-frames @44.1KHz sample-rate

This is truncating from double->float, do you mean to?  If so please use a static_cast<> to make it explicit for readers and the compiler.

> WebCore/platform/audio/HRTFKernel.h:50
> +    static PassRefPtr<HRTFKernel> create(float* impulseResponse, size_t responseLength, size_t fftSize, double sampleRate, bool bassBoost)

Can you use something other than a float*/size_t pair?  It looks like we might be able to use an AudioChannel here.  Passing raw pointer/length pairs around is error prone.

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

WebKit code typically implements functions like fftSize() as just return m_fftFrame->fftSize() instead of providing a const FFTFrame* const getter.

> WebCore/platform/audio/HRTFKernel.h:71
> +    // Converts back into impulse-response form.
> +    // Response must point to storage of size fftSize().
> +    void generateImpulseResponse(float* response);

This seems fragile as well.  What will the callers of this look like?  Is there any way way to do the logic associated with this in a way that doesn't require passing raw ptrs around?

[Chris Rogers]

Created attachment 67845 [details]
Patch

[Chris Rogers]

Created attachment 67854 [details]
Patch

[Chris Rogers]

I addressed Simon Fraser's comment about HRTF by adding a comment in the header file.

James, I think I've addressed your comments:

> WebCore/platform/audio/HRTFKernel.cpp:73
> +    size_t truncatedResponseLength = std::min(responseLength, fftSize / 2); // truncate if necessary to max impulse response length allowed by FFT

WebKit style is to add a 'using namespace std;' declaration and then call undecorated min().

FIXED



> WebCore/platform/audio/HRTFKernel.cpp:76
> +    float numberOfFadeOutFrames = sampleRate / 4410; // 10 sample-frames @44.1KHz sample-rate

This is truncating from double->float, do you mean to?  If so please use a static_cast<> to make it explicit for readers and the compiler.

ADDED static_cast<unsigned> since it needs to be quantized to an integer number of sample-frames to do the fade-out.




> WebCore/platform/audio/HRTFKernel.h:50
> +    static PassRefPtr<HRTFKernel> create(float* impulseResponse, size_t responseLength, size_t fftSize, double sampleRate, bool bassBoost)

Can you use something other than a float*/size_t pair?  It looks like we might be able to use an AudioChannel here.  Passing raw pointer/length pairs around is error prone.


FIXED: changed to take AudioChannel* argument



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

WebKit code typically implements functions like fftSize() as just return m_fftFrame->fftSize() instead of providing a const FFTFrame* const getter.

FIXED




> WebCore/platform/audio/HRTFKernel.h:71
> +    // Converts back into impulse-response form.
> +    // Response must point to storage of size fftSize().
> +    void generateImpulseResponse(float* response);

This seems fragile as well.  What will the callers of this look like?  Is there any way way to do the logic associated with this in a way that doesn't require passing raw ptrs around?


Yes, you're right.  A better way is to return a PassOwnPtr<AudioChannel> -- changing to createImpulseResponse().
I wrote this original code a long time ago before I was as familiar with the OwnPtr and create() style and didn't notice this.

[James Robinson]

Comment on attachment 67854 [details]
Patch

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.

[Chris Rogers]

Created attachment 67936 [details]
Patch

[Chris Rogers]

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

[James Robinson]

Comment on attachment 67936 [details]
Patch

Looks good! Just some nits, feel free to address while landing or post a new patch.

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

> WebCore/platform/audio/HRTFKernel.cpp:124
> +    bool checkX = x >= 0.0 && x < 1.0;
> +    ASSERT(checkX);

nit: better to put the predicate directly into the ASSERT()

> WebCore/platform/audio/HRTFKernel.cpp:126
> +    x = max(0.0, x);
> +    x = min(1.0, x);

nit: could be one expression

> WebCore/platform/audio/HRTFKernel.h:67
> +    

nit: extra newline

[Chris Rogers]

Created attachment 68293 [details]
Patch

[Chris Rogers]

This was previously R+ by James Robinson.  This last patch just addresses his last few fixup comments.

[James Robinson]

Comment on attachment 68293 [details]
Patch

Still looks good!

[WebKit Commit Bot]

Comment on attachment 68293 [details]
Patch

Clearing flags on attachment: 68293

Committed r68025: <http://trac.webkit.org/changeset/68025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.