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

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from James Robinson <jamesr at chromium.org>  2010-09-24 15:52:01 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 int HRTFDatabase::MinElevation = -45.0;
> +const int HRTFDatabase::MaxElevation = 90.0;
> +const unsigned HRTFDatabase::RawElevationAngleSpacing = 15.0;

nit: int value shouldn't have .0

> WebCore/platform/audio/HRTFDatabase.cpp:69
> +    for (int elevation = -45; elevation <= 90; elevation += 15) {

use MinElevation/MaxElevation/RawElevationAngleSpacing here instead of using magic numbers

> WebCore/platform/audio/HRTFDatabase.cpp:80
> +    // Now, go back and interpolate elevations.
> +    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.

> WebCore/platform/audio/HRTFDatabase.h:52
> +    static void cleanup();

This needs some documentation or a better name.  What does it clean up?

> 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.

> WebCore/platform/audio/HRTFDatabase.h:79
> +    // Minimum and maximum elevation angles (inclusive) for a HRTFDatabase.
> +    static const int MinElevation;
> +    static const int MaxElevation;
> +    static const unsigned RawElevationAngleSpacing;
> +    
> +    // Number of elevations loaded from resource.
> +    static const unsigned NumberOfRawElevations;
> +
> +    // Interpolates by this factor to get the total number of elevations from every elevation loaded from resource.
> +    static const unsigned InterpolationFactor;
> +    
> +    // Total number of elevations after interpolation.
> +    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.

> WebCore/platform/audio/HRTFDatabase.h:97
> +    // Keeps track of HRTFDatabases for different human subjects.
> +    typedef HashMap<String, RefPtr<HRTFDatabase> > HRTFSubjectMap;
> +    static HRTFSubjectMap* s_subjectMapP;
> +
> +    static HRTFSubjectMap& subjectMap()
> +    {
> +        // Lazily initialize subject map.
> +        if (!s_subjectMapP)
> +            s_subjectMapP = new HRTFSubjectMap();
> +        return *s_subjectMapP;
> +    }

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.

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