Bug 217322

Created attachment 410530 [details] Patch

Created attachment 410536 [details] Patch

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.

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.

Created attachment 410556 [details] Patch

Created attachment 410558 [details] Patch

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

<rdar://problem/69972790>