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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Chris Rogers <crogers at google.com>  2010-09-16 16:04:33 PST ---
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.

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