[Webkit-unassigned] [Bug 45864] Add HRTFElevation files

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Chris Rogers <crogers at google.com>  2010-09-16 17:14:45 PST ---
> 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.

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