Bug 217322 – [Cocoa] Vectorize linear to decibels conversion in RealtimeAnalyser

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Vectorize linear to decibels conversion in RealtimeAnalyser.

[Chris Dumez]

Created attachment 410530 [details]
Patch

[Chris Dumez]

Created attachment 410536 [details]
Patch

[Darin Adler]

Comment on attachment 410536 [details]
Patch

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

> Source/WebCore/Modules/webaudio/RealtimeAnalyser.cpp:190
> +    if (length > 0)

Is this if statement an important optimization? I would have done all of this in one line.

> Source/WebCore/platform/audio/VectorMath.cpp:755
> +    int n = framesToProcess;

Not OK to just put a size_t into an int, especially when there is no reason to.

Also not sure why the loop is written this way. Could use a for loop instead, I guess?

> Source/WebCore/platform/audio/VectorMath.h:56
> +void linearToDecibels(const float* sourceP, int sourceStride, float* destP, int destStride, size_t framesToProcess);

Can we do better on these argument names? Not sure sourceP is better than inputVector, for example. Looking at vDSP_vdbcon they seem to refer to these as input and output vector. Also they use the terminology "number of elements to process". I don’t think the argument should be named "frames to process" since it’s not frames, it’s a number.

Do we need to pass the stride values in? I understand that the underlying vector function takes a stride algorithm, but I am not sure we need that flexibility in our vector math wrapper interface.

[Chris Dumez]

Comment on attachment 410536 [details]
Patch

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

>> Source/WebCore/platform/audio/VectorMath.cpp:755
>> +    int n = framesToProcess;
> 
> Not OK to just put a size_t into an int, especially when there is no reason to.
> 
> Also not sure why the loop is written this way. Could use a for loop instead, I guess?

Ok. I was merely following the pattern in this file. I will fix all of them.

>> Source/WebCore/platform/audio/VectorMath.h:56
>> +void linearToDecibels(const float* sourceP, int sourceStride, float* destP, int destStride, size_t framesToProcess);
> 
> Can we do better on these argument names? Not sure sourceP is better than inputVector, for example. Looking at vDSP_vdbcon they seem to refer to these as input and output vector. Also they use the terminology "number of elements to process". I don’t think the argument should be named "frames to process" since it’s not frames, it’s a number.
> 
> Do we need to pass the stride values in? I understand that the underlying vector function takes a stride algorithm, but I am not sure we need that flexibility in our vector math wrapper interface.

Ok. I will look into improving our VectorMath API in a follow-up since your comments apply to all functions, not just the one I am adding.

[Chris Dumez]

Created attachment 410556 [details]
Patch

[Chris Dumez]

Created attachment 410558 [details]
Patch

[EWS]

Committed r268006: <https://trac.webkit.org/changeset/268006>

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

[Radar WebKit Bug Importer]

<rdar://problem/69972790>