WebAudio: Optimize AudioChannel::maxAbsValue().
Created attachment 118881 [details] Patch
Created attachment 118883 [details] Patch Rebased patch against ToT.
Created attachment 118884 [details] Patch Rebased patch against ToT.
Created attachment 118885 [details] Patch Rebased patch against ToT.
Created attachment 118886 [details] Patch Rebased patch against ToT.
Created attachment 118887 [details] Patch Rebased patch against ToT.
Sigh. Webkit-patch upload -g <commitish> still doesn't upload to the correct URL.
Created attachment 118892 [details] Patch Rebased patch against ToT.
Comment on attachment 118892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118892&action=review > Source/WebCore/platform/audio/VectorMath.cpp:85 > +void vmaxmgv(const float *sourceP, int sourceStride, float *dest, size_t framesToProcess) * on the wrong side. > Source/WebCore/platform/audio/VectorMath.cpp:291 > +void vmaxmgv(const float *sourceP, int sourceStride, float *dest, size_t framesToProcess) * on the wrong side. > Source/WebCore/platform/audio/VectorMath.h:39 > +void vmaxmgv(const float *sourceP, int sourceStride, float *dest, size_t framesToProcess); * on the wrong side.
Created attachment 118910 [details] Patch Fixed off-by one error in vmaxmgv due to an excess of cleverness.
Comment on attachment 118910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118910&action=review Looks good overall. You might need to rebaseline this patch before landing > Source/WebCore/platform/audio/VectorMath.cpp:85 > +void vmaxmgv(const float *sourceP, int sourceStride, float *dest, size_t framesToProcess) nit: I would call the argument "max" or "maxP" instead of "dest" since the other functions with "dest" type arguments are all output vectors instead of a scalar > Source/WebCore/platform/audio/VectorMath.h:39 > +void vmaxmgv(const float *sourceP, int sourceStride, float *dest, size_t framesToProcess); I would add a brief comment about what this function does, since its not clear from its name. nit: I would call the argument "max" or "maxP" instead of "dest" since the other functions with "dest" type arguments are all output vectors instead of a scalar
Created attachment 122303 [details] Patch Nits picked, and comments added.
Comment on attachment 122303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122303&action=review > Source/WebCore/platform/audio/VectorMath.cpp:105 > + vDSP_maxmgv(sourceP, sourceStride, maxP, framesToProcess); Do we need the #if defined(__ppc__) || defined(__i386__) work-around for this similar to the other functions in this file? If that work-around is no longer necessary then we should remove them all. > Source/WebCore/platform/audio/VectorMath.cpp:402 > + *maxP = max; just to be pedantic, maybe check for NULL pointer
Comment on attachment 122303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122303&action=review >> Source/WebCore/platform/audio/VectorMath.cpp:105 >> + vDSP_maxmgv(sourceP, sourceStride, maxP, framesToProcess); > > Do we need the #if defined(__ppc__) || defined(__i386__) work-around for this similar to the other functions in this file? If that work-around is no longer necessary then we should remove them all. Not all of the vDSP functions are included in the vDSP_translate.h file, and vDSP_maxmgv is one of those which isn't, so that particular workaround isn't necessary for this function. >> Source/WebCore/platform/audio/VectorMath.cpp:402 >> + *maxP = max; > > just to be pedantic, maybe check for NULL pointer Sure thing.
Created attachment 122465 [details] Patch
Looks good.
Comment on attachment 122465 [details] Patch Attachment 122465 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11237085
Created attachment 122496 [details] Patch Addded necessary header files to VectorMath.cpp to fix Chromium compile error.
Comment on attachment 122496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122496&action=review > Source/WebCore/platform/audio/VectorMath.cpp:101 > +void vmaxmgv(const float *sourceP, int sourceStride, float *maxP, size_t framesToProcess) Aren't the *s on the wrong side? > Source/WebCore/platform/audio/VectorMath.cpp:423 > +void vmaxmgv(const float *sourceP, int sourceStride, float *maxP, size_t framesToProcess) Ditto. > Source/WebCore/platform/audio/VectorMath.h:41 > +void vmaxmgv(const float *sourceP, int sourceStride, float *maxP, size_t framesToProcess); Ditto.
Comment on attachment 122496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122496&action=review >> Source/WebCore/platform/audio/VectorMath.cpp:101 >> +void vmaxmgv(const float *sourceP, int sourceStride, float *maxP, size_t framesToProcess) > > Aren't the *s on the wrong side? According to the Style Guidelines: "Pointer types in non-C++ code — Pointer types should be written with a space between the type and the * (so the * is adjacent to the following identifier if any)." Of course, I've been told in the past that the guidelines page is out of date, and that "non-C++ code" should read "ObjectiveC code". At this point, I have no idea where the * should go. I'll move them, just to make them match the rest of the code.
Committed r105078: <http://trac.webkit.org/changeset/105078>
Build fix: Committed r105079: <http://trac.webkit.org/changeset/105079>