Bug 75334 – Use VectorMath lib when possible to optimize the processing in WebAudio AudioBus

• Component: Web Audio
• Status: RESOLVED FIXED

[Wei James (wistoch)]

Use VectorMath lib when possible to optimize the processing in WebAudio AudioBus

[Wei James (wistoch)]

Created attachment 120717 [details]
Patch

[Chris Rogers]

Comment on attachment 120717 [details]
Patch

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

> Source/WebCore/platform/audio/AudioBus.cpp:241
> +#if (defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))) || (OS(WINDOWS) && COMPILER(MSVC) && (_M_IX86_FP)) 

I think we should have this conditional somewhere in DenormalDisabler.h, since that's where the rest of those low-level details are dealt with

Also, the macro name shouldn't be called USE_VECTOR_MATH_LIB, but instead should be more general -- something like HAS_HARDWARE_DENORMAL_FLUSHING
(or maybe you can think of a better and more accurate name)

> Source/WebCore/platform/audio/AudioBus.cpp:301
> +#define STEREO_NO_SUM_V \

I'd love to see if we could just stop worrying about denormals in the case when the gain has already converged to the target and avoid
the #if #else here and in the other two places below (in other words just always go with the vsmul case).  That might also simplify some of the other macros you've defined.

We can clamp (to zero) the "targetGain" when it's less than a certain lower value (like -100dB or maybe lower?)

It's true that there could be cases where targetGain is say -80dB and the values of the vector being scaled are already
quite small, pushing them into denormal range, but I think this case will not be common (and we can work more to avoid generating
them in the first place).   I'm worried about the extra code complexity of having to deal with denormals in this case, especially when we add in the other up and down-mixing cases.

[Raymond Toy]

Comment on attachment 120717 [details]
Patch

Should be fine, once Chris's comments are fixed.

[Wei James (wistoch)]

Created attachment 121220 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 120717 [details]
Patch

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

>> Source/WebCore/platform/audio/AudioBus.cpp:241
>> +#if (defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))) || (OS(WINDOWS) && COMPILER(MSVC) && (_M_IX86_FP)) 
> 
> I think we should have this conditional somewhere in DenormalDisabler.h, since that's where the rest of those low-level details are dealt with
> 
> Also, the macro name shouldn't be called USE_VECTOR_MATH_LIB, but instead should be more general -- something like HAS_HARDWARE_DENORMAL_FLUSHING
> (or maybe you can think of a better and more accurate name)

as said in the following comment, this part will be removed. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:301
>> +#define STEREO_NO_SUM_V \
> 
> I'd love to see if we could just stop worrying about denormals in the case when the gain has already converged to the target and avoid
> the #if #else here and in the other two places below (in other words just always go with the vsmul case).  That might also simplify some of the other macros you've defined.
> 
> We can clamp (to zero) the "targetGain" when it's less than a certain lower value (like -100dB or maybe lower?)
> 
> It's true that there could be cases where targetGain is say -80dB and the values of the vector being scaled are already
> quite small, pushing them into denormal range, but I think this case will not be common (and we can work more to avoid generating
> them in the first place).   I'm worried about the extra code complexity of having to deal with denormals in this case, especially when we add in the other up and down-mixing cases.

modified the code according to your comments. thanks

[Chris Rogers]

Comment on attachment 121220 [details]
Patch

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

> Source/WebCore/platform/audio/AudioBus.cpp:257
> +#define STEREO_SUM_V \

We could potentially also optimize the "SUM_V" cases.  We can't use vsmul() directly, but if we had something like vsmul_add() then we could!

Maybe add a FIXME here about this?

[Chris Rogers]

Comment on attachment 121220 [details]
Patch

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

Looks good to me with comments addressed.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Please remove this line.

[Kenneth Russell]

Comment on attachment 121220 [details]
Patch

Since James isn't a committer a new patch is needed.

[Wei James (wistoch)]

(In reply to comment #6)
> (From update of attachment 121220 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121220&action=review
> > Source/WebCore/platform/audio/AudioBus.cpp:257
> > +#define STEREO_SUM_V \
> We could potentially also optimize the "SUM_V" cases.  We can't use vsmul() directly, but if we had something like vsmul_add() then we could!
> Maybe add a FIXME here about this?

yes, I am working on it now. will add a new funciton on VectorMath and then use it here. thanks

[Wei James (wistoch)]

Created attachment 121350 [details]
Patch

[Chris Rogers]

(In reply to comment #9)
> > We could potentially also optimize the "SUM_V" cases.  We can't use vsmul() directly, but if we had something like vsmul_add() then we could!
> > Maybe add a FIXME here about this?
> 
> yes, I am working on it now. will add a new funciton on VectorMath and then use it here. thanks

Thanks James - looks good to me.

[Wei James (wistoch)]

(In reply to comment #11)
> (In reply to comment #9)
> > > We could potentially also optimize the "SUM_V" cases.  We can't use vsmul() directly, but if we had something like vsmul_add() then we could!
> > > Maybe add a FIXME here about this?
> > 
> > yes, I am working on it now. will add a new funciton on VectorMath and then use it here. thanks
> 
> Thanks James - looks good to me.

hi, roger, as we will use IPP to do FFT in web audio. In IPP, there are many highly optimized functions for vector operations. Can we also use them in VectorMath.cpp when we enabled IPP?

[Chris Rogers]

(In reply to comment #12)
> hi, roger, as we will use IPP to do FFT in web audio. In IPP, there are many highly optimized functions for vector operations. Can we also use them in VectorMath.cpp when we enabled IPP?

Sure, I don't see why you couldn't call into the IPP versions similar to what we already do today for OS(DARWIN) with its <Accelerate.framework> functions.  Partly, this depends on your judgement of whether you believe there would be an advantage compared with the SSE optimizations you've already made.

By the way, no worries, but my name is actually "Chris" :)

[Wei James (wistoch)]

(In reply to comment #13)
> (In reply to comment #12)
> > hi, roger, as we will use IPP to do FFT in web audio. In IPP, there are many highly optimized functions for vector operations. Can we also use them in VectorMath.cpp when we enabled IPP?
> 
> Sure, I don't see why you couldn't call into the IPP versions similar to what we already do today for OS(DARWIN) with its <Accelerate.framework> functions.  Partly, this depends on your judgement of whether you believe there would be an advantage compared with the SSE optimizations you've already made.
> 
> By the way, no worries, but my name is actually "Chris" :)

:) Chris, sorry for the misunderstanding. :) 

we will evaluate it and enable it if its performance is better(I believe so. :)) 

btw, could you have a look at this simple patch when you are free? Ken needs your confirmation to r+ it. 
https://bugs.webkit.org/show_bug.cgi?id=74693

[Wei James (wistoch)]

(In reply to comment #11)
> (In reply to comment #9)
> > > We could potentially also optimize the "SUM_V" cases.  We can't use vsmul() directly, but if we had something like vsmul_add() then we could!
> > > Maybe add a FIXME here about this?
> > 
> > yes, I am working on it now. will add a new funciton on VectorMath and then use it here. thanks
> 
> Thanks James - looks good to me.

Ken, could you help to land this patch firstly? 

we will do further optimize including adding new VectorMath function etc in another patch. 

thanks

[Kenneth Russell]

Comment on attachment 121350 [details]
Patch

If it looks good to Chris it's fine with me. rs=me

[WebKit Review Bot]

Comment on attachment 121350 [details]
Patch

Clearing flags on attachment: 121350

Committed r104308: <http://trac.webkit.org/changeset/104308>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.