[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 #7 from Raymond Toy <rtoy at chromium.org>  2011-10-14 15:40:55 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"

Done.

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

Yes we could.  I just wanted to keep this change self-contained.  But I can go and change the signature and all the callers appropriately.  I'd rather do that in two stages, though: This one for the important fix, and a follow-on with the targetGain and m_busGain change.

>> Source/WebCore/platform/audio/AudioBus.cpp:259
>> +    const float DezipperRate = 0.005;
> 
> doesn't this need to now be 0.005f ?

Done.

>> Source/WebCore/platform/audio/AudioBus.cpp:269
>> +                *destinationL++ = sumL;
> 
> probably can avoid the intermediate "sumL" and "sumR" variables and simply assign directly

Done.

>> Source/WebCore/platform/audio/AudioBus.cpp:282
>> +                float sumR = FlushToFloatZero(*destinationR + scaled);
> 
> ditto: can avoid sumL and sumR and assign directly

Done.

>> Source/WebCore/platform/audio/AudioBus.cpp:295
>> +                *destinationL++ = sum;
> 
> can avoid sum and assign directly

Done.

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

Same comment as for targetGain, above.

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

Done.

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

How about FlushDenormalFloatToZero?

I'll move it to a static inline method.

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

That's just an implementation detail; just made it obvious there's no impact even on debug builds.  But I won't have a choice if I make it an static inline method.

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