| Summary: | Add DynamicsCompressorNode implementation | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | jamesr, kbr, senorblanco | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Chris Rogers
2011-05-11 18:18:58 PDT
Created attachment 93231 [details]
Patch
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 Comment on attachment 93231 [details] Patch Attachment 93231 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8684628 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. 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? Created attachment 93528 [details]
Patch
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 Stephen, thanks for having a look. I think I've addressed all of your comments and fixed the mac build. 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). Committed r86607: <http://trac.webkit.org/changeset/86607> |