Bug 77509

Summary: Enable IPP for Biquad filter
Product: WebKit Reporter: Xingnan Wang <xingnan.wang>
Component: Web AudioAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, dglazkov, rtoy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 75522    
Bug Blocks:    
Attachments:
Description Flags
Patch
webkit.review.bot: commit-queue-
Patch
none
Patch
none
Patch
webkit.review.bot: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
crogers: review+, webkit.review.bot: commit-queue-
Patch
none
output without ipp
none
output with ipp
none
Patch none

Description Xingnan Wang 2012-01-31 21:27:34 PST
Use the  IIR filter in IPP library to implement the Biquad filter.
Comment 1 Xingnan Wang 2012-01-31 21:36:36 PST
Created attachment 124881 [details]
Patch

From my test in linux, the performance increased ~27% for the processing of biquad.
Comment 2 WebKit Review Bot 2012-01-31 22:14:40 PST
Comment on attachment 124881 [details]
Patch

Attachment 124881 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11386329
Comment 3 Xingnan Wang 2012-01-31 22:34:00 PST
Created attachment 124886 [details]
Patch
Comment 4 Raymond Toy 2012-02-01 14:00:18 PST
Comment on attachment 124886 [details]
Patch

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

> Source/WebCore/platform/audio/Biquad.cpp:58
> +    m_dlyLine = 0;

Can we name this m_delayLine?

> Source/WebCore/platform/audio/Biquad.cpp:205
> +#if USE(WEBAUDIO_IPP)

Maybe combine this with #endif above and use

#elif USE(WEBAUDIO_IPP)

> Source/WebCore/platform/audio/Biquad.cpp:241
>  }

Rather than sprinkling this ippBiquadInit in each filter, I would be better to refactor each set<filter> routine to always call setNormalizedCoefficients instead of assigning the coefficients directly.  Then setNormalizedCoefficients can have the ippBiquadInit call in just the one place.

> Source/WebCore/platform/audio/Biquad.cpp:485
> +{

If the code is refactored to use setNormalizedCoefficients everywhere, then this function can probably be removed and placed directly in setNormalizedCoefficients.

> Source/WebCore/platform/audio/Biquad.h:111
> +void ippBiquadInit(const double b0, const double b1, const double b2, const double a0, const double a1, const double a2);

This should be indented.  (But maybe it can be removed completely if setNormalizedCoefficients is used.)
Comment 5 Raymond Toy 2012-02-01 14:03:40 PST
(In reply to comment #1)
> Created an attachment (id=124881) [details]
> Patch
> 
> From my test in linux, the performance increased ~27% for the processing of biquad.

To clarify, this gives 27% increase over the original code.  And the other version in bug 75528 (https://bugs.webkit.org/show_bug.cgi?id=75528) that uses SSE2 gives about 20%?
Comment 6 Chris Rogers 2012-02-01 14:28:40 PST
Xingnan, I agree with Ray's comments about re-factoring the call to ippBiquadInit.  But, first it will probably be a lot easier for you if Ray's other patch lands first:
https://bugs.webkit.org/show_bug.cgi?id=71413

This should land very soon!
Comment 7 Xingnan Wang 2012-02-01 17:32:28 PST
(In reply to comment #5)
> (In reply to comment #1)
> > Created an attachment (id=124881) [details] [details]
> > Patch
> > 
> > From my test in linux, the performance increased ~27% for the processing of biquad.
> 
> To clarify, this gives 27% increase over the original code.  And the other version in bug 75528 (https://bugs.webkit.org/show_bug.cgi?id=75528) that uses SSE2 gives about 20%?

These two patches are different. I just optimized the multiply-add in biquad process in bug 75528, but in bug 77509 I used the whole IIR filter in IPP to implement the biquad, in which I guess highly optimized method may be used so it achieves a better performance, not being sure because I could not touch the IPP source code either.

These two patches also are used in different scenarios. If IPP is enable, we could use IIR filter in IPP. Or we could use the optimization in bug 75528 for common case as long as the platform supports SSE2.
Comment 8 Xingnan Wang 2012-02-01 17:35:39 PST
(In reply to comment #6)
> Xingnan, I agree with Ray's comments about re-factoring the call to ippBiquadInit.  But, first it will probably be a lot easier for you if Ray's other patch lands first:
> https://bugs.webkit.org/show_bug.cgi?id=71413
> 
> This should land very soon!

Chris, That`s OK. I will update the patch as Ray`s comments after bug 71413 lands.
Comment 9 Xingnan Wang 2012-02-01 17:37:29 PST
(In reply to comment #6)
> Xingnan, I agree with Ray's comments about re-factoring the call to ippBiquadInit.  But, first it will probably be a lot easier for you if Ray's other patch lands first:
> https://bugs.webkit.org/show_bug.cgi?id=71413
> 
> This should land very soon!

Also very sorry for misusing your name before, I should call you Chris, right?
Comment 10 Chris Rogers 2012-02-01 17:59:55 PST
(In reply to comment #9)
> (In reply to comment #6)
> > Xingnan, I agree with Ray's comments about re-factoring the call to ippBiquadInit.  But, first it will probably be a lot easier for you if Ray's other patch lands first:
> > https://bugs.webkit.org/show_bug.cgi?id=71413
> > 
> > This should land very soon!
> 
> Also very sorry for misusing your name before, I should call you Chris, right?

No problem, yes Chris is correct :)
Comment 11 Raymond Toy 2012-02-02 09:24:49 PST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #1)
> > > Created an attachment (id=124881) [details] [details] [details]
> > > Patch
> > > 
> > > From my test in linux, the performance increased ~27% for the processing of biquad.
> > 
> > To clarify, this gives 27% increase over the original code.  And the other version in bug 75528 (https://bugs.webkit.org/show_bug.cgi?id=75528) that uses SSE2 gives about 20%?
> 
> These two patches are different. I just optimized the multiply-add in biquad process in bug 75528, but in bug 77509 I used the whole IIR filter in IPP to implement the biquad, in which I guess highly optimized method may be used so it achieves a better performance, not being sure because I could not touch the IPP source code either.
> 
> These two patches also are used in different scenarios. If IPP is enable, we could use IIR filter in IPP. Or we could use the optimization in bug 75528 for common case as long as the platform supports SSE2.

Thanks for clarifying this.  I understand speed difference now.
Comment 12 Raymond Toy 2012-02-02 09:28:24 PST
(In reply to comment #8)
> (In reply to comment #6)
> > Xingnan, I agree with Ray's comments about re-factoring the call to ippBiquadInit.  But, first it will probably be a lot easier for you if Ray's other patch lands first:
> > https://bugs.webkit.org/show_bug.cgi?id=71413
> > 
> > This should land very soon!
> 
> Chris, That`s OK. I will update the patch as Ray`s comments after bug 71413 lands.

My apologies for taking so long on getting 71413 done!  The test exposed some issues with sample timing, making it difficult to compare actual and expected results in Javascript.  Fixing the timing issue caused other issues which took sometime to get them all worked out, but I think we have correct sample-timing now.
Comment 13 Raymond Toy 2012-02-02 09:30:50 PST
Comment on attachment 124886 [details]
Patch

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

> Source/WebCore/platform/audio/Biquad.cpp:57
> +#if USE(WEBAUDIO_IPP)

One quick question:  Where is WEBAUDIO_IPP defined/enabled?  Is this in the gyp file that is part of patch for the IPP for FFTframe?
Comment 14 Xingnan Wang 2012-02-02 17:41:22 PST
(In reply to comment #13)
> (From update of attachment 124886 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124886&action=review
> 
> > Source/WebCore/platform/audio/Biquad.cpp:57
> > +#if USE(WEBAUDIO_IPP)
> 
> One quick question:  Where is WEBAUDIO_IPP defined/enabled?  Is this in the gyp file that is part of patch for the IPP for FFTframe?

WEBAUDIO_IPP should be enable in gyp if we want to enable IPP, by default it `s not enabled and the FFMPEG is used.

WEBAUDIO_IPP is included in patch for IPP for FFTFrame, after which this patch should be.

Set the dependence on bug 75522.
Comment 15 Xingnan Wang 2012-02-06 19:05:04 PST
Created attachment 125752 [details]
Patch
Comment 16 Xingnan Wang 2012-02-07 00:08:39 PST
Created attachment 125775 [details]
Patch
Comment 17 Xingnan Wang 2012-02-07 00:30:43 PST
(In reply to comment #16)
> Created an attachment (id=125775) [details]
> Patch

Hi Ray and Chris, patch is updated as comments but I met an issue when running the biquad-*.html layout tests.

For my new patch, I could pass the every biquad-*.html test by running them separately but not when running them together. 
Like run-webkit-tests webaudio and run-webkit-tests biquad*, it sometimes failed.

I tried to adjust the value of 'timeStep' in biquad-testing.js as the comments, but it did not work. I could not figure out the problem in patch or in the test scripts, could you provide some help on this?  Thanks a lot.

Some details:

xingnan@xingnan-pc:~/Workspace/chromium/src/third_party/WebKit$ ./Tools/Scripts/run-webkit-tests webaudio  webaudio/biquad-lowshelf.html -> unexpected text diff mismatch
                                                    
Retrying 1 unexpected failure(s) ...

33 tests ran as expected, 1 didn't:                 


Unexpected flakiness: text diff mismatch (1)
  webaudio/biquad-lowshelf.html = TEXT PASS

Tests Biquad lowshelf filter.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

PASS

FAIL Rendered output did not have has 831 infinities or NaNs.
PASS
FAIL Lowshelf filter response is correct.
PASS incorrect.  Max err = Infinity at 0.  Threshold = 5.9e-8
FAIL Test signal was not correctly filtered.
PASS successfullyParsed is true

TEST COMPLETE

xingnan@xingnan-pc:~/Workspace/chromium/src/third_party/WebKit$ ./Tools/Scripts/run-webkit-tests webaudio/biquad-*
All 8 tests ran as expected.                       

xingnan@xingnan-pc:~/Workspace/chromium/src/third_party/WebKit$ ./Tools/Scripts/run-webkit-tests webaudio/biquad-*
  webaudio/biquad-notch.html -> unexpected text diff mismatch
                                                   
Retrying 1 unexpected failure(s) ...

7 tests ran as expected, 1 didn't:                  


Unexpected flakiness: text diff mismatch (1)
  webaudio/biquad-notch.html = TEXT PASS


Exception in thread QueueFeederThread (most likely raised during interpreter shutdown):Exception in thread QueueFeederThread (most likely raised during interpreter shutdown):
xingnan@xingnan-pc:~/Workspace/chromium/src/third_party/WebKit$

Tests Biquad notch filter.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

PASS Rendered output did not have infinities or NaNs.
PASS
FAIL Notch filter response is correct.
PASS incorrect.  Max err = 1 at 8820.  Threshold = 5.9e-8
FAIL Test signal was not correctly filtered.
PASS successfullyParsed is true

TEST COMPLETE
Comment 18 WebKit Review Bot 2012-02-07 00:46:36 PST
Comment on attachment 125775 [details]
Patch

Attachment 125775 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11439177

New failing tests:
webaudio/biquad-lowshelf.html
webaudio/biquad-notch.html
webaudio/biquad-highshelf.html
webaudio/biquad-peaking.html
webaudio/biquad-bandpass.html
webaudio/biquad-allpass.html
Comment 19 Raymond Toy 2012-02-07 09:08:18 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=125775) [details] [details]
> > Patch
> 
> Hi Ray and Chris, patch is updated as comments but I met an issue when running the biquad-*.html layout tests.
> 
> For my new patch, I could pass the every biquad-*.html test by running them separately but not when running them together. 
> Like run-webkit-tests webaudio and run-webkit-tests biquad*, it sometimes failed.

I had this problem on Windows.  I think it was fixed when I recompiled DumpRenderTree.

But I will look into this some more on my system (but without IPP.)
> 
> I tried to adjust the value of 'timeStep' in biquad-testing.js as the comments, but it did not work. I could not figure out the problem in patch or in the test scripts, could you provide some help on this?  Thanks a lot.

What did you change timeStep to?  And in what way did it not work?

> 
> Some details:
> 
> xingnan@xingnan-pc:~/Workspace/chromium/src/third_party/WebKit$ ./Tools/Scripts/run-webkit-tests webaudio  webaudio/biquad-lowshelf.html -> unexpected text diff mismatch
> 
> Retrying 1 unexpected failure(s) ...
> 
> 33 tests ran as expected, 1 didn't:                 
> 
> 
> Unexpected flakiness: text diff mismatch (1)
>   webaudio/biquad-lowshelf.html = TEXT PASS
> 
> Tests Biquad lowshelf filter.
> 
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> PASS
> 
> FAIL Rendered output did not have has 831 infinities or NaNs.

Oops.  A typo got into the message and no one noticed.  Could you update your patch to fix this to read "output has 831 infinities..."?

> PASS
> FAIL Lowshelf filter response is correct.

Oops.  That should be "is not correct".
Comment 20 Raymond Toy 2012-02-07 10:01:44 PST
Comment on attachment 125775 [details]
Patch

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

This new patch looks much simpler than the previous one.  Thanks for applying this patch over the other one that added tests.

> Source/WebCore/ChangeLog:8
> +        Used IIR filter in IPP and imporved ~27% performance in linux.

"Used" -> "Use"
"and imporved..." -> "which improves performance in linux by ~27%"

> Source/WebCore/platform/audio/Biquad.cpp:56
> +

nit:  Remove extra blank line.

> Source/WebCore/platform/audio/Biquad.cpp:317
> +

Question:  Should ipp normalize the coefficients by dividing everything by a0 like we do for non-ipp?  Makes the actual coefficients consistent between implementations.  Perhaps this doesn't matter.

> Source/WebCore/platform/audio/Biquad.cpp:324
> +        return;

This breaks my expectations on what setNormalizedCoefficients should do and is, I think, incorrect.  In setLowShelfParams, there is the call setNormalizedCoefficients(1,0,0,1,0,0).  Since a0=1, nothing is done, including setting up the actual filter parameters.

I think 323-324 should be removed.  Or at least be sure you still set the filter parameters in this case.

This is probably the source of some of the test failures.

> Source/WebCore/platform/audio/Biquad.h:113
> +    Ipp8u* m_buffer;

nit:  Is there a more descriptive name for m_buffer?
Comment 21 Xingnan Wang 2012-02-07 19:00:47 PST
(In reply to comment #20)
> (From update of attachment 125775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125775&action=review
> 
> This new patch looks much simpler than the previous one.  Thanks for applying this patch over the other one that added tests.
> 
That`s OK.
> > Source/WebCore/ChangeLog:8
> > +        Used IIR filter in IPP and imporved ~27% performance in linux.
> 
> "Used" -> "Use"
> "and imporved..." -> "which improves performance in linux by ~27%"
> 
Ok.
> > Source/WebCore/platform/audio/Biquad.cpp:56
> > +
> 
> nit:  Remove extra blank line.
> 
OK.
> > Source/WebCore/platform/audio/Biquad.cpp:317
> > +
> 
> Question:  Should ipp normalize the coefficients by dividing everything by a0 like we do for non-ipp?  Makes the actual coefficients consistent between implementations.  Perhaps this doesn't matter.
> 
ippsIIRInit64f_BiQuad_32f normalizes the coefficients internally, so it does not matter.
> > Source/WebCore/platform/audio/Biquad.cpp:324
> > +        return;
> 
> This breaks my expectations on what setNormalizedCoefficients should do and is, I think, incorrect.  In setLowShelfParams, there is the call setNormalizedCoefficients(1,0,0,1,0,0).  Since a0=1, nothing is done, including setting up the actual filter parameters.
> 
> I think 323-324 should be removed.  Or at least be sure you still set the filter parameters in this case.
> 
Agree, it`s incorrect. Thanks for correcting me and I`ll remove them.
> This is probably the source of some of the test failures.
> 
But it seems it`s not the root cause of test failures. In ipp m_b0, m_b1 and others are not used while processing, and the tests still failed after removing 323-324.
> > Source/WebCore/platform/audio/Biquad.h:113
> > +    Ipp8u* m_buffer;
> 
> nit:  Is there a more descriptive name for m_buffer?
How about m_ippInternalBuffer?
Comment 22 Xingnan Wang 2012-02-07 19:23:53 PST
(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Created an attachment (id=125775) [details] [details] [details]
> > > Patch
> > 
> > Hi Ray and Chris, patch is updated as comments but I met an issue when running the biquad-*.html layout tests.
> > 
> > For my new patch, I could pass the every biquad-*.html test by running them separately but not when running them together. 
> > Like run-webkit-tests webaudio and run-webkit-tests biquad*, it sometimes failed.
> 
> I had this problem on Windows.  I think it was fixed when I recompiled DumpRenderTree.
> 
> But I will look into this some more on my system (but without IPP.)

I cleaned the out dir and recompiled the DumpRenderTree, the tests still failed.
> > 
> > I tried to adjust the value of 'timeStep' in biquad-testing.js as the comments, but it did not work. I could not figure out the problem in patch or in the test scripts, could you provide some help on this?  Thanks a lot.
> 
> What did you change timeStep to?  And in what way did it not work?
> 
As your comments, I tried enlarge the timeStep to the values like 0.2, 0.5 ,1, 10 and I even tried to set it to some small values, like 0.01. All the tryings could not make all the tests pass always while running the tests together. For every single value, the tests are not passed occasionally, and different tests failed by different values.

Most of the failed tests shows it has infinites or NaNs, some of them shows err values.

But every single test could passed absolutely. That`s what confuse me. 
> > 
> > Some details:
> > 
> > xingnan@xingnan-pc:~/Workspace/chromium/src/third_party/WebKit$ ./Tools/Scripts/run-webkit-tests webaudio  webaudio/biquad-lowshelf.html -> unexpected text diff mismatch
> > 
> > Retrying 1 unexpected failure(s) ...
> > 
> > 33 tests ran as expected, 1 didn't:                 
> > 
> > 
> > Unexpected flakiness: text diff mismatch (1)
> >   webaudio/biquad-lowshelf.html = TEXT PASS
> > 
> > Tests Biquad lowshelf filter.
> > 
> > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > 
> > PASS
> > 
> > FAIL Rendered output did not have has 831 infinities or NaNs.
> 
> Oops.  A typo got into the message and no one noticed.  Could you update your patch to fix this to read "output has 831 infinities..."?
It may be not typo, it`s the diff log from the error page, which may confuse you.
> 
> > PASS
> > FAIL Lowshelf filter response is correct.
> 
> Oops.  That should be "is not correct".
Comment 23 Raymond Toy 2012-02-07 21:01:38 PST
(In reply to comment #22)
> > But I will look into this some more on my system (but without IPP.)
> 
> I cleaned the out dir and recompiled the DumpRenderTree, the tests still failed.

Ok.  I think I will need to get a copy of ipp to test with.

> > > 
> > > I tried to adjust the value of 'timeStep' in biquad-testing.js as the comments, but it did not work. I could not figure out the problem in patch or in the test scripts, could you provide some help on this?  Thanks a lot.
> > 
> > What did you change timeStep to?  And in what way did it not work?
> > 
> As your comments, I tried enlarge the timeStep to the values like 0.2, 0.5 ,1, 10 and I even tried to set it to some small values, like 0.01. All the tryings could not make all the tests pass always while running the tests together. For every single value, the tests are not passed occasionally, and different tests failed by different values.
> 
> Most of the failed tests shows it has infinites or NaNs, some of them shows err values.

If you can, open up the javascript console and look at renderedData and see if the values make sense.  You may want to modify the test and have the location of the infinity or NaN printed out so you can verify renderedData really has those bad values.

Otherwise, I'll need to get ipp going on my linux machine to debug this.
Comment 24 Xingnan Wang 2012-02-07 21:03:39 PST
(In reply to comment #23)
> (In reply to comment #22)
> > > But I will look into this some more on my system (but without IPP.)
> > 
> > I cleaned the out dir and recompiled the DumpRenderTree, the tests still failed.
> 
> Ok.  I think I will need to get a copy of ipp to test with.
> 
> > > > 
> > > > I tried to adjust the value of 'timeStep' in biquad-testing.js as the comments, but it did not work. I could not figure out the problem in patch or in the test scripts, could you provide some help on this?  Thanks a lot.
> > > 
> > > What did you change timeStep to?  And in what way did it not work?
> > > 
> > As your comments, I tried enlarge the timeStep to the values like 0.2, 0.5 ,1, 10 and I even tried to set it to some small values, like 0.01. All the tryings could not make all the tests pass always while running the tests together. For every single value, the tests are not passed occasionally, and different tests failed by different values.
> > 
> > Most of the failed tests shows it has infinites or NaNs, some of them shows err values.
> 
> If you can, open up the javascript console and look at renderedData and see if the values make sense.  You may want to modify the test and have the location of the infinity or NaN printed out so you can verify renderedData really has those bad values.
> 
> Otherwise, I'll need to get ipp going on my linux machine to debug this.

All right, I`ll try this.
Comment 25 Xingnan Wang 2012-02-09 21:40:24 PST
Created attachment 126449 [details]
Patch

Hi Ray,
Fixed the issue that the layout tests could not be passed sometimes.
Root cause is one parameter of IIR filter are not set correctly and the out-place buffer must be zeroed while reset();

Thank you.
Comment 26 Chris Rogers 2012-02-10 13:26:56 PST
Comment on attachment 126449 [details]
Patch

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

Overall looks pretty good!  I'm glad the layout tests are now passing

> Source/WebCore/platform/audio/Biquad.cpp:256
> +    setNormalizedCoefficients(m_b0, m_b1, m_b2, 1, m_a1, m_a2);

It seems odd that we first set the values directly on the member variables in this method, then call setNormalizedCoefficients().

Can we improve this in this method and setHighpassParams() and any similar methods to be more consistent and similar to what we do in setLowShelfParams()
Comment 27 Raymond Toy 2012-02-10 13:35:20 PST
Comment on attachment 126449 [details]
Patch

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

LGTM

> LayoutTests/webaudio/resources/biquad-testing.js:73
> +        a2 = 0;

nit: Can you explain why this is needed now when it wasn't needed without IPP?  When I run this with freq = 0 (and q = 1 and gain = 1) I get b0=b1=b2=0 and a1=-2 and a2=1.  Is it because there's a pole at 1?

> LayoutTests/webaudio/resources/biquad-testing.js:386
> +    var a2 = filterCoef.a2;

Sorry about that.  I'm surprised this worked at all before.
Comment 28 Xingnan Wang 2012-02-12 22:35:47 PST
Comment on attachment 126449 [details]
Patch

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

>> LayoutTests/webaudio/resources/biquad-testing.js:73
>> +        a2 = 0;
> 
> nit: Can you explain why this is needed now when it wasn't needed without IPP?  When I run this with freq = 0 (and q = 1 and gain = 1) I get b0=b1=b2=0 and a1=-2 and a2=1.  Is it because there's a pole at 1?

I found you set b0=b1=b2=a1=a2=0 in setLowpassparams() in Biquad.cpp, and did not in the test script, I`ve thought they should be the same. Any reason to treat freq==0 condition differently?

>> LayoutTests/webaudio/resources/biquad-testing.js:386
>> +    var a2 = filterCoef.a2;
> 
> Sorry about that.  I'm surprised this worked at all before.

All right, it seems JS is smarter than C++ ~

>> Source/WebCore/platform/audio/Biquad.cpp:256
>> +    setNormalizedCoefficients(m_b0, m_b1, m_b2, 1, m_a1, m_a2);
> 
> It seems odd that we first set the values directly on the member variables in this method, then call setNormalizedCoefficients().
> 
> Can we improve this in this method and setHighpassParams() and any similar methods to be more consistent and similar to what we do in setLowShelfParams()

That`s right, I`ll refine it.
Comment 29 Xingnan Wang 2012-02-13 18:14:21 PST
(In reply to comment #28)
> (From update of attachment 126449 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126449&action=review
> 
> >> LayoutTests/webaudio/resources/biquad-testing.js:73
> >> +        a2 = 0;
> > 
> > nit: Can you explain why this is needed now when it wasn't needed without IPP?  When I run this with freq = 0 (and q = 1 and gain = 1) I get b0=b1=b2=0 and a1=-2 and a2=1.  Is it because there's a pole at 1?
> 
> I found you set b0=b1=b2=a1=a2=0 in setLowpassparams() in Biquad.cpp, and did not in the test script, I`ve thought they should be the same. Any reason to treat freq==0 condition differently?
> 
Ray, there is nothing special for IPP. So I won`t change the code here, if it should be different for condition freq=0.
> >> LayoutTests/webaudio/resources/biquad-testing.js:386
> >> +    var a2 = filterCoef.a2;
> > 
> > Sorry about that.  I'm surprised this worked at all before.
> 
> All right, it seems JS is smarter than C++ ~
> 
> >> Source/WebCore/platform/audio/Biquad.cpp:256
> >> +    setNormalizedCoefficients(m_b0, m_b1, m_b2, 1, m_a1, m_a2);
> > 
> > It seems odd that we first set the values directly on the member variables in this method, then call setNormalizedCoefficients().
> > 
> > Can we improve this in this method and setHighpassParams() and any similar methods to be more consistent and similar to what we do in setLowShelfParams()
> 
> That`s right, I`ll refine it.
Comment 30 Raymond Toy 2012-02-14 09:36:05 PST
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 126449 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126449&action=review
> > 
> > >> LayoutTests/webaudio/resources/biquad-testing.js:73
> > >> +        a2 = 0;
> > > 
> > > nit: Can you explain why this is needed now when it wasn't needed without IPP?  When I run this with freq = 0 (and q = 1 and gain = 1) I get b0=b1=b2=0 and a1=-2 and a2=1.  Is it because there's a pole at 1?
> > 
> > I found you set b0=b1=b2=a1=a2=0 in setLowpassparams() in Biquad.cpp, and did not in the test script, I`ve thought they should be the same. Any reason to treat freq==0 condition differently?
> > 
> Ray, there is nothing special for IPP. So I won`t change the code here, if it should be different for condition freq=0.

Ok.  I can't remember why I did that in Biquad.cpp, but in the test, I wanted to have as few special cases as possible there.
Comment 31 Xingnan Wang 2012-02-14 20:59:29 PST
Created attachment 127107 [details]
Patch
Comment 32 Xingnan Wang 2012-02-15 19:06:46 PST
Comment on attachment 127107 [details]
Patch

Patch is update as comments.
Comment 33 Raymond Toy 2012-02-16 08:57:48 PST
Comment on attachment 127107 [details]
Patch

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

Other than the typo and one additional comment, this looks good.

> Source/WebCore/ChangeLog:6
> +        Use IIR filter in IPP and imporve ~27% performance in linux.

"imporve".  Maybe better to say "and improve performance on linux by ~27%"

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

Add comment the changes are covered by the current tests?
Comment 34 Xingnan Wang 2012-02-16 23:52:50 PST
Created attachment 127537 [details]
Patch

Update the patch as comments.
Comment 35 Raymond Toy 2012-02-17 16:27:52 PST
Comment on attachment 127537 [details]
Patch

Looks good.
Comment 36 Chris Rogers 2012-02-17 17:29:07 PST
Comment on attachment 127537 [details]
Patch

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

> Source/WebCore/platform/audio/Biquad.cpp:57
> +    m_biquadState = 0;

I'm not clear how m_biquadState is allocated, since I see that we call ippsIIRFree64f_32f(m_biquadState); on line 78

Is it in the ippsIIRInit64f_BiQuad_32f() call in setNormalizedCoefficients()?  I'm not clear about that.

If this is the case, then what happens if somebody tries to use the filter with the default settings (where setNormalizedCoefficients() never is called).

Also, it looks like reset() sets m_biquadState back to 0 and we have a similar problem?

> Source/WebCore/platform/audio/Biquad.cpp:213
> +        m_biquadState = 0;

See comment above about how m_biquadState is initialized

> Source/WebCore/platform/audio/Biquad.h:113
> +#endif // USE(WEBAUDIO_IPP)

Do we even use the m_b0 (and similar) and m_x1 (and similar) member variables in the IPP case?
If not then we should wrap them in
#if !USE(WEBAUDIO_IPP)
Comment 37 Xingnan Wang 2012-02-19 23:16:43 PST
Created attachment 127768 [details]
Patch
Comment 38 Xingnan Wang 2012-02-19 23:37:42 PST
(In reply to comment #36)
> (From update of attachment 127537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127537&action=review
> 
> > Source/WebCore/platform/audio/Biquad.cpp:57
> > +    m_biquadState = 0;
> 
> I'm not clear how m_biquadState is allocated, since I see that we call ippsIIRFree64f_32f(m_biquadState); on line 78
> 
> Is it in the ippsIIRInit64f_BiQuad_32f() call in setNormalizedCoefficients()?  I'm not clear about that.
> 
> If this is the case, then what happens if somebody tries to use the filter with the default settings (where setNormalizedCoefficients() never is called).
> 
> Also, it looks like reset() sets m_biquadState back to 0 and we have a similar problem?
> 
m_biquadState is not needed to be allocated the memory in out of place mode in which a external buffer(m_ippInternalBuffer) is used and the buffer stores the IIR filter structure. The ippsIIRInit64f_BiQuad_32f() is for initializing the structure with parameters into m_ippInternalBuffer. So I should not call ippsIIRFree*() and should call setNormalizedCoefficients() to initialize m_biquadState in Biquad().
> > Source/WebCore/platform/audio/Biquad.cpp:213
> > +        m_biquadState = 0;
> 
> See comment above about how m_biquadState is initialized
> 
> > Source/WebCore/platform/audio/Biquad.h:113
> > +#endif // USE(WEBAUDIO_IPP)
> 
> Do we even use the m_b0 (and similar) and m_x1 (and similar) member variables in the IPP case?
> If not then we should wrap them in
> #if !USE(WEBAUDIO_IPP)
m_b0 (and similar) are used in getFrequencyResponse() so I should keep them in IPP case.
m_x1 (and similar) are neither used in IPP and DARWIN, so wrap them.

Patch is updated as the comments.
Comment 39 Chris Rogers 2012-02-23 11:30:52 PST
Comment on attachment 127768 [details]
Patch

Xingnan, thanks for the patch!
Comment 40 WebKit Review Bot 2012-02-23 11:48:11 PST
Comment on attachment 127768 [details]
Patch

Rejecting attachment 127768 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11602340
Comment 41 Chris Rogers 2012-02-23 12:13:34 PST
Comment on attachment 127768 [details]
Patch

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

I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.

Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html

> LayoutTests/ChangeLog:5
> +

Please add line (followed by blank line):

Reviewed by NOBODY (OOPS!).
Comment 42 Xingnan Wang 2012-02-23 19:10:39 PST
(In reply to comment #41)
> (From update of attachment 127768 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> 
> I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> 
> Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> 
Chris, I will re-run the latest layout tests and verify the demo.
> > LayoutTests/ChangeLog:5
> > +
> 
> Please add line (followed by blank line):
> 
> Reviewed by NOBODY (OOPS!).

Sorry for missing this.
Comment 43 Xingnan Wang 2012-02-23 23:16:50 PST
Created attachment 128663 [details]
Patch
Comment 44 Xingnan Wang 2012-02-23 23:20:49 PST
(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 127768 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> > 
> > I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> > 
> > Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> > http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> > 
> Chris, I will re-run the latest layout tests and verify the demo.
Layout tests are OK.
The demo looks generally good, but in some situations there is a devita 
> > > LayoutTests/ChangeLog:5
> > > +
> > 
> > Please add line (followed by blank line):
> > 
> > Reviewed by NOBODY (OOPS!).
> 
> Sorry for missing this.
Fixed.
Comment 45 Xingnan Wang 2012-02-23 23:24:10 PST
(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 127768 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> > 
> > I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> > 
> > Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> > http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> > 
> Chris, I will re-run the latest layout tests and verify the demo.
Layout tests are OK.
The demo looks generally good, but I found there is deviation that the magnitude mismatched in lowpass.
You can see the attached pictures.
I can not make sure whether it`s OK? 
> > > LayoutTests/ChangeLog:5
> > > +
> > 
> > Please add line (followed by blank line):
> > 
> > Reviewed by NOBODY (OOPS!).
> 
> Sorry for missing this.
Fixed.
Comment 46 Xingnan Wang 2012-02-23 23:25:16 PST
Created attachment 128666 [details]
output without ipp
Comment 47 Xingnan Wang 2012-02-23 23:25:46 PST
Created attachment 128667 [details]
output with ipp
Comment 48 Raymond Toy 2012-02-24 09:55:25 PST
(In reply to comment #45)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > (From update of attachment 127768 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> > > 
> > > I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> > > 
> > > Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> > > http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> > > 
> > Chris, I will re-run the latest layout tests and verify the demo.
> Layout tests are OK.
> The demo looks generally good, but I found there is deviation that the magnitude mismatched in lowpass.

This doesn't make any sense to me.  The frequency response should not depend on whether we are using IPP or not.  The only thing I can think of is that the filter coefficients are somehow slightly different between the two graphs.  The displayed values for cutoff, Q, and gain are not printed to full precision.
Comment 49 Chris Rogers 2012-02-24 10:36:37 PST
(In reply to comment #48)
> (In reply to comment #45)
> > (In reply to comment #42)
> > > (In reply to comment #41)
> > > > (From update of attachment 127768 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> > > > 
> > > > I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> > > > 
> > > > Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> > > > http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> > > > 
> > > Chris, I will re-run the latest layout tests and verify the demo.
> > Layout tests are OK.
> > The demo looks generally good, but I found there is deviation that the magnitude mismatched in lowpass.
> 
> This doesn't make any sense to me.  The frequency response should not depend on whether we are using IPP or not.  The only thing I can think of is that the filter coefficients are somehow slightly different between the two graphs.  The displayed values for cutoff, Q, and gain are not printed to full precision.

Wow, that's really interesting.  Can you try to debug a little bit more and maybe add some "printf" statements to determine what the calculated filter coefficients are in the two cases?

How do the filters sound when filtering the noise?
Comment 50 Xingnan Wang 2012-02-24 18:55:06 PST
(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #45)
> > > (In reply to comment #42)
> > > > (In reply to comment #41)
> > > > > (From update of attachment 127768 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> > > > > 
> > > > > I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> > > > > 
> > > > > Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> > > > > http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> > > > > 
> > > > Chris, I will re-run the latest layout tests and verify the demo.
> > > Layout tests are OK.
> > > The demo looks generally good, but I found there is deviation that the magnitude mismatched in lowpass.
> > 
> > This doesn't make any sense to me.  The frequency response should not depend on whether we are using IPP or not.  The only thing I can think of is that the filter coefficients are somehow slightly different between the two graphs.  The displayed values for cutoff, Q, and gain are not printed to full precision.
> 
> Wow, that's really interesting.  Can you try to debug a little bit more and maybe add some "printf" statements to determine what the calculated filter coefficients are in the two cases?
> 
> How do the filters sound when filtering the noise?

Chris and Ray,
I think the reason is I compared the graphs in different version.
One is the latest dev-version of chromium with IPP, another is released chrome(17.0.963.56) without IPP. 

I double checked the latest dev-version of chromium, the output with IPP and without IPP from the demo are the same, without the deviation.

So sorry for noising you.

BTW, what`s the reason of the difference of the filter output between different version?
Comment 51 Raymond Toy 2012-02-27 09:16:00 PST
(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (In reply to comment #45)
> > > > (In reply to comment #42)
> > > > > (In reply to comment #41)
> > > > > > (From update of attachment 127768 [details] [details] [details] [details] [details] [details])
> > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=127768&action=review
> > > > > > 
> > > > > > I think the commit queue bot needs to have the ChangeLog include the "Reviewed by..." line.  Please re-upload patch with this fix.
> > > > > > 
> > > > > > Xingnan, I'm assuming you've run the layout tests locally on your linux box with IPP enabled and that they passed.  Also, please verify that this page works with the IPP code running:
> > > > > > http://chromium.googlecode.com/svn/trunk/samples/audio/mag-phase.html
> > > > > > 
> > > > > Chris, I will re-run the latest layout tests and verify the demo.
> > > > Layout tests are OK.
> > > > The demo looks generally good, but I found there is deviation that the magnitude mismatched in lowpass.
> > > 
> > > This doesn't make any sense to me.  The frequency response should not depend on whether we are using IPP or not.  The only thing I can think of is that the filter coefficients are somehow slightly different between the two graphs.  The displayed values for cutoff, Q, and gain are not printed to full precision.
> > 
> > Wow, that's really interesting.  Can you try to debug a little bit more and maybe add some "printf" statements to determine what the calculated filter coefficients are in the two cases?
> > 
> > How do the filters sound when filtering the noise?
> 
> Chris and Ray,
> I think the reason is I compared the graphs in different version.
> One is the latest dev-version of chromium with IPP, another is released chrome(17.0.963.56) without IPP. 
> 
> I double checked the latest dev-version of chromium, the output with IPP and without IPP from the demo are the same, without the deviation.
> 
> So sorry for noising you.
> 
> BTW, what`s the reason of the difference of the filter output between different version?

Good question.  The only thing I can think of is that the function for computing the magnitude and phase had an issue with computing the response as the filter was being changed. I doubt this is the case here.

Since the responses are now similar, between a recent version of chromium with and without IPP, I'm satisfied.
Comment 52 Chris Rogers 2012-02-27 12:45:02 PST
Xingnan, thanks for doing the extra tests.  This looks ok, but I'm afraid you'll have to upload the patch yet one more time after rebasing against latest sources (seems the patch didn't apply cleanly).
Comment 53 Xingnan Wang 2012-02-27 20:51:35 PST
Created attachment 129182 [details]
Patch
Comment 54 Xingnan Wang 2012-02-27 20:53:10 PST
(In reply to comment #53)
> Created an attachment (id=129182) [details]
> Patch

Update the re-based patch.
Comment 55 WebKit Review Bot 2012-03-01 12:41:25 PST
Comment on attachment 129182 [details]
Patch

Rejecting attachment 129182 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11757574
Comment 56 Adam Barth 2012-03-01 14:43:30 PST
Comment on attachment 129182 [details]
Patch

Sorry, one of the commit-queue bots is confused.  I'm trying to track down which one.
Comment 57 WebKit Review Bot 2012-03-01 15:59:18 PST
Comment on attachment 129182 [details]
Patch

Clearing flags on attachment: 129182

Committed r109458: <http://trac.webkit.org/changeset/109458>
Comment 58 WebKit Review Bot 2012-03-01 15:59:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 59 Xingnan Wang 2012-03-01 17:37:08 PST
(In reply to comment #56)
> (From update of attachment 129182 [details])
> Sorry, one of the commit-queue bots is confused.  I'm trying to track down which one.

Hi Adam,

Seems https://bugs.webkit.org/show_bug.cgi?id=77950 met the same error msg when committed, but 77950 did not land and this bug did. Whether the two bugs were blocked with different reasons?