Bug 74372 - WebAudio: Optimize calculateNormalizationScale().
Summary: WebAudio: Optimize calculateNormalizationScale().
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 17:06 PST by Jer Noble
Modified: 2012-01-13 15:52 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.82 KB, patch)
2011-12-12 17:13 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (6.55 KB, patch)
2012-01-12 16:08 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (6.38 KB, patch)
2012-01-13 10:59 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2012-01-13 12:41 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 17:06:26 PST
WebAudio: Optimize calculateNormalizationScale().
Comment 1 Jer Noble 2011-12-12 17:13:28 PST
Created attachment 118916 [details]
Patch
Comment 2 Chris Rogers 2012-01-12 14:44:52 PST
Comment on attachment 118916 [details]
Patch

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

> Source/WebCore/platform/audio/Reverb.cpp:67
> +        float channelPower = 0.0;

WebKit style: 0 instead of 0.0

> Source/WebCore/platform/audio/Reverb.cpp:68
> +        vsvesq(response->channel(i)->data(), 1, &power, length);

You need to pass in &channelPower instead of &power

> Source/WebCore/platform/audio/VectorMath.cpp:90
> +void vsvesq(const float* sourceP, int sourceStride, float* dest, size_t framesToProcess)

nit: I'd use destP instead of dest to be consistent with the other functions

> Source/WebCore/platform/audio/VectorMath.cpp:92
> +    vDSP_svesq(const_cast<float*>(sourceP), sourceStride, dest, framesToProcess);

Might need the #if defined(__ppc__) || defined(__i386__) work-around like the other functions...

> Source/WebCore/platform/audio/VectorMath.cpp:309
> +void vsvesq(const float* sourceP, int sourceStride, float* dest, size_t framesToProcess);

nit: I'd use destP instead of dest to be consistent with the other functions

> Source/WebCore/platform/audio/VectorMath.h:40
> +void vsvesq(const float* sourceP, int sourceStride, float* dest, size_t framesToProcess);

Simple comment describing what function does.

consistency nit: dest -> destP
Comment 3 Jer Noble 2012-01-12 16:06:27 PST
(In reply to comment #2)
> (From update of attachment 118916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118916&action=review
> 
> > Source/WebCore/platform/audio/Reverb.cpp:67
> > +        float channelPower = 0.0;
> 
> WebKit style: 0 instead of 0.0

Will change.

> > Source/WebCore/platform/audio/Reverb.cpp:68
> > +        vsvesq(response->channel(i)->data(), 1, &power, length);
> 
> You need to pass in &channelPower instead of &power

Whoops!  Good catch.

> > Source/WebCore/platform/audio/VectorMath.cpp:90
> > +void vsvesq(const float* sourceP, int sourceStride, float* dest, size_t framesToProcess)
> 
> nit: I'd use destP instead of dest to be consistent with the other functions

How about "sumP"?

> > Source/WebCore/platform/audio/VectorMath.cpp:92
> > +    vDSP_svesq(const_cast<float*>(sourceP), sourceStride, dest, framesToProcess);
> 
> Might need the #if defined(__ppc__) || defined(__i386__) work-around like the other functions...

Nope, this is one of the functions not in vDSP_translate.h.

> > Source/WebCore/platform/audio/VectorMath.cpp:309
> > +void vsvesq(const float* sourceP, int sourceStride, float* dest, size_t framesToProcess);
> 
> nit: I'd use destP instead of dest to be consistent with the other functions

Ditto.

> > Source/WebCore/platform/audio/VectorMath.h:40
> > +void vsvesq(const float* sourceP, int sourceStride, float* dest, size_t framesToProcess);
> 
> Simple comment describing what function does.

Sure thing.

> consistency nit: dest -> destP

Ditto.
Comment 4 Jer Noble 2012-01-12 16:08:06 PST
Created attachment 122330 [details]
Patch
Comment 5 Chris Rogers 2012-01-12 17:06:28 PST
Looks ok to me, but it seems like your patch is not applying.  You might need to update your source and re-upload a new patch.

Ok with me once that's done and the bots pass.  I'm not a reviewer so I can't R+
Comment 6 Jer Noble 2012-01-13 10:59:41 PST
Created attachment 122462 [details]
Patch
Comment 7 WebKit Review Bot 2012-01-13 12:31:35 PST
Comment on attachment 122462 [details]
Patch

Attachment 122462 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11236142
Comment 8 Jer Noble 2012-01-13 12:41:01 PST
Created attachment 122480 [details]
Patch

Fixed a compile error in the non-Darwin section of VectorMath.cpp.
Comment 9 Eric Carlson 2012-01-13 15:05:15 PST
Comment on attachment 122480 [details]
Patch

rs=me based on Chris' "unofficial" review
Comment 10 Jer Noble 2012-01-13 15:52:24 PST
Committed r105004: <http://trac.webkit.org/changeset/105004>