Bug 60682 – Add DynamicsCompressorNode implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add DynamicsCompressorNode implementation

[Chris Rogers]

Created attachment 93231 [details]
Patch

[Chris Rogers]

This implements DynamicsCompressorNode as described here:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#DynamicsCompressorNode-section

A dynamics compression effect is commonly used in musical production and game audio. It lowers the volume of the loudest parts of the signal and raises the volume of the softest parts, making the sound richer, fuller, and more controlled.

Here are some links to help review the different parts of the compression algorithm:

* The pre/post emphasis filter is shown here
http://chromium.googlecode.com/svn/trunk/samples/audio/emphasis-filter.html

* The static compression curve
http://chromium.googlecode.com/svn/trunk/samples/audio/compression-curve.html

* Exponential slew warping
http://chromium.googlecode.com/svn/trunk/samples/audio/exponential-warp.html

* Adaptive release rate
http://chromium.googlecode.com/svn/trunk/samples/audio/adaptive-release.html

[WebKit Review Bot]

Comment on attachment 93231 [details]
Patch

Attachment 93231 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8684628

[Chris Rogers]

This Wikipedia article is probably a useful reference:
http://en.wikipedia.org/wiki/Dynamic_range_compression

Please note that the "ratio" is implemented as "headroom" in this algorithm.  See the "static compression curve" demo:
http://chromium.googlecode.com/svn/trunk/samples/audio/compression-curve.html

But it would be trivial to implement other compression curves, including ones which use "ratio" as a parameter.

This algorithm is "stereo linked" as described in the article.  "Makeup gain" is automatically computed, although additional "post gain" can be set.  There is a "Look-ahead" section, with adjustable pre-delay.

In theory, multi-band compression can be achieved by combining several AudioNodes together with appropriate cross-over filters.

[Stephen White]

Comment on attachment 93231 [details]
Patch

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?

[Chris Rogers]

Created attachment 93528 [details]
Patch

[Chris Rogers]

View in context: https://bugs.webkit.org/attachment.cgi?id=93231&action=review

>> 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.

FIXED

>> Source/WebCore/platform/audio/DynamicsCompressor.cpp:220
>> +    m_lastFilterStageGain = -1.0;
> 
> As above, these three should be -1, not -1.0.

FIXED

>> Source/WebCore/platform/audio/DynamicsCompressor.h:58
>> +        ParamReduction,
> 
> This enum element seems to be unused, and could be removed.

FIXED

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

FIXED

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

FIXED

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

FIXED

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

FIXED

>> 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.

FIXED

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

FIXED

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

FIXED

>> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:189
>> +            m_maxAttackCompressionDiffDb = -1.0;
> 
> -1, etc (apply to the rest of this patch as necessary)

FIXED

>> 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.

FIXED

>> 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).

FIXED

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

FIXED

>> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:352
>> +    // printf("m_compressorGain = %f\n", m_compressorGain);
> 
> Commented-out code should probably be removed.

FIXED

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

FIXED

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

FIXED

[Chris Rogers]

Stephen, thanks for having a look.  I think I've addressed all of your comments and fixed the mac build.

[Stephen White]

Comment on attachment 93528 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93528&action=review

Looks good.  r=me

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:298
> +                double dbPerFrame = attenuationDb / satReleaseFrames;

Perf nit:  This division could still be hoisted out of the loop (just a side note, totally up to you if it's necessary).

[Chris Rogers]

Committed r86607: <http://trac.webkit.org/changeset/86607>