Bug 215391

Created attachment 406420 [details] Patch

Comment on attachment 406420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406420&action=review > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:111 > + unsigned k = floor(v); > + double f = v - k; What's the rationale for converting v to an unsigned and back to a double? That’s inefficient and I don’t think it’s needed once we’ve done the floor.

Comment on attachment 406420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406420&action=review >> Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:111 >> + double f = v - k; > > What's the rationale for converting v to an unsigned and back to a double? That’s inefficient and I don’t think it’s needed once we’ve done the floor. I don't understand this comment. I tried to match the formula in the specification as closely as possible. That said, the formula does not mention what data type should be used for each variable. k is used as index in curveData array so it needs to be an unsigned integer. However, v and f are not integers as far as I can tell.

Comment on attachment 406420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406420&action=review > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:102 > const float input = source[i]; I wonder if we have to convert everything to double to get correct results. Would be more efficient to do the work as float, but may not give identical results. > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:107 > + else if (v >= (curveLength - 1)) Unnecessary parentheses here. > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:108 > + destination[i] = curveData[curveLength -1]; Missing space after "-". >>> Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:111 >>> + double f = v - k; >> >> What's the rationale for converting v to an unsigned and back to a double? That’s inefficient and I don’t think it’s needed once we’ve done the floor. > > I don't understand this comment. I tried to match the formula in the specification as closely as possible. That said, the formula does not mention what data type should be used for each variable. > k is used as index in curveData array so it needs to be an unsigned integer. However, v and f are not integers as far as I can tell. I see; I didn’t notice that k was used as an array index. Could do this: double dk = floor(v); unsigned k = dk; double f = v - k; This would be more efficient than the code above.

Created attachment 406428 [details] Patch

Comment on attachment 406428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406428&action=review Ok, I have now using float instead of double for the computation. This still passes the tests and should suffice since values use float precision in the end. > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:110 > + float fk = floorf(v); Ok, I did it on 2 lines here as suggested. I don't really understand why it is faster though. If you have time, I'd love to understand why this is faster than: unsigned k = floorf(v);

Comment on attachment 406428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406428&action=review > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:112 > + float f = v - k; This is where the faster code would be: float f = v - fk; Sorry I wasn’t clear on that before!

Created attachment 406431 [details] Patch

(In reply to Darin Adler from comment #7) > Comment on attachment 406428 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406428&action=review > > > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:112 > > + float f = v - k; > > This is where the faster code would be: > > float f = v - fk; > > Sorry I wasn’t clear on that before! Oh, it makes sense now. Thanks for clarifying.

Comment on attachment 406431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406431&action=review > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:104 > + float v = (curveLength - 1) * 0.5 * (input + 1); This 0.5 is a double, so it makes the expression compute as double and then narrow down to float. Needs to be 0.5f if we want it to be done as float.

Committed r265536: <https://trac.webkit.org/changeset/265536> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406431 [details].

<rdar://problem/66870911>

(In reply to Darin Adler from comment #10) > Comment on attachment 406431 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406431&action=review > > > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:104 > > + float v = (curveLength - 1) * 0.5 * (input + 1); > > This 0.5 is a double, so it makes the expression compute as double and then > narrow down to float. Needs to be 0.5f if we want it to be done as float. Fixed in <https://trac.webkit.org/changeset/265538>, thanks.

Comment on attachment 406431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406431&action=review > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:110 > + float k = floorf(v); Tiny nit, but I have been trying to convert to using std::floor() in cases like this (it is overloaded based on type, so will end up calling floorf(), but generalizes better).

(In reply to Sam Weinig from comment #14) > Comment on attachment 406431 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406431&action=review > > > Source/WebCore/Modules/webaudio/WaveShaperDSPKernel.cpp:110 > > + float k = floorf(v); > > Tiny nit, but I have been trying to convert to using std::floor() in cases > like this (it is overloaded based on type, so will end up calling floorf(), > but generalizes better). Thanks for pointing it out. I agree it is nicer and made the nit fix in <https://trac.webkit.org/changeset/265552>.