Bug 45864 – Add HRTFElevation files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add HRTFElevation files

[Chris Rogers]

Created attachment 67759 [details]
Patch

[James Robinson]

Comment on attachment 67759 [details]
Patch

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

> WebCore/platform/audio/HRTFElevation.cpp:57
> +    bool checkX = x >= 0.0 && x < 1.0;
> +    ASSERT(checkX);
> +    if (!checkX)
> +        x = 0.0;

What's wrong with x == 1.0?

> WebCore/platform/audio/HRTFElevation.cpp:122
> +    snprintf(filePath, sizeof(filePath), "%s/IRC_%s_C_R0195_T%03d_P%03d.aif", hrirSubDirectory, subjectName, azimuth, positiveElevation);

Yuck :(.  What's the plan for improving this?

> WebCore/platform/audio/HRTFElevation.cpp:266
> +    HRTFKernelList& kernelListL = *this->kernelListL();
> +    HRTFKernelList& kernelListR = *this->kernelListR();

I think it'd be clearer to just use m_kernelListL/m_kernelListR in the rest of this function.

> WebCore/platform/audio/HRTFElevation.h:43
> +class HRTFElevation {

If this is intended to be stored in an OwnPtr it should probably inherit from Noncopyable.

> WebCore/platform/audio/HRTFElevation.h:48
> +    static PassOwnPtr<HRTFElevation> createForSubject(const char* subjectName, int elevation, double sampleRate);

WebCore typically uses String for strings, not const char*.  What does subjectName mean here?

> WebCore/platform/audio/HRTFElevation.h:63
> +    void getKernelsFromAzimuth(double azimuthBlend, unsigned azimuthIndex, HRTFKernel* &kernelL, HRTFKernel* &kernelR, double& frameDelayL, double& frameDelayR);

Pointers to references?

> WebCore/platform/audio/HRTFElevation.h:75
> +    // Spacing, in degrees, between every azimuth loaded from resource.
> +    static unsigned azimuthSpacing() { return 15; }
> +    
> +    // Number of azimuths loaded from resource.
> +    static unsigned numberOfRawAzimuths() { return 360 / azimuthSpacing(); }
> +
> +    // Interpolates by this factor to get the total number of azimuths from every azimuth loaded from resource.
> +    static unsigned interpolationFactor() { return 8; }    
> +    
> +    // Total number of azimuths after interpolation.
> +    static unsigned numberOfTotalAzimuths() { return numberOfRawAzimuths() * interpolationFactor(); }

Why are these static functions instead of constants?

> WebCore/platform/audio/HRTFElevation.h:78
> +    // Given two HRTFKernels, and an interpolation factor x: 0 -> 1, returns an interpolated HRTFKernel.
> +    static PassRefPtr<HRTFKernel> createInterpolatedKernel(HRTFKernel* kernel1, HRTFKernel* kernel2, double x);

Why is this function part of HRTFElevation and not HRTFKernel?

> WebCore/platform/audio/HRTFElevation.h:92
> +    // For testing / development.  Not compiled / linked in normal usage.
> +    void writeHRIRFiles(const char* subjectName);

Should this be behind #ifndef NDEBUG then?  When do we plan to remove these?

[Chris Rogers]

Created attachment 67863 [details]
Patch

[Chris Rogers]

> WebCore/platform/audio/HRTFElevation.cpp:57
> +    bool checkX = x >= 0.0 && x < 1.0;
> +    ASSERT(checkX);
> +    if (!checkX)
> +        x = 0.0;

What's wrong with x == 1.0?

CHANGED: now clipping x to range 0 -> 1
An alternative would be to simply return 0 here.





> WebCore/platform/audio/HRTFElevation.cpp:122
> +    snprintf(filePath, sizeof(filePath), "%s/IRC_%s_C_R0195_T%03d_P%03d.aif", hrirSubDirectory, subjectName, azimuth, positiveElevation);

Yuck :(.  What's the plan for improving this?


Yeah, I know... For sandboxing reasons I understand that Chrome can't read directly from the file system (and probably WebKit2 will have a similar problem).
The long-term goal is to create an audio-file resource abstraction.  In a sandboxed implementation it could get the file data into the renderer process via shared memory.
Even once the code is changed to use this abstraction I suspect there will still be a similar type of snprintf (or equivalent) in order to construct the audio resource name.

Migrating to this resource abstraction must happen before it will be able to run in sandboxed Chrome, so there's little chance of this falling through the cracks.
In the meantime, during bringup of this code in WebKit trunk, it's essential to have this simpler code working in order to exercise and test that all of the other parts of the code
are working correctly.

I'll put a FIXME here to describe this situation.  But much of this code will still remain the same here since the createBusFromAudioFile() function I'm
currently using can access this abstraction.





> WebCore/platform/audio/HRTFElevation.cpp:266
> +    HRTFKernelList& kernelListL = *this->kernelListL();
> +    HRTFKernelList& kernelListR = *this->kernelListR();

I think it'd be clearer to just use m_kernelListL/m_kernelListR in the rest of this function.

AGREED - CHANGED




> WebCore/platform/audio/HRTFElevation.h:43
> +class HRTFElevation {

If this is intended to be stored in an OwnPtr it should probably inherit from Noncopyable.

FIXED






> WebCore/platform/audio/HRTFElevation.h:48
> +    static PassOwnPtr<HRTFElevation> createForSubject(const char* subjectName, int elevation, double sampleRate);

WebCore typically uses String for strings, not const char*.  What does subjectName mean here?


NOT YET ADDRESSED - will fix in next patch




> WebCore/platform/audio/HRTFElevation.h:63
> +    void getKernelsFromAzimuth(double azimuthBlend, unsigned azimuthIndex, HRTFKernel* &kernelL, HRTFKernel* &kernelR, double& frameDelayL, double& frameDelayR);

Pointers to references?

I'm returning two HRTFKernel* pointers.  Would you prefer HRTFKernel** ??




> WebCore/platform/audio/HRTFElevation.h:75
> +    // Spacing, in degrees, between every azimuth loaded from resource.
> +    static unsigned azimuthSpacing() { return 15; }
> +    
> +    // Number of azimuths loaded from resource.
> +    static unsigned numberOfRawAzimuths() { return 360 / azimuthSpacing(); }
> +
> +    // Interpolates by this factor to get the total number of azimuths from every azimuth loaded from resource.
> +    static unsigned interpolationFactor() { return 8; }    
> +    
> +    // Total number of azimuths after interpolation.
> +    static unsigned numberOfTotalAzimuths() { return numberOfRawAzimuths() * interpolationFactor(); }

Why are these static functions instead of constants?

FIXED





> WebCore/platform/audio/HRTFElevation.h:78
> +    // Given two HRTFKernels, and an interpolation factor x: 0 -> 1, returns an interpolated HRTFKernel.
> +    static PassRefPtr<HRTFKernel> createInterpolatedKernel(HRTFKernel* kernel1, HRTFKernel* kernel2, double x);

Why is this function part of HRTFElevation and not HRTFKernel?

FIXED: MOVED TO HRTFKernel




> WebCore/platform/audio/HRTFElevation.h:92
> +    // For testing / development.  Not compiled / linked in normal usage.
> +    void writeHRIRFiles(const char* subjectName);

Should this be behind #ifndef NDEBUG then?  When do we plan to remove these?


FIXED: REMOVED this since it's best to have this be a separate standalone function (in a separate file in my audio branch) operating on HRTFElevation rather than complicate the
header file.  This is never used when running in the browser environment normally.

[Simon Fraser (smfr)]

> > WebCore/platform/audio/HRTFElevation.cpp:122
> > +    snprintf(filePath, sizeof(filePath), "%s/IRC_%s_C_R0195_T%03d_P%03d.aif", hrirSubDirectory, subjectName, azimuth, positiveElevation);
> 
> Yuck :(.  What's the plan for improving this?
> 
> 
> Yeah, I know... For sandboxing reasons I understand that Chrome can't read directly from the file system (and probably WebKit2 will have a similar problem).
> The long-term goal is to create an audio-file resource abstraction.  In a sandboxed implementation it could get the file data into the renderer process via shared memory.
> Even once the code is changed to use this abstraction I suspect there will still be a similar type of snprintf (or equivalent) in order to construct the audio resource name.
> 
> Migrating to this resource abstraction must happen before it will be able to run in sandboxed Chrome, so there's little chance of this falling through the cracks.
> In the meantime, during bringup of this code in WebKit trunk, it's essential to have this simpler code working in order to exercise and test that all of the other parts of the code
> are working correctly.
> 
> I'll put a FIXME here to describe this situation.  But much of this code will still remain the same here since the createBusFromAudioFile() function I'm
> currently using can access this abstraction.

This worries me. Any new access to the filesystem could potentially open a huge security hole. I think you need to disable this feature until the issue is resolved.

Do you expect browser vendors to ship a bunch of new resource files? If so, where are they?

[Chris Rogers]

Hi Simon, yes in order to do spatialized panning, a key feature of this audio API, we need to have access to the spatialization files.  Currently they're a little over 1Mb, but I'm confident that I can reduce them down to less than 500Kb total.  You can see them in the audio branch here:

https://svn.webkit.org/repository/webkit/branches/audio/WebCore/webaudio/AudioSpatialization/IRC_Composite/

I definitely understand your concern.  The HRTFElevation.cpp file is enabled with

#if ENABLE(WEB_AUDIO)

And I understand that WEB_AUDIO will not be enabled until we have proper layout tests, and this file reading issue is resolved.


But, in the meantime it's very important for me to be able to verify that the audio engine code still works as I'm landing patches into trunk from my branch.  In the course of addressing reviewers comments for my many patches, I have to make lots of re-factorings and code cleanup fixes and I need to verify that each change I make still works with all the rest of the code.  That's why it's important to me to be able to have this intermediate stage (before WEB_AUDIO is enabled) where I can directly read these spatialization files.

[Simon Fraser (smfr)]

Why not load these files via remote URLs like everything else?

[Chris Rogers]

We don't do that for other things in the browser like standard fonts for rendering a web page.  I view these resources to be in the same category.  It's not as if there are many different sets of these spatialization files.  They're quite hard to generate and require lots of specialized post-processing, so I don't envision anybody using any other than the standard dataset that we provide.

[Simon Fraser (smfr)]

We use file urls to load images etc. from the WebKit bundle, so I think we should do the same with these.

Also, a blocking read on the main thread is very undesirable.

FWIW, I guess they'd ship with WebKit, not with the browser.

[Simon Fraser (smfr)]

See Image::loadPlatformResource() for an example of how we load local resources.

[Chris Rogers]

Ok, loading from file URLs seems like a good solution for Safari's implementation of the abstraction layer.  Chrome might do it a little bit differently (maybe with shared memory).  I'll have a look at Image::loadPlatformResource()

Also I agree about the blocking reads on the main thread.  I'm planning on loading and processing these files on a separate thread.  The separate thread part will be part of the patch for HRTFDatabase which I'll post soon.

Thanks for your help, Simon.

[Chris Rogers]

Created attachment 67947 [details]
Patch

[Chris Rogers]

James, I think I've addressed your comments except for "Pointers to references?" and am waiting for your opinion on how you would like it changed.

Otherwise, I've created an abstraction function called createBusFromAudioFileResource(const String&),
which given an audio resource name will return the AudioBus for its PCM data.  This removes a lot of the ugliness from this part of my previous patch.

Also, I'm still calling fprintf(stderr, ...) in a few places.  Can you recommend which appropriate LOG() function I should use?

Thanks.

[James Robinson]

Comment on attachment 67947 [details]
Patch

Some comments.  It looks like this file doesn't actually handle the resource loading itself, that's over in AudioBus.

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

> WebCore/platform/audio/HRTFElevation.cpp:54
> +    RefPtr<HRTFKernel> l1;

"l1" as in ell-one looks almost identical to 11 as in eleven in the code review tool's font.  Can you please rename these variables to something more descriptive?

> WebCore/platform/audio/HRTFElevation.cpp:90
> +    String resourceName = String::format("IRC_%s/IRC_%s_C_R0195_T%03d_P%03d.aif", subjectName.utf8().data(), subjectName.utf8().data(), azimuth, positiveElevation);

Going from UTF16 -> UTF8 here in the formatting is unfortunate.  I think it'd be better to format the numerical values (azimuth, positiveElevation) to Strings using String::number() and then append the strings together using operator+ or .append() or a StringBuilder.

I bring it up because I'm not sure how format()'s %s operator handles arbitrary UTF8 data.

Another approach would be to let the AudioBus class handle formatting a resource name, since it appears that it's responsible for doing the resource load.

> WebCore/platform/audio/HRTFElevation.cpp:97
> +        fprintf(stderr, "Cannot read HRIR impulse response: %s\n", resourceName.utf8().data());
> +        exit(-1);

I think you want ASSERT() here and an early-return in release builds.  That will crash in debug builds.

> WebCore/platform/audio/HRTFElevation.cpp:198
> +    bool checkX = x >= 0.0 && x < 1.0;
> +    ASSERT(checkX);
> +    x = max(0.0, x);
> +    x = min(1.0, x);

nit: this could all be one expression.  Additionally, if passing an out-of-bounds value is an error by the caller and causes an ASSERT() failure then you could assume that x is always in bounds in release builds and not clamp it.

> WebCore/platform/audio/HRTFElevation.cpp:217
> +    double angle1 = hrtfElevation1->elevationAngle();
> +    double angle2 = hrtfElevation2->elevationAngle();
> +    double angle = (1.0 - x) * angle1 + x * angle2;

These locals don't seem to add very much here.

> WebCore/platform/audio/HRTFElevation.cpp:241
> +    kernelL = (*m_kernelListL)[azimuthIndex].get();
> +    kernelR = (*m_kernelListR)[azimuthIndex].get();

You can also do m_kernelListL->at(azimuthIndex) here and other places if you think that's prettier.

> WebCore/platform/audio/HRTFElevation.h:67
> +    void getKernelsFromAzimuth(double azimuthBlend, unsigned azimuthIndex, HRTFKernel* &kernelL, HRTFKernel* &kernelR, double& frameDelayL, double& frameDelayR);

Ah, I see.  These are two references to pointers to HRTFKernels and are being used as out parameters.  Whole lotta out params here.

> WebCore/platform/audio/HRTFElevation.h:79
> +    // Spacing, in degrees, between every azimuth loaded from resource.
> +    static const unsigned AzimuthSpacing = 15;
> +    
> +    // Number of azimuths loaded from resource.
> +    static const unsigned NumberOfRawAzimuths = 360 / AzimuthSpacing;
> +
> +    // Interpolates by this factor to get the total number of azimuths from every azimuth loaded from resource.
> +    static const unsigned InterpolationFactor = 8;
> +    
> +    // Total number of azimuths after interpolation.
> +    static const unsigned NumberOfTotalAzimuths = NumberOfRawAzimuths * InterpolationFactor;

I believe the all need storage declared in the .cpp.  You could also put the actual values in the .cpp to make the interface boundary a bit stronger.

> WebCore/platform/audio/HRTFElevation.h:98
> +    HRTFElevation()
> +        : m_kernelListL(0)
> +        , m_kernelListR(0)
> +        , m_elevationAngle(0)
> +        , m_sampleRate(44100.0)
> +    {
> +    }

Looks like this constructor is unused, can it be removed?

[Chris Rogers]

Created attachment 68303 [details]
Patch

[Chris Rogers]

James, I think I've addressed all of your comments.  I've simplified the string formatting down as we discussed offline and added a comment there.

Thanks for your help!

[James Robinson]

Comment on attachment 68303 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 68303 [details]
Patch

Clearing flags on attachment: 68303

Committed r68029: <http://trac.webkit.org/changeset/68029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.