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