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

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from James Robinson <jamesr at chromium.org>  2010-09-20 14:37:28 PST ---
(From update of attachment 67947)
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?

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