[Webkit-unassigned] [Bug 70140] Flush denormals to zero on Windows.

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Chris Rogers <crogers at google.com>  2011-10-14 15:04:18 PST ---
(From update of attachment 111072)
View in context: https://bugs.webkit.org/attachment.cgi?id=111072&action=review

> Source/WebCore/platform/audio/AudioBus.cpp:34
> +#include "DenormalDisabler.h"

WebKit style is to put one blank line after: #include "AudioBus.h"

> Source/WebCore/platform/audio/AudioBus.cpp:243
> +    float totalDesiredGain = static_cast<float>(m_busGain * targetGain);

Can't we also make "targetGain" and "m_busGain" a float and avoid the static_cast?

This will likely require changing certain other methods to also take "float" arguments

> Source/WebCore/platform/audio/AudioBus.cpp:246
> +    float gain = static_cast<float>(m_isFirstTime ? totalDesiredGain : *lastMixGain);

ditto

> Source/WebCore/platform/audio/AudioBus.cpp:259
> +    const float DezipperRate = 0.005;

doesn't this need to now be 0.005f ?

> Source/WebCore/platform/audio/AudioBus.cpp:269
> +                *destinationL++ = sumL;

probably can avoid the intermediate "sumL" and "sumR" variables and simply assign directly

> Source/WebCore/platform/audio/AudioBus.cpp:282
> +                float sumR = FlushToFloatZero(*destinationR + scaled);

ditto: can avoid sumL and sumR and assign directly

> Source/WebCore/platform/audio/AudioBus.cpp:295
> +                *destinationL++ = sum;

can avoid sum and assign directly

> Source/WebCore/platform/audio/AudioBus.cpp:342
> +    *lastMixGain = static_cast<double>(gain);

Probably best to make "lastMixGain" float instead of double and avoid the static_cast -- this will require all callers of this function to also switch over to float...

> Source/WebCore/platform/audio/DenormalDisabler.h:34
> +#include <math.h>

Header files should not be included inside of "namespace WebCore {"
please move includes to above the namespace

> Source/WebCore/platform/audio/DenormalDisabler.h:38
> +static inline float FlushToFloatZero(float f)

I'm not very fond of the name "FlushToFloatZero" -- I'd suggest including the word "denormal" in the name.
How about: "FlushFloatDenormalToZero"

Also, it's better to have functions like this be in some kind of namespace.  One idea is to move this inline function as a static inline method of DenormalDisabler

> Source/WebCore/platform/audio/DenormalDisabler.h:43
> +#define FlushToFloatZero(f) static_cast<float>(f)

It seems odd that on Windows this is an inline function, and otherwise is a macro -- maybe just have it uniformly be a static inline method of DenormalDisabler

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