Bug 74359 - WebAudio: Optimize AudioChannel::maxAbsValue().
Summary: WebAudio: Optimize AudioChannel::maxAbsValue().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-12 15:24 PST by Jer Noble
Modified: 2012-01-16 11:29 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.93 KB, patch)
2011-12-12 15:26 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2011-12-12 15:37 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (18.45 KB, patch)
2011-12-12 15:37 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (159.24 KB, patch)
2011-12-12 15:38 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2011-12-12 15:38 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (8.91 KB, patch)
2011-12-12 15:38 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2011-12-12 15:55 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (4.02 KB, patch)
2011-12-12 16:57 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (4.09 KB, patch)
2012-01-12 13:46 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2012-01-13 11:06 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (4.07 KB, patch)
2012-01-13 14:00 PST, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-12-12 15:24:28 PST
WebAudio: Optimize AudioChannel::maxAbsValue().
Comment 1 Jer Noble 2011-12-12 15:26:48 PST
Created attachment 118881 [details]
Patch
Comment 2 Jer Noble 2011-12-12 15:37:46 PST
Created attachment 118883 [details]
Patch

Rebased patch against ToT.
Comment 3 Jer Noble 2011-12-12 15:37:57 PST
Created attachment 118884 [details]
Patch

Rebased patch against ToT.
Comment 4 Jer Noble 2011-12-12 15:38:07 PST
Created attachment 118885 [details]
Patch

Rebased patch against ToT.
Comment 5 Jer Noble 2011-12-12 15:38:17 PST
Created attachment 118886 [details]
Patch

Rebased patch against ToT.
Comment 6 Jer Noble 2011-12-12 15:38:28 PST
Created attachment 118887 [details]
Patch

Rebased patch against ToT.
Comment 7 Jer Noble 2011-12-12 15:39:48 PST
Sigh.  Webkit-patch upload -g <commitish> still doesn't upload to the correct URL.
Comment 8 Jer Noble 2011-12-12 15:55:39 PST
Created attachment 118892 [details]
Patch

Rebased patch against ToT.
Comment 9 Sam Weinig 2011-12-12 16:47:48 PST
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.
Comment 10 Jer Noble 2011-12-12 16:57:00 PST
Created attachment 118910 [details]
Patch

Fixed off-by one error in vmaxmgv due to an excess of cleverness.
Comment 11 Chris Rogers 2012-01-12 12:35:39 PST
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
Comment 12 Jer Noble 2012-01-12 13:46:05 PST
Created attachment 122303 [details]
Patch

Nits picked, and comments added.
Comment 13 Chris Rogers 2012-01-12 14:32:43 PST
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 14 Jer Noble 2012-01-12 14:59:40 PST
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.
Comment 15 Jer Noble 2012-01-13 11:06:53 PST
Created attachment 122465 [details]
Patch
Comment 16 Chris Rogers 2012-01-13 12:54:29 PST
Looks good.
Comment 17 WebKit Review Bot 2012-01-13 12:58:41 PST
Comment on attachment 122465 [details]
Patch

Attachment 122465 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11237085
Comment 18 Jer Noble 2012-01-13 14:00:51 PST
Created attachment 122496 [details]
Patch

Addded necessary header files to VectorMath.cpp to fix Chromium compile error.
Comment 19 Eric Carlson 2012-01-16 08:57:26 PST
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 20 Jer Noble 2012-01-16 10:03:01 PST
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.
Comment 21 Jer Noble 2012-01-16 10:03:05 PST
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.
Comment 22 Jer Noble 2012-01-16 10:53:22 PST
Committed r105078: <http://trac.webkit.org/changeset/105078>
Comment 23 Jer Noble 2012-01-16 11:29:06 PST
Build fix:

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