Bug 219546

Created attachment 415438 [details]
Patch

Created attachment 415442 [details]
Patch

Comment on attachment 415442 [details]
Patch

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

How did you test all four versions?

> Source/WebCore/platform/audio/SincResampler.cpp:348
> +    vDSP_dotpr(inputP, 1, k1, 1, &sum1, kernelSize);
> +    vDSP_dotpr(inputP, 1, k2, 1, &sum2, kernelSize);

Is there a reason that we don’t write a 4 different implementations of a "dot product" function instead of having the algorithm differ a level above that?

> Source/WebCore/platform/audio/SincResampler.cpp:351
> +    return static_cast<float>((1.0 - kernelInterpolationFactor) * sum1 + kernelInterpolationFactor * sum2);

Is it important do do this as double and then convert back to float? I notice we do that in both versions.

(In reply to Darin Adler from comment #3)
> Comment on attachment 415442 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415442&action=review
> 
> How did you test all four versions?

I have an Intel Mac so I was able to test ACCELERATE, SSE and slow implementations.
The only implementation I could not test was NEON but this implementation is a straight import from Chromium.

CPU(ARM_NEON) does not appear to be true on iPhone unless I tested wrong.

> 
> > Source/WebCore/platform/audio/SincResampler.cpp:348
> > +    vDSP_dotpr(inputP, 1, k1, 1, &sum1, kernelSize);
> > +    vDSP_dotpr(inputP, 1, k2, 1, &sum2, kernelSize);
> 
> Is there a reason that we don’t write a 4 different implementations of a
> "dot product" function instead of having the algorithm differ a level above
> that?

I guess we could do that. I was trying to stay in sync with Chromium here. ACCELERATE() is the only implementation that is not present in Chromium but that I chose to add for better performance on Cocoa platforms.

> 
> > Source/WebCore/platform/audio/SincResampler.cpp:351
> > +    return static_cast<float>((1.0 - kernelInterpolationFactor) * sum1 + kernelInterpolationFactor * sum2);
> 
> Is it important do do this as double and then convert back to float? I
> notice we do that in both versions.

Probably not since our vectorized versions only work with floats. I will test and fix.

Created attachment 415453 [details]
Patch

Committed r270457: <https://trac.webkit.org/changeset/270457>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415453 [details].

<rdar://problem/71993191>

Maybe related: https://bugs.webkit.org/show_bug.cgi?id=219676?