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

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Chris Rogers <crogers at google.com>  2010-09-28 17:58:55 PST ---
(From update of attachment 69117)
View in context: https://bugs.webkit.org/attachment.cgi?id=69117&action=review

>> WebCore/platform/audio/HRTFPanner.cpp:117
>> +    // IRCAM HRTF azimuths which we're loading are reversed from the panner's notion...
> 
> Still not technically a sentence.  If this is normalization for the way our data files are formatted, why is it being done here?

I can suggest two ways to attack this problem and I'll leave it up to you:
1) improve the comment with a FIXME to re-bake the HRTF data files with the azimuths not reversed.  We have to re-bake the files anyway for the file-size reduction, bass boost, and maybe other things.
2) remove this code from here, but do the reversal in HRTFElevation and add a similar comment as proposed in (1) there.

>> WebCore/platform/audio/HRTFPanner.cpp:121
>> +        azimuth += 360.0; // make positive
> 
> Not a sentence.  Why is this needed?

I improved the comment.

>> WebCore/platform/audio/HRTFPanner.cpp:141
>> +    desiredAzimuthIndex = min(numberOfAzimuths - 1, desiredAzimuthIndex);
> 
> We don't typically use suffixes like F for floating point values in WebKit.  This segment feels like it should be in a helper function (since this function is a bit on the long side).  Are the min/max needed given that we've already checked that azimuth is in the range [0.0,360.0] ?

I created a new method called calculateDesiredAzimuthIndexAndBlend().
I renamed desiredAzimuthIndexF to desiredAzimuthIndexFloat
I'd rather still do the min/max check since this is now moved into a separate method and is not inefficient.

>> WebCore/platform/audio/HRTFPanner.cpp:174
>> +    
> 
> nit: don't need the extra newline here

FIXED

>> WebCore/platform/audio/HRTFPanner.cpp:198
>> +        float* destinationR = outputBus->channelByType(AudioBus::ChannelRight)->data();
> 
> Should some of this be outside the loop?

FIXED

>> WebCore/platform/audio/HRTFPanner.h:32
>> +#include "DelayProcessor.h"
> 
> I don't see this header in my checkout.  Does it define DelayDSPKernel?  Should you include DelayDSPKernel.h directly (once it exists)?

I changed the code to include the header file directly.
It is not yet landed in trunk, but you can view the interface here:
https://svn.webkit.org/repository/webkit/branches/audio/WebCore/webaudio/DelayDSPKernel.h
The DelayDSPKernel implementation is not yet ready for code review, but the interface used by HRTFPanner is simple.

>> WebCore/platform/audio/HRTFPanner.h:39
>> +class HRTFDatabase;
> 
> This forward declarations aren't needed.

FIXED

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