Bug 46233 – Add HRTFDatabase files

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add HRTFDatabase files

[Chris Rogers]

Created attachment 68314 [details]
Patch

[James Robinson]

Comment on attachment 68314 [details]
Patch

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.

[Chris Rogers]

Comment on attachment 68314 [details]
Patch

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

[Chris Rogers]

Created attachment 68959 [details]
Patch

[Chris Rogers]

I've done some performance tests to get a sense for how long it takes to re-create the HRTFDatabase each time a page is loaded.  It doesn't appear that bad, so HRTFDatabaseLoader no longer keeps the database in memory until the process dies.

Also, I've removed the "subjectMap" stuff from the code since it was just making things more complicated than necessary and isn't used in normal builds.

[James Robinson]

Comment on attachment 68959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68959&action=review

Looks good.  Two nits, feel free to fix while landing or upload a new patch.

> WebCore/platform/audio/HRTFDatabase.cpp:78
> +                double x = double(jj) / double(InterpolationFactor);

nit: static_cast<double>()

> WebCore/platform/audio/HRTFDatabase.h:62
> +    HRTFDatabase(double sampleRate);

nit: this needs explicit

[Chris Rogers]

Created attachment 68999 [details]
Patch

[Chris Rogers]

Thanks James.  This is a patch for the last nits - was R+ already.

[WebKit Commit Bot]

Comment on attachment 68999 [details]
Patch

Clearing flags on attachment: 68999

Committed r68481: <http://trac.webkit.org/changeset/68481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.