Bug 74359 – WebAudio: Optimize AudioChannel::maxAbsValue().

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

WebAudio: Optimize AudioChannel::maxAbsValue().

[Jer Noble]

Created attachment 118881 [details]
Patch

[Jer Noble]

Created attachment 118883 [details]
Patch

Rebased patch against ToT.

[Jer Noble]

Created attachment 118884 [details]
Patch

Rebased patch against ToT.

[Jer Noble]

Created attachment 118885 [details]
Patch

Rebased patch against ToT.

[Jer Noble]

Created attachment 118886 [details]
Patch

Rebased patch against ToT.

[Jer Noble]

Created attachment 118887 [details]
Patch

Rebased patch against ToT.

[Jer Noble]

Sigh.  Webkit-patch upload -g <commitish> still doesn't upload to the correct URL.

[Jer Noble]

Created attachment 118892 [details]
Patch

Rebased patch against ToT.

[Sam Weinig]

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.

[Jer Noble]

Created attachment 118910 [details]
Patch

Fixed off-by one error in vmaxmgv due to an excess of cleverness.

[Chris Rogers]

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

[Jer Noble]

Created attachment 122303 [details]
Patch

Nits picked, and comments added.

[Chris Rogers]

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

[Jer Noble]

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.

[Jer Noble]

Created attachment 122465 [details]
Patch

[Chris Rogers]

Looks good.

[WebKit Review Bot]

Comment on attachment 122465 [details]
Patch

Attachment 122465 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11237085

[Jer Noble]

Created attachment 122496 [details]
Patch

Addded necessary header files to VectorMath.cpp to fix Chromium compile error.

[Eric Carlson]

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.

[Jer Noble]

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.

[Jer Noble]

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.

[Jer Noble]

Committed r105078: <http://trac.webkit.org/changeset/105078>

[Jer Noble]

Build fix:

Committed r105079: <http://trac.webkit.org/changeset/105079>