[Webkit-unassigned] [Bug 60682] Add DynamicsCompressorNode implementation

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Stephen White <senorblanco at chromium.org>  2011-05-13 13:28:33 PST ---
(From update of attachment 93231)
View in context: https://bugs.webkit.org/attachment.cgi?id=93231&action=review

Most of my comments are style-related; I don't have anything useful to say about the audio algorithm itself (looks very cool, though).

The Mac build breakage should be fixed.

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:89
> +    float gk = 1.0 - gain / 20.0;

Use 1 and 20 here.  According to WebKit style, "Unless required in order to force floating point math, do not append .0, .f and .0f to floating point literals."  (I hate this rule, but when in Rome..).  Since gain is already a float, 20 will be promoted to float, as will 1.

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:220
> +    m_lastFilterStageGain = -1.0;

As above, these three should be -1, not -1.0.

> Source/WebCore/platform/audio/DynamicsCompressor.h:58
> +        ParamReduction,

This enum element seems to be unused, and could be removed.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:50
> +    return 1.0 - exp(-k * x);

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:106
> +    float dryMix = 1.0 - effectBlend;

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:117
> +    double fullRangeGain = (linearThreshold + kk * saturate(1.0 - linearThreshold, 1.0));

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:118
> +    double fullRangeMakeupGain = 1.0 / fullRangeGain;

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:141
> +    double kA = 0.9999999999999998*y1 + 1.8432219684323923e-16*y2 - 1.9373394351676423e-16*y3 + 8.824516011816245e-18*y4;

Nit:  probably should document how these coefficients were derived.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:165
> +            m_detectorAverage = 1.0;

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:167
> +            m_detectorAverage = 1.0;

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:189
> +            m_maxAttackCompressionDiffDb = -1.0;

-1, etc (apply to the rest of this patch as necessary)

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:219
> +            // Fix gremlins.

Don't feed them NaN after midnight.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:256
> +                    float L = *sourceL++;

Variable names shouldn't start with a capital.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:284
> +                double shapedInput = absInput < linearThreshold ? absInput : linearThreshold + kk * saturate(absInput - linearThreshold, 1.0 / kk);

Note:  If this loop is hot, I think you could hoist this reciprocal out of the loop (just in case the compiler doesn't).

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:291
> +                double dbPerFrame = attenuationDb / (0.0025 * sampleRate);

Same here.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:352
> +    // printf("m_compressorGain = %f\n", m_compressorGain);

Commented-out code should probably be removed.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:358
> +    m_compressorGain = 1.0;

1.0 -> 1

> Source/WebCore/platform/audio/DynamicsCompressorKernel.h:80
> +    enum { DefaultPreDelayFrames = 256 /*512*/ }; // setPreDelayTime() will override this initial value

/*512*/ should probably 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.