Bug 80256 – Optimize for DARWIN in DirectConvolver::process()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Xingnan Wang]

conv() function in Accelerate.framework can be used.

[Chris Rogers]

Accelerate.framework conv() function may be optimized in DirectConvolver.cpp, but unlikely critical.

[Xingnan Wang]

Created attachment 137657 [details]
Patch

[Build Bot]

Comment on attachment 137657 [details]
Patch

Attachment 137657 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12429019

[Xingnan Wang]

Created attachment 137659 [details]
Patch

[Chris Rogers]

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?

[Chris Rogers]

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

[Xingnan Wang]

Created attachment 137815 [details]
Patch

[Xingnan Wang]

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.

[Chris Rogers]

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...

[Chris Rogers]

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!

[Jer Noble]

(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.

[Xingnan Wang]

Created attachment 137852 [details]
Patch

[Xingnan Wang]

(In reply to comment #12)
> Created an attachment (id=137852) [details]
> Patch

Updated the patch.

[Chris Rogers]

Comment on attachment 137852 [details]
Patch

Thanks Xingnan!

[WebKit Review Bot]

Comment on attachment 137852 [details]
Patch

Clearing flags on attachment: 137852

Committed r114621: <http://trac.webkit.org/changeset/114621>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.