Bug 77509

Created attachment 124881 [details] Patch From my test in linux, the performance increased ~27% for the processing of biquad.

Comment on attachment 124881 [details] Patch Attachment 124881 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11386329

Created attachment 124886 [details] Patch

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

(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%?

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!

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

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

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

(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 :)

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

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

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

Created attachment 125752 [details] Patch

Created attachment 125775 [details] Patch

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

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

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

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

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

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

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

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

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

Created attachment 127107 [details] Patch

Comment on attachment 127107 [details] Patch Patch is update as comments.

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?

Created attachment 127537 [details] Patch Update the patch as comments.

Comment on attachment 127537 [details] Patch Looks good.

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)

Created attachment 127768 [details] Patch

(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 on attachment 127768 [details] Patch Xingnan, thanks for the patch!

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

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

Created attachment 128663 [details] Patch

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

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

Created attachment 128666 [details] output without ipp

Created attachment 128667 [details] output with ipp

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

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

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

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

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

Created attachment 129182 [details] Patch

(In reply to comment #53) > Created an attachment (id=129182) [details] > Patch Update the re-based patch.

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 on attachment 129182 [details] Patch Sorry, one of the commit-queue bots is confused. I'm trying to track down which one.

Comment on attachment 129182 [details] Patch Clearing flags on attachment: 129182 Committed r109458: <http://trac.webkit.org/changeset/109458>

All reviewed patches have been landed. Closing bug.

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