Bug 74054 – fast path to accelerate processing in AudioBus::processWithGainFromMonoStereo

• Component: Web Audio
• Status: RESOLVED FIXED

[Wei James (wistoch)]

fast path to accelerate processing in AudioBus::processWithGainFromMonoStereo

[Wei James (wistoch)]

Created attachment 118326 [details]
Patch

[Raymond Toy]

Comment on attachment 118326 [details]
Patch

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

Just a few minor questions;  the code changes look good.

> Source/WebCore/ChangeLog:3
> +        fast path to accelerate processing in AudioBus::processWithGainFromMonoStereo

Any idea on how much faster this runs with these changes?

> Source/WebCore/platform/audio/AudioBus.cpp:250
> +    bool needDezipper = fabs(totalDesiredGain-gain) > 0.001f;

How did you come up with 0.001?  If you just picked this "at random", please add a comment saying something like .001 seems small enough, but may need tweaking.

[Raymond Toy]

A side note on a different optimization.  It's not necessary to implement this instead of the proposed patch.

With the current patch, we still do (slightly) extra work if needDezipper is true.  During the loop, we may get close enough to the target that we don't need to dezipper anymore.

Since the dezippering is just an exponential filter, we can figure exactly how many steps will be needed to get with epsilon of the target value.  So compute this value, nConv, and have two loops:

N = min(nConv, framesToProcess);
for (k = 0; k < N; ++k) {
   <loop with dezippering>
}

for (k < framesToProcess; ++k) {
   <loop without dezippering>
}

Assuming I did the math correctly, 

nConv = log(eps/abs(g0 - totalDesiredGain))/log(1 - DezipperRate)
      = log(eps)/log(1-DezipperRate) 
         - log(abs(g0-totalDesiredGain))/log(1-DezipperRate)

where g0 is the starting gain value at the beginning of the routine.  All of the logs are constants except for log(abs(g0-totalDesiredGain)), so the computation of nConv is not too bad.  log, however, is not cheap, so it's hard to tell if we would get a net win or not since the processing in the loops is fairly simple.  A quick and dirty log approximation might be good enough.

[Wei James (wistoch)]

(In reply to comment #3)
> A side note on a different optimization.  It's not necessary to implement this instead of the proposed patch.
> 
> With the current patch, we still do (slightly) extra work if needDezipper is true.  During the loop, we may get close enough to the target that we don't need to dezipper anymore.
> 
> Since the dezippering is just an exponential filter, we can figure exactly how many steps will be needed to get with epsilon of the target value.  So compute this value, nConv, and have two loops:
> 
> N = min(nConv, framesToProcess);
> for (k = 0; k < N; ++k) {
>    <loop with dezippering>
> }
> 
> for (k < framesToProcess; ++k) {
>    <loop without dezippering>
> }
> 
> Assuming I did the math correctly, 
> 
> nConv = log(eps/abs(g0 - totalDesiredGain))/log(1 - DezipperRate)
>       = log(eps)/log(1-DezipperRate) 
>          - log(abs(g0-totalDesiredGain))/log(1-DezipperRate)
> 
> where g0 is the starting gain value at the beginning of the routine.  All of the logs are constants except for log(abs(g0-totalDesiredGain)), so the computation of nConv is not too bad.  log, however, is not cheap, so it's hard to tell if we would get a net win or not since the processing in the loops is fairly simple.  A quick and dirty log approximation might be good enough.

Raymond, thanks for the review. I think your optimization solution is more graceful. I want to try it and test the performance to decide whether or not to use it. 

for the performance improvement of my changes, on my Ubuntu 32 bit/Core i7 dev machine, it will get about 2.5~2.9x speed up in different code pathes(sumToBus as true or false, stereo or not etc).

[Chris Rogers]

(In reply to comment #3)

I'd recommend not going so far as doing this optimization.  This code is very soon going to get a lot more complicated when we
add upmixing paths for 5.1, etc., so I'd prefer to avoid this extra complexity.

> A side note on a different optimization.  It's not necessary to implement this instead of the proposed patch.
> With the current patch, we still do (slightly) extra work if needDezipper is true.  During the loop, we may get close enough to the target that we don't need to dezipper anymore.
> Since the dezippering is just an exponential filter, we can figure exactly how many steps will be needed to get with epsilon of the target value.  So compute this value, nConv, and have two loops:
> N = min(nConv, framesToProcess);
> for (k = 0; k < N; ++k) {
>    <loop with dezippering>
> }
> for (k < framesToProcess; ++k) {
>    <loop without dezippering>
> }
> Assuming I did the math correctly, 
> nConv = log(eps/abs(g0 - totalDesiredGain))/log(1 - DezipperRate)
>       = log(eps)/log(1-DezipperRate) 
>          - log(abs(g0-totalDesiredGain))/log(1-DezipperRate)
> where g0 is the starting gain value at the beginning of the routine.  All of the logs are constants except for log(abs(g0-totalDesiredGain)), so the computation of nConv is not too bad.  log, however, is not cheap, so it's hard to tell if we would get a net win or not since the processing in the loops is fairly simple.  A quick and dirty log approximation might be good enough.

[Chris Rogers]

It would be nice to be able to use our friends vsmul() and vadd() in VectorMath.h when the gain has converged on the target.  At the very least it would be good to be able to avoid the clamping of denormals in the inner loop there.  We can consider a strategy where gain is forced to zero if it ever turns out to be less than say -200dB

[Raymond Toy]

(In reply to comment #5)
> (In reply to comment #3)
> 
> I'd recommend not going so far as doing this optimization.  This code is very soon going to get a lot more complicated when we
> add upmixing paths for 5.1, etc., so I'd prefer to avoid this extra complexity.

Uh, ok.  I don't see that this adds any additional code except for the simple computation of nConv.  There will be two loops in any case.  Unless you're saying that we should not include the proposed patch at all.

> 
> > A side note on a different optimization.  It's not necessary to implement this instead of the proposed patch.
> > With the current patch, we still do (slightly) extra work if needDezipper is true.  During the loop, we may get close enough to the target that we don't need to dezipper anymore.
> > Since the dezippering is just an exponential filter, we can figure exactly how many steps will be needed to get with epsilon of the target value.  So compute this value, nConv, and have two loops:
> > N = min(nConv, framesToProcess);
> > for (k = 0; k < N; ++k) {
> >    <loop with dezippering>
> > }
> > for (k < framesToProcess; ++k) {
> >    <loop without dezippering>
> > }
> > Assuming I did the math correctly, 
> > nConv = log(eps/abs(g0 - totalDesiredGain))/log(1 - DezipperRate)
> >       = log(eps)/log(1-DezipperRate) 
> >          - log(abs(g0-totalDesiredGain))/log(1-DezipperRate)
> > where g0 is the starting gain value at the beginning of the routine.  All of the logs are constants except for log(abs(g0-totalDesiredGain)), so the computation of nConv is not too bad.  log, however, is not cheap, so it's hard to tell if we would get a net win or not since the processing in the loops is fairly simple.  A quick and dirty log approximation might be good enough.

[Wei James (wistoch)]

Created attachment 118767 [details]
Patch updated

[Wei James (wistoch)]

(In reply to comment #3)
> A side note on a different optimization.  It's not necessary to implement this instead of the proposed patch.
> 
> With the current patch, we still do (slightly) extra work if needDezipper is true.  During the loop, we may get close enough to the target that we don't need to dezipper anymore.
> 
> Since the dezippering is just an exponential filter, we can figure exactly how many steps will be needed to get with epsilon of the target value.  So compute this value, nConv, and have two loops:
> 
> N = min(nConv, framesToProcess);
> for (k = 0; k < N; ++k) {
>    <loop with dezippering>
> }
> 
> for (k < framesToProcess; ++k) {
>    <loop without dezippering>
> }
> 
> Assuming I did the math correctly, 
> 
> nConv = log(eps/abs(g0 - totalDesiredGain))/log(1 - DezipperRate)
>       = log(eps)/log(1-DezipperRate) 
>          - log(abs(g0-totalDesiredGain))/log(1-DezipperRate)
> 
> where g0 is the starting gain value at the beginning of the routine.  All of the logs are constants except for log(abs(g0-totalDesiredGain)), so the computation of nConv is not too bad.  log, however, is not cheap, so it's hard to tell if we would get a net win or not since the processing in the loops is fairly simple.  A quick and dirty log approximation might be good enough.

Raymond, I updated the patch to apply your solution. 

The test showed that it will get 75% performance gain at most (when nConv == 0). For nConv = framesToProcess (all frames need de-zippering), it will not impact the overall performance. 

In the previous comment, I said 2.x performance boost, I checked the test code and found the memory cache impact the test result. So I made some change to avoid memory cache in my test and it also shows the same result. 

BTW, in current implementation, the epsilon is a pre-defined value. Should it be a pre-defined value or a percentage of the difference between gain and targetGain? If it is the latter, no log needed.

[Wei James (wistoch)]

(In reply to comment #6)
> It would be nice to be able to use our friends vsmul() and vadd() in VectorMath.h when the gain has converged on the target.  At the very least it would be good to be able to avoid the clamping of denormals in the inner loop there.  We can consider a strategy where gain is forced to zero if it ever turns out to be less than say -200dB

yes, it can be optimized with SSE as said in the FIXME. 

I saw four FIXMEs in this function and want to do it step by step. This patch mainly focus on the fast path for converged gain. 

Will focus on SSE optimization later. thanks

[Wei James (wistoch)]

(In reply to comment #5)
> (In reply to comment #3)
> 
> I'd recommend not going so far as doing this optimization.  This code is very soon going to get a lot more complicated when we
> add upmixing paths for 5.1, etc., so I'd prefer to avoid this extra complexity.
> 
> > A side note on a different optimization.  It's not necessary to implement this instead of the proposed patch.
> > With the current patch, we still do (slightly) extra work if needDezipper is true.  During the loop, we may get close enough to the target that we don't need to dezipper anymore.
> > Since the dezippering is just an exponential filter, we can figure exactly how many steps will be needed to get with epsilon of the target value.  So compute this value, nConv, and have two loops:
> > N = min(nConv, framesToProcess);
> > for (k = 0; k < N; ++k) {
> >    <loop with dezippering>
> > }
> > for (k < framesToProcess; ++k) {
> >    <loop without dezippering>
> > }
> > Assuming I did the math correctly, 
> > nConv = log(eps/abs(g0 - totalDesiredGain))/log(1 - DezipperRate)
> >       = log(eps)/log(1-DezipperRate) 
> >          - log(abs(g0-totalDesiredGain))/log(1-DezipperRate)
> > where g0 is the starting gain value at the beginning of the routine.  All of the logs are constants except for log(abs(g0-totalDesiredGain)), so the computation of nConv is not too bad.  log, however, is not cheap, so it's hard to tell if we would get a net win or not since the processing in the loops is fairly simple.  A quick and dirty log approximation might be good enough.

with some MACROs to avoid code duplication, I think the new patch with Raymond's solution makes the code clearer and readable. It will be easier to add more pathes with this solution. please help to review. thanks

[Wei James (wistoch)]

BTW, I met an error when using webkit-patch to upload patch. It seems that I have no permission to change the bug owner to me, so it exits without the patch uploaded. 

could anyone assign this bug to me so I can use webkit-patch to upload patch later? thanks

[Raymond Toy]

(In reply to comment #12)
> BTW, I met an error when using webkit-patch to upload patch. It seems that I have no permission to change the bug owner to me, so it exits without the patch uploaded. 
> 
> could anyone assign this bug to me so I can use webkit-patch to upload patch later? thanks

Seems like this might be a bug in webkit-patch upload.  It seems to me that I could upload patches even though I did not have permission to change the bug ownership.  In any case, webkit-patch should have printed some instructions on how to get permission; you basically have to ask someone via email or irc to get permission.  

I've assigned the bug to you.

[Raymond Toy]

Comment on attachment 118767 [details]
Patch updated

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

Comments below.  I think it would be fine if you change epsilon to mean that abs(1-gain/totalDesiredGain)) < epsilon.  There's a problem here, though if totalDesiredGain = 0.  Need to solve that in some way.

> Source/WebCore/ChangeLog:9
> +        It can get about 75% perfromance gain at most when all frames don't need

"perfromance" -> "performance".

75% is pretty impressive!

> Source/WebCore/platform/audio/AudioBus.cpp:263
> +    const float epsilon = 0.001f; // 0.001 seems small enough, but may need tweaking.

Need better description of epsilon.  Something like

If the gain is within epsilon of totalDesiredGain, we can skip dezippering.  This value may need tweaking.

> Source/WebCore/platform/audio/AudioBus.cpp:264
> +    int nConv = framesToProcess; // From which frame we can skip de-zippering. 

I would change "we can skip de-zippering" to "we can skip de-zippering because we are now close enough to the target gain."

> Source/WebCore/platform/audio/AudioBus.cpp:271
> +        nConv = 1378.094 + log(gainDiff) / 0.0050125;

This seems not right.  If someone changes DezipperRate, the constants here will be wrong.  I do not know, but I'm pretty sure most compilers are smart enough to constant-fold everything.

Also, did you check the math?  A comment on where the formula comes from will be useful to future maintainers.  Something like

dezippering is an exponential filter:  g(n+1) = (G - g(n))*DezipperRate, where G = totalDesiredGain and g(0) = lastMixGain.  This can be solved analytically to give g(n) = <correct expression>.  

We can skip dezippering if g(n) is within epsilon of G.  This happens when n = <formula above>.

> Source/WebCore/platform/audio/AudioBus.cpp:279
> +// Slowly change gain to desired gain.

Although I tend to use macros a fair amount for things like this, in this case, it seems that these macros hurt readability.  At the very least it would help to move them outside the function because this big mass of macros makes it hard to see where the real code is.

> Source/WebCore/platform/audio/AudioBus.cpp:336
> +            for (; k < framesToProcess; k++)  \

Safer to do

for (...) {
  OP
}

in case OP ever becomes more than one expression.

[Wei James (wistoch)]

Comment on attachment 118767 [details]
Patch updated

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

>> Source/WebCore/platform/audio/AudioBus.cpp:263
>> +    const float epsilon = 0.001f; // 0.001 seems small enough, but may need tweaking.
> 
> Need better description of epsilon.  Something like
> 
> If the gain is within epsilon of totalDesiredGain, we can skip dezippering.  This value may need tweaking.

got it. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:271
>> +        nConv = 1378.094 + log(gainDiff) / 0.0050125;
> 
> This seems not right.  If someone changes DezipperRate, the constants here will be wrong.  I do not know, but I'm pretty sure most compilers are smart enough to constant-fold everything.
> 
> Also, did you check the math?  A comment on where the formula comes from will be useful to future maintainers.  Something like
> 
> dezippering is an exponential filter:  g(n+1) = (G - g(n))*DezipperRate, where G = totalDesiredGain and g(0) = lastMixGain.  This can be solved analytically to give g(n) = <correct expression>.  
> 
> We can skip dezippering if g(n) is within epsilon of G.  This happens when n = <formula above>.

I have double checked the math and it is correct. I will add the comment on it.

>> Source/WebCore/platform/audio/AudioBus.cpp:279
>> +// Slowly change gain to desired gain.
> 
> Although I tend to use macros a fair amount for things like this, in this case, it seems that these macros hurt readability.  At the very least it would help to move them outside the function because this big mass of macros makes it hard to see where the real code is.

I will try to make the code more readable. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:336
>> +            for (; k < framesToProcess; k++)  \
> 
> Safer to do
> 
> for (...) {
>   OP
> }
> 
> in case OP ever becomes more than one expression.

I have add {} in the code but the check-webkit-style reports it is incorrect because it took OP as one line statement for for loop. 
I also think adding {} more reasonable. thanks

[Wei James (wistoch)]

Created attachment 118966 [details]
Patch

[WebKit Review Bot]

Attachment 118966 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/AudioBus.cpp:249:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wei James (wistoch)]

(In reply to comment #14)
> (From update of attachment 118767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118767&action=review
> 
> Comments below.  I think it would be fine if you change epsilon to mean that abs(1-gain/totalDesiredGain)) < epsilon.  There's a problem here, though if totalDesiredGain = 0.  Need to solve that in some way.
> 

Raymond, I found it will add more complexity to consider epsilon in this way(we will need to consider two situations: one is the absolute difference(for the starting gain is already close to the target gain) and the second one is the relative difference) so I still use the original epsilon. 

also I changed the the positions to hold MACRO to make it more readable. could you help to review it? thanks

[Chris Rogers]

Comment on attachment 118966 [details]
Patch

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

James, this looks really good.  I like how your macros work, and think it will scale well when we add additional upmixing for quad, 5.1, etc.

> Source/WebCore/platform/audio/AudioBus.cpp:287
> +        nConv = 0;

I'm not very fond of the name "nConv"

How about "framesToConverge"?

> Source/WebCore/platform/audio/AudioBus.cpp:290
> +        // De-zippering is an exponential filter: g(n+1) = g(n) + (g - g(n))*DezipperRate, where g = totalDesiredGain. 

I'm a bit afraid about this codepath.  I think we should defer it to a subsequent patch. We don't have proper layout testing yet to verify this, and I'd rather keep the changes smaller and more manageable.
So, I'd recommend taking out this else branch.  It seems like the rest of your code works fine without it.

in any case, samples are processed usually in batches of 128 at a time, so this path will only save us less than 128 samples of converging which isn't nearly as much as the rest of this change is giving us.

> Source/WebCore/platform/audio/AudioBus.cpp:313
>              }

Can we move this macro and the following ones to the same area as PROCESS_WITH_GAIN macro (after line 249)

So the macro definitions are defined separately and outside of this function

[Wei James (wistoch)]

Comment on attachment 118966 [details]
Patch

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

>> Source/WebCore/platform/audio/AudioBus.cpp:287
>> +        nConv = 0;
> 
> I'm not very fond of the name "nConv"
> 
> How about "framesToConverge"?

got it. I will change it. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:290
>> +        // De-zippering is an exponential filter: g(n+1) = g(n) + (g - g(n))*DezipperRate, where g = totalDesiredGain. 
> 
> I'm a bit afraid about this codepath.  I think we should defer it to a subsequent patch. We don't have proper layout testing yet to verify this, and I'd rather keep the changes smaller and more manageable.
> So, I'd recommend taking out this else branch.  It seems like the rest of your code works fine without it.
> 
> in any case, samples are processed usually in batches of 128 at a time, so this path will only save us less than 128 samples of converging which isn't nearly as much as the rest of this change is giving us.

for the case of DezipperRate = 0.005 and epsilon = 0.001, it will skip de-zippering after 918 frames. 
so I agree with you that if usually we process 128 frames at a time, it will not enter into this code path. 

I will remove it for further improvement. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:313
>>              }
> 
> Can we move this macro and the following ones to the same area as PROCESS_WITH_GAIN macro (after line 249)
> 
> So the macro definitions are defined separately and outside of this function

got it. I will do it. thanks

[Wei James (wistoch)]

Created attachment 119119 [details]
Patch

[WebKit Review Bot]

Attachment 119119 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/AudioBus.cpp:249:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymond Toy]

Comment on attachment 119119 [details]
Patch

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

>> Source/WebCore/platform/audio/AudioBus.cpp:249
>> +            }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

This needs to be fixed.

> Source/WebCore/platform/audio/AudioBus.cpp:337
> +    int framesToConverge = (gainDiff < epsilon) ? 0 : framesToProcess; 

I don't think framesToConverge has the right meaning anymore.  Maybe framesToDezipper?  If you change it here, don't forget to change it in the macro above.

[Wei James (wistoch)]

Created attachment 119121 [details]
Patch

[Wei James (wistoch)]

(In reply to comment #23)
> (From update of attachment 119119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119119&action=review
> 
> >> Source/WebCore/platform/audio/AudioBus.cpp:249
> >> +            }
> > 
> > One line control clauses should not use braces.  [whitespace/braces] [4]
> 
> This needs to be fixed.
> 
> > Source/WebCore/platform/audio/AudioBus.cpp:337
> > +    int framesToConverge = (gainDiff < epsilon) ? 0 : framesToProcess; 
> 
> I don't think framesToConverge has the right meaning anymore.  Maybe framesToDezipper?  If you change it here, don't forget to change it in the macro above.

agree. I changed it to framesToDezipper. 

and also I added an empty MACRO to avoid webkit style check. thanks

[Chris Rogers]

Comment on attachment 119119 [details]
Patch

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

> Source/WebCore/platform/audio/AudioBus.cpp:236
> +                gain += (totalDesiredGain - gain) * DezipperRate; \

Not sure about the indentation here.  You should check other macro definitions in WebKit and see if we should use four-space indent.

>> Source/WebCore/platform/audio/AudioBus.cpp:337
>> +    int framesToConverge = (gainDiff < epsilon) ? 0 : framesToProcess; 
> 
> I don't think framesToConverge has the right meaning anymore.  Maybe framesToDezipper?  If you change it here, don't forget to change it in the macro above.

Seems the same to me - either way is fine.

> Source/WebCore/platform/audio/AudioBus.cpp:361
> +            PROCESS_WITH_GAIN(STEREO_NO_SUM)

I wouldn't bother implementing this now, but maybe add FIXME here (same applies to other cases below)

// FIXME: if (framesToConverge == 0) and DenormalDisabler::flushDenormalFloatToZero() is a NOP (gcc vs. Visual Studio)
// then we can further optimize using vsmul()

[Wei James (wistoch)]

Comment on attachment 119119 [details]
Patch

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

>> Source/WebCore/platform/audio/AudioBus.cpp:236
>> +                gain += (totalDesiredGain - gain) * DezipperRate; \
> 
> Not sure about the indentation here.  You should check other macro definitions in WebKit and see if we should use four-space indent.

I checked html/parser/HTMLTokenizer.cpp and it uses four-space indent, I will change my patch to follow it. thanks for your guiding.

>> Source/WebCore/platform/audio/AudioBus.cpp:361
>> +            PROCESS_WITH_GAIN(STEREO_NO_SUM)
> 
> I wouldn't bother implementing this now, but maybe add FIXME here (same applies to other cases below)
> 
> // FIXME: if (framesToConverge == 0) and DenormalDisabler::flushDenormalFloatToZero() is a NOP (gcc vs. Visual Studio)
> // then we can further optimize using vsmul()

ok. I will add it. thanks

[Wei James (wistoch)]

Created attachment 119129 [details]
Patch

[Wei James (wistoch)]

patch updated according to your comments. could you help to review? thanks

[Raymond Toy]

Comment on attachment 119129 [details]
Patch

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

Just one small nit that you don't have to fix.  Otherwise, looks good.

> Source/WebCore/platform/audio/AudioBus.cpp:243
> +#define SKIP_DEZIPPER  

Not sure I like this dummy #define to fix style issue.  Might be better to just remove this and the braces from PROCESS_WITH_GAIN.

[Chris Rogers]

(In reply to comment #30)
> (From update of attachment 119129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119129&action=review
> 
> Just one small nit that you don't have to fix.  Otherwise, looks good.
> 
> > Source/WebCore/platform/audio/AudioBus.cpp:243
> > +#define SKIP_DEZIPPER  
> 
> Not sure I like this dummy #define to fix style issue.  Might be better to just remove this and the braces from PROCESS_WITH_GAIN.

I'm not sure he can just remove the braces.  The commit queue needs to pass style

[Raymond Toy]

(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 119129 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=119129&action=review
> > 
> > Just one small nit that you don't have to fix.  Otherwise, looks good.
> > 
> > > Source/WebCore/platform/audio/AudioBus.cpp:243
> > > +#define SKIP_DEZIPPER  
> > 
> > Not sure I like this dummy #define to fix style issue.  Might be better to just remove this and the braces from PROCESS_WITH_GAIN.
> 
> I'm not sure he can just remove the braces.  The commit queue needs to pass style

In the second patch, there were no braces around OP, and the style bot said it was ok.

[Chris Rogers]

Comment on attachment 119129 [details]
Patch

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

> Source/WebCore/platform/audio/AudioBus.cpp:246
> +    for (k = 0; k < framesToDezipper; k++) { \

nit: WebKit style is ++k and not k++

please fix here and related places

> Source/WebCore/platform/audio/AudioBus.cpp:256
> +// Stereo with summing

nit: WebKit style is to add comments only where they're instructive and avoid obvious comments, but in this case the comment is a bit redundant with the macro name which itself is quite clear.

please remove comment here and in similar redundant places

> Source/WebCore/platform/audio/AudioBus.cpp:337
> +    // This value may need tweaking.

nit: may want to prefix this comment with FIXME:

// FIXME: this value may need tweaking.

> Source/WebCore/platform/audio/AudioBus.cpp:341
> +    // From which frame we can skip de-zippering because we are now close enough to the target gain.

This comment seems a little funny.  I would word this as:

// Number of frames to de-zipper before we are close enough to the target gain.

> Source/WebCore/platform/audio/AudioBus.cpp:361
> +        if ((this == &sourceBus) && (*lastMixGain == targetGain) && (targetGain == 1.0))

I think the extra layer of parentheses are not necessary

> Source/WebCore/platform/audio/AudioBus.cpp:366
> +            // FIXME: if (framesToDezipper == 0) and DenormalDisabler::flushDenormalFloatToZero() is a NOP (gcc vs. Visual Studio) 

thanks for adding the comment.  But now I see it looks a bit funny duplicated three times.

I'd recommend moving this comment to just above line 364 and have it apply to all three cases, and change the wording slightly:

// then we can further optimize the PROCESS_WITH_GAIN codepaths below using vsmul().

[Wei James (wistoch)]

Comment on attachment 119129 [details]
Patch

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

>>>> Source/WebCore/platform/audio/AudioBus.cpp:243
>>>> +#define SKIP_DEZIPPER  
>>> 
>>> Not sure I like this dummy #define to fix style issue.  Might be better to just remove this and the braces from PROCESS_WITH_GAIN.
>> 
>> I'm not sure he can just remove the braces.  The commit queue needs to pass style
> 
> In the second patch, there were no braces around OP, and the style bot said it was ok.

ok. I will remove both the macro and braces. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:246
>> +    for (k = 0; k < framesToDezipper; k++) { \
> 
> nit: WebKit style is ++k and not k++
> 
> please fix here and related places

got it. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:256
>> +// Stereo with summing
> 
> nit: WebKit style is to add comments only where they're instructive and avoid obvious comments, but in this case the comment is a bit redundant with the macro name which itself is quite clear.
> 
> please remove comment here and in similar redundant places

I will remove them. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:337
>> +    // This value may need tweaking.
> 
> nit: may want to prefix this comment with FIXME:
> 
> // FIXME: this value may need tweaking.

will do so. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:341
>> +    // From which frame we can skip de-zippering because we are now close enough to the target gain.
> 
> This comment seems a little funny.  I would word this as:
> 
> // Number of frames to de-zipper before we are close enough to the target gain.

thanks for your revise. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:361
>> +        if ((this == &sourceBus) && (*lastMixGain == targetGain) && (targetGain == 1.0))
> 
> I think the extra layer of parentheses are not necessary

got it. thanks

>> Source/WebCore/platform/audio/AudioBus.cpp:366
>> +            // FIXME: if (framesToDezipper == 0) and DenormalDisabler::flushDenormalFloatToZero() is a NOP (gcc vs. Visual Studio) 
> 
> thanks for adding the comment.  But now I see it looks a bit funny duplicated three times.
> 
> I'd recommend moving this comment to just above line 364 and have it apply to all three cases, and change the wording slightly:
> 
> // then we can further optimize the PROCESS_WITH_GAIN codepaths below using vsmul().

will follow your instruction. thanks

[Wei James (wistoch)]

Created attachment 119341 [details]
Patch

[Chris Rogers]

Looks good.  James, thanks for doing this!  It's a good optimization and also helps cleanup the code for other mixing paths...

[Kenneth Russell]

Comment on attachment 119341 [details]
Patch

rs=me

[Wei James (wistoch)]

(In reply to comment #36)
> Looks good.  James, thanks for doing this!  It's a good optimization and also helps cleanup the code for other mixing paths...

Roger, Raymond, thanks a lot for your help to refine this patch. I really learned a lot from this process. thanks 

Ken, thanks a lot for your support too. thanks

[WebKit Review Bot]

Comment on attachment 119341 [details]
Patch

Clearing flags on attachment: 119341

Committed r102888: <http://trac.webkit.org/changeset/102888>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.