Bug 80256 - Optimize for DARWIN in DirectConvolver::process()
Summary: Optimize for DARWIN in DirectConvolver::process()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 75564
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-05 01:47 PST by Xingnan Wang
Modified: 2012-04-19 00:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.86 KB, patch)
2012-04-18 00:11 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (2.86 KB, patch)
2012-04-18 01:24 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2012-04-18 19:10 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (2.70 KB, patch)
2012-04-18 23:19 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xingnan Wang 2012-03-05 01:47:06 PST
conv() function in Accelerate.framework can be used.
Comment 1 Chris Rogers 2012-04-05 00:28:17 PDT
Accelerate.framework conv() function may be optimized in DirectConvolver.cpp, but unlikely critical.
Comment 2 Xingnan Wang 2012-04-18 00:11:32 PDT
Created attachment 137657 [details]
Patch
Comment 3 Build Bot 2012-04-18 00:47:26 PDT
Comment on attachment 137657 [details]
Patch

Attachment 137657 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12429019
Comment 4 Xingnan Wang 2012-04-18 01:24:41 PDT
Created attachment 137659 [details]
Patch
Comment 5 Chris Rogers 2012-04-18 10:29:39 PDT
Comment on attachment 137659 [details]
Patch

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

Xingnan, thanks for doing this - looks reasonable, but I'd like to have a chance to try out the patch...

> Source/WebCore/platform/audio/DirectConvolver.h:40
> +#endif

Can we move this include to the .cpp file?
Comment 6 Chris Rogers 2012-04-18 14:39:50 PDT
Comment on attachment 137659 [details]
Patch

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

Seems to sound fine, passes layout tests, etc. just the two nits with the #include and the #if stuff:

> Source/WebCore/platform/audio/DirectConvolver.cpp:88
> +#if OS(DARWIN)

Instead of nesting the #if OS(DARWIN) inside of the  #if USE(WEBAUDIO_IPP)
can we instead use #elif

#if USE(WEBAUDIO_IPP)
#elif OS(DARWIN)
#else
#endif
Comment 7 Xingnan Wang 2012-04-18 19:10:28 PDT
Created attachment 137815 [details]
Patch
Comment 8 Xingnan Wang 2012-04-18 19:13:00 PDT
Comment on attachment 137659 [details]
Patch

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


Updated the patch, thanks.

>> Source/WebCore/platform/audio/DirectConvolver.cpp:88
>> +#if OS(DARWIN)
> 
> Instead of nesting the #if OS(DARWIN) inside of the  #if USE(WEBAUDIO_IPP)
> can we instead use #elif
> 
> #if USE(WEBAUDIO_IPP)
> #elif OS(DARWIN)
> #else
> #endif

Done.

>> Source/WebCore/platform/audio/DirectConvolver.h:40
>> +#endif
> 
> Can we move this include to the .cpp file?

Done.
Comment 9 Chris Rogers 2012-04-18 20:32:58 PDT
Xingnan, I'm very sorry I didn't see this earlier.  I now see that your original approach was better since it's able to share more common code with the un-optimized case.  Can we try a new patch similar to patch #2, but with the header file fix?  Sorry about that.

Also, we *might* need to handle the macro insanity similar to what we do with some of the other vDSP functions in VectorMath.cpp - adding Jer Noble to verify...
Comment 10 Chris Rogers 2012-04-18 20:34:14 PDT
Jer, can you please have a look to see if we need the:

#if defined(__ppc__) || defined(__i386__)

stuff that some other vecLib functions need (in VectorMath.cpp

Thanks!
Comment 11 Jer Noble 2012-04-18 22:31:00 PDT
(In reply to comment #10)
> Jer, can you please have a look to see if we need the:
> 
> #if defined(__ppc__) || defined(__i386__)
> 
> stuff that some other vecLib functions need (in VectorMath.cpp
> 
> Thanks!

Yep.  vDSP_conv() is defined as a macro in vDSP_translate.h on x86_64.
Comment 12 Xingnan Wang 2012-04-18 23:19:02 PDT
Created attachment 137852 [details]
Patch
Comment 13 Xingnan Wang 2012-04-18 23:23:28 PDT
(In reply to comment #12)
> Created an attachment (id=137852) [details]
> Patch

Updated the patch.
Comment 14 Chris Rogers 2012-04-19 00:12:43 PDT
Comment on attachment 137852 [details]
Patch

Thanks Xingnan!
Comment 15 WebKit Review Bot 2012-04-19 00:20:53 PDT
Comment on attachment 137852 [details]
Patch

Clearing flags on attachment: 137852

Committed r114621: <http://trac.webkit.org/changeset/114621>
Comment 16 WebKit Review Bot 2012-04-19 00:20:58 PDT
All reviewed patches have been landed.  Closing bug.