[Webkit-unassigned] [Bug 46233] Add HRTFDatabase files

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Chris Rogers <crogers at google.com>  2010-09-24 17:53:10 PST ---
(From update of attachment 68314)
View in context: https://bugs.webkit.org/attachment.cgi?id=68314&action=review

>> WebCore/platform/audio/HRTFDatabase.cpp:43
>> +const unsigned HRTFDatabase::RawElevationAngleSpacing = 15.0;
> 
> nit: int value shouldn't have .0

FIXED

>> WebCore/platform/audio/HRTFDatabase.cpp:69
>> +    for (int elevation = -45; elevation <= 90; elevation += 15) {
> 
> use MinElevation/MaxElevation/RawElevationAngleSpacing here instead of using magic numbers

FIXED

>> WebCore/platform/audio/HRTFDatabase.cpp:80
>> +    if (InterpolationFactor > 1) {
> 
> This branch seems dead (InterpolationFactor is const and set to 1).  Is InterpolationFactor intended to be configurable in the future?
> 
> Remove the code for now if it really is unreachable.

I believe there's a good chance I will use this path in the future.  If you feel strongly about removing this path for now, then I should replace it with:
ASSERT(InterpolationFactor == 1);

>> WebCore/platform/audio/HRTFDatabase.h:52
>> +    static void cleanup();
> 
> This needs some documentation or a better name.  What does it clean up?

Added comment:

    // De-references all HRTFDatabase objects held in the subject map.

>> WebCore/platform/audio/HRTFDatabase.h:65
>> +    static unsigned indexFromElevationAngle(double);
> 
> I think it make more sense to let callers to getKernelsFromAzimuthElevation pass in elevation as a double and have getKernels..() map from elevation to index internally.

I've fixed this.  The reason I had it the other way was because I wanted the method getKernelsFromAzimuthElevation() to be symmetric in its handling of azimuth and elevation, and I anticipated that I might interpolate between elevation indices in HRTFPanner.  But I'm not sure that this will be very likely.  In the meantime, I agree that you're way is cleaner to changed.

>> WebCore/platform/audio/HRTFDatabase.h:79
>> +    static const unsigned NumberOfTotalElevations;
> 
> What's a "raw" elevation and how is it different from a normal elevation?
> 
> Do all of these have to be exposed publicly?  It seems that InterpolationFactor and NumberOfTotalElevations at least are implementation details of this class alone.

I tried to document raw and normal in the comments I have there.
I've moved all of these constants into the private section.

>> WebCore/platform/audio/HRTFDatabase.h:97
>> +    }
> 
> This seems a little fragile.  Is there any reason we want to keep the HRTFDatabase objects alive for the lifetime of the process instead of having it owned by something?  The HRTFDatabase class is RefCounted so presumably the caller could hold the object alive as long as it wanted.

It may be overkill to actually land this code right now with the "subject map" since it will only every use a single HRTFDatabase "Composite".  I can remove it and have a create() method instead of createForSubject().
But, otherwise you bring up the point about the lifetime of the HRTFDatabase.  Because it takes awhile to compute, I currently have it lazily initialized when its first needed.  Then it hangs around until the process goes away so it doesn't need to be recomputed everytime a new page needs to use the audio features.  This speeds up subsequent loading of audio pages at the expense of some extra memory cost.  We should talk about the relative merits of increased speed vs. extra memory.  I should measure both the time cost and the memory cost...

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