[webkit-reviews] review denied: [Bug 45864] Add HRTFElevation files : [Attachment 67759] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 45864: Add HRTFElevation files
https://bugs.webkit.org/show_bug.cgi?id=45864

Attachment 67759: Patch
https://bugs.webkit.org/attachment.cgi?id=67759&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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?