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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Chris Rogers <crogers at google.com>  2010-09-28 15:20:51 PST ---
(From update of attachment 69002)
View in context: https://bugs.webkit.org/attachment.cgi?id=69002&action=review

>> WebCore/platform/audio/HRTFPanner.cpp:47
>> +const double HRTFPanner::MaxDelayTime = 0.012; // 12ms
> 
> why 12?
> 
> add the unit to the name - looks like it's seconds here so perhaps MaxDelayTimeSeconds

FIXED: I actually measured the largest delay time in all the HRTFKernels and picked a good round value higher than this.  I added a comment here.

>> WebCore/platform/audio/HRTFPanner.cpp:170
>> +        // 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.
> 
> Should it be an error at a higher level to connect a stereo input to this sort of node?  That is an odd sounding pan.

No, I don't think so.  Although it is more unusual than panning a mono source, it is expected to be able to pan a stereo source to stereo as well, and it results in a reasonable effect.

>> WebCore/platform/audio/HRTFPanner.h:43
>> +    HRTFPanner(double sampleRate)
> 
> one-arg constructor needs to be explicit
> 
> could the constructor definition be moved to the .cpp? I'm a little concerned about inlining the c'tor for FFTConvolver and DelayDSPKernel without know how big they are
> 
> please declare a virtual d'tor and define it in the .cpp

FIXED: I don't think the virtual destructor is necessary since it's empty and will be implicitly generated, but I added it anyway.

>> WebCore/platform/audio/HRTFPanner.h:71
>> +    int m_azimuthIndex;
> 
> What's de-zippering?
> 
> Comments should be sentences

FIXED

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