Flush denormals to zero on Windows.
Created attachment 111070 [details] Patch
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.
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.
Created attachment 111072 [details] Patch
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.
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
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.
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 =.)
Created attachment 111105 [details] Patch
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.
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
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>
(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
(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
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.
Created attachment 111451 [details] Patch
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.
All review comments should be addressed now. My apologies for overlooking some fixes from last time.
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.
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?
Created attachment 111467 [details] Patch
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 .
Created attachment 111519 [details] Patch
Renamed function to follow webkit style and removed the guards for MathExtras.h.
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.
Created attachment 111643 [details] Patch
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.
Comment on attachment 111643 [details] Patch Looks great, thanks. r=me again
Comment on attachment 111643 [details] Patch Clearing flags on attachment: 111643 Committed r97912: <http://trac.webkit.org/changeset/97912>
All reviewed patches have been landed. Closing bug.