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

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67759|review?                     |review-
               Flag|                            |




--- Comment #2 from James Robinson <jamesr at chromium.org>  2010-09-16 13:24:32 PST ---
(From update of attachment 67759)
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?

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