Bug 70140 – Flush denormals to zero on Windows.

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

Flush denormals to zero on Windows.

[Raymond Toy]

Created attachment 111070 [details]
Patch

[WebKit Review Bot]

Attachment 111070 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/DenormalDisabler.h:32:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymond Toy]

This patch fixes an issue with denormal floats on Windows.  The issue can be seen at http://chromium.googlecode.com/svn/trunk/samples/audio/dj.html.  Drag the leftRhythmEffect slider to the right and then all the way to the left.  You will hear (and see) artifacts.  These are caused by denormal numbers which are very slow to process.

This doesn't happen on Linux or Mac because denormals are flushed to zero by the SSE hardware.  On windows, SSE is not used, and the x87 has no HW support for flushing denormals to zero.

Also, we converted the local variable, gain, from a double to a float to better match the float values of the inputs and outputs so unnecessary conversions are not needed.

[Raymond Toy]

Created attachment 111072 [details]
Patch

[WebKit Review Bot]

Attachment 111072 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/DenormalDisabler.h:32:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Rogers]

Comment on attachment 111072 [details]
Patch

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

[Raymond Toy]

Comment on attachment 111072 [details]
Patch

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.

[Raymond Toy]

Comment on attachment 111072 [details]
Patch

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

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

Oops.  Can't do that because the compiler correctly complains that 

*d++ = Flush(*d + gain * *source)

is undefined.  (Because we don't know when d++ happens because there's no sequence point for =.)

[Raymond Toy]

Created attachment 111105 [details]
Patch

[WebKit Review Bot]

Attachment 111105 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/DenormalDisabler.h:28:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/audio/DenormalDisabler.h:81:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Rogers]

Hi Ray, several of the fixes marked as "Done" are not actually in the latest patch - could you please re-check.

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

Ok, but then in that case please add a FIXME here

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

Seems fine to me.

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

I know, but it's more of a consistency/style readability thing

[Chris Rogers]

Comment on attachment 111105 [details]
Patch

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

Please look at style errors and address for next patch.

>> Source/WebCore/platform/audio/DenormalDisabler.h:28
>> +#if OS(WIN) && defined(_MSC_VER) && (_M_IX86_FP == 0)
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Is there any reason we need to include a Windows specific header file here?  It doesn't seem like we have to.  I'd recommend:

#include <wtf/MathExtras.h>

[Chris Rogers]

(In reply to comment #8)
> Oops.  Can't do that because the compiler correctly complains that 
> 
> *d++ = Flush(*d + gain * *source)
> 
> is undefined.  (Because we don't know when d++ happens because there's no sequence point for =.)

This should not be the case - please re-check

[Raymond Toy]

(In reply to comment #13)
> (In reply to comment #8)
> > Oops.  Can't do that because the compiler correctly complains that 
> > 
> > *d++ = Flush(*d + gain * *source)
> > 
> > is undefined.  (Because we don't know when d++ happens because there's no sequence point for =.)
> 
> This should not be the case - please re-check

I changed the lines from 

 float sumL = DenormalDisabler::FlushDenormalFloatToZero(*destinationL + gain * *sourceL++);
               
 *destinationL++ = sumL;

to

 *destinationL++ = DenormalDisabler::FlushDenormalFloatToZero(*destinationL + gain * *sourceL++);

Gcc (linux) complains:

cc1plus: warnings being treated as errors
third_party/WebKit/Source/WebCore/platform/audio/AudioBus.cpp: In member function ‘void WebCore::AudioBus::processWithGainFromMonoStereo(const WebCore::AudioBus&, double*, double, bool)’:
third_party/WebKit/Source/WebCore/platform/audio/AudioBus.cpp:276: error: operation on ‘destinationL’ may be undefined

[Chris Rogers]

Ok, if there's a compile error, then let's leave that for now and see if we can address the other comments.  Seems like we're getting close.

[Raymond Toy]

Created attachment 111451 [details]
Patch

[WebKit Review Bot]

Attachment 111451 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/DenormalDisabler.h:28:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/audio/DenormalDisabler.h:80:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymond Toy]

All review comments should be addressed now.  My apologies for overlooking some fixes from last time.

[Kenneth Russell]

Comment on attachment 111451 [details]
Patch

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

>> Source/WebCore/platform/audio/DenormalDisabler.h:28
>> +#if OS(WINDOWS) && COMPILER(MSVC) && (_M_IX86_FP == 0)
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

To satisfy the style queue the last expression should be written as (!_M_IX86_FP). Here and below.

[Chris Rogers]

Comment on attachment 111451 [details]
Patch

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

>>> Source/WebCore/platform/audio/DenormalDisabler.h:28
>>> +#if OS(WINDOWS) && COMPILER(MSVC) && (_M_IX86_FP == 0)
>> 
>> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> To satisfy the style queue the last expression should be written as (!_M_IX86_FP). Here and below.

Actually, just to make things simpler and more readable I would recommend removing the #if

It's true that it'll include an unneeded include file on some platforms, but it seems worth the cost due to readability.  What do you think Ken?

[Raymond Toy]

Created attachment 111467 [details]
Patch

[Kenneth Russell]

Comment on attachment 111467 [details]
Patch

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

Code looks good overall but there's a small naming convention issue that needs to be fixed before commit.

Also, please mark subsequent patches cq? if you'd like them submitted to the commit queue after review.

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

This should be "flushDenormalFloatToZero"; WebKit naming convention is camelCase for methods. See http://www.webkit.org/coding/coding-style.html .

[Raymond Toy]

Created attachment 111519 [details]
Patch

[Raymond Toy]

Renamed function to follow webkit style and removed the guards for MathExtras.h.

[Kenneth Russell]

Comment on attachment 111519 [details]
Patch

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

One more minor process nit. I'm not going to hold this patch up any further since I missed this in the last version, but please keep this in mind for future patches. r=me

> Source/WebCore/ChangeLog:13
> +        (WebCore::FlushToFloatZero):

Please regenerate the ChangeLog after renaming methods. Yes, this is tedious.

[Raymond Toy]

Created attachment 111643 [details]
Patch

[Raymond Toy]

Comment on attachment 111519 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        (WebCore::FlushToFloatZero):
> 
> Please regenerate the ChangeLog after renaming methods. Yes, this is tedious.

Fixed.  Sorry about that.

[Kenneth Russell]

Comment on attachment 111643 [details]
Patch

Looks great, thanks. r=me again

[WebKit Review Bot]

Comment on attachment 111643 [details]
Patch

Clearing flags on attachment: 111643

Committed r97912: <http://trac.webkit.org/changeset/97912>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.