Use VectorMath lib when possible to optimize the processing in WebAudio AudioBus
Created attachment 120717 [details] Patch
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.
Comment on attachment 120717 [details] Patch Should be fine, once Chris's comments are fixed.
Created attachment 121220 [details] Patch
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
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?
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.
Comment on attachment 121220 [details] Patch Since James isn't a committer a new patch is needed.
(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
Created attachment 121350 [details] Patch
(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.
(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?
(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" :)
(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
(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
Comment on attachment 121350 [details] Patch If it looks good to Chris it's fine with me. rs=me
Comment on attachment 121350 [details] Patch Clearing flags on attachment: 121350 Committed r104308: <http://trac.webkit.org/changeset/104308>
All reviewed patches have been landed. Closing bug.