Bug 75522 – Enable IPP for FFTFrame

• Component: Web Audio
• Status: RESOLVED FIXED

[Xingnan Wang]

Add FFTFrameIPP as another backend of FFT in web audio.

[Xingnan Wang]

Created attachment 121060 [details]
Patch

[Xingnan Wang]

Created attachment 121062 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121062 [details]
Patch

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

[Xingnan Wang]

Created attachment 121075 [details]
Patch

[Chris Rogers]

Thanks for the patch!  From what I understand, IPP is among the fastest, so it's great to have it as an option.  I'll have a closer look in a little while, but I wanted to ask about testing...

The timing of this patch is very good, since Ray is about to add a new very important layout test which will (indirectly) test if this code is working correctly or not:
https://bugs.webkit.org/show_bug.cgi?id=75126

Have you tested this code in any way?  For example, have you tried running a Web Audio API demo such as:
http://chromium.googlecode.com/svn/trunk/samples/audio/convolution-effects.html

[Xingnan Wang]

(In reply to comment #5)
> Thanks for the patch!  From what I understand, IPP is among the fastest, so it's great to have it as an option.  I'll have a closer look in a little while, but I wanted to ask about testing...
> 
> The timing of this patch is very good, since Ray is about to add a new very important layout test which will (indirectly) test if this code is working correctly or not:
> https://bugs.webkit.org/show_bug.cgi?id=75126
> 
> Have you tested this code in any way?  For example, have you tried running a Web Audio API demo such as:
> http://chromium.googlecode.com/svn/trunk/samples/audio/convolution-effects.html

Yes, I did some test for FFTFrameIPP.
For the correctness, I tried to run http://chromium.googlecode.com/svn/trunk/samples/audio/convolution-effects.html
but it always failed to load the page (maybe network reason), so wrote a simple test case using ConvolverNode and it was OK.
Also I tried to run the drum machine (http://chromium.googlecode.com/svn/trunk/samples/audio/shiny-drum-machine.html), it works very well.
For the performance, I roughly did a function test in ConvolverNode::process(). 
It boosted about 20% if we used accurate DFT in IPP(ippAlgHintFast).
It boosted about 100% if we used fast DFT in IPP(ippAlgHintAccurate).
All tests in 
Intel(R) Core(TM) i7 CPU 870  @ 2.93GHz 
Ubuntu 11.10

[Chris Rogers]

Thanks for running the performance tests.  It would be good to know if we can use the "fast" option and get the maximum performance benefit.

When you have time, can you test the "fast" option with Raymond's latest layout test.  His patch should be committed soon, but you can grab his test and try it locally on your machine:
https://bugs.webkit.org/show_bug.cgi?id=75126

Thanks!

[Xingnan Wang]

(In reply to comment #7)
> Thanks for running the performance tests.  It would be good to know if we can use the "fast" option and get the maximum performance benefit.
> 
> When you have time, can you test the "fast" option with Raymond's latest layout test.  His patch should be committed soon, but you can grab his test and try it locally on your machine:
> https://bugs.webkit.org/show_bug.cgi?id=75126
> 
> Thanks!

That`s OK~

[Xingnan Wang]

(In reply to comment #8)
> (In reply to comment #7)
> > Thanks for running the performance tests.  It would be good to know if we can use the "fast" option and get the maximum performance benefit.
> > 
> > When you have time, can you test the "fast" option with Raymond's latest layout test.  His patch should be committed soon, but you can grab his test and try it locally on your machine:
> > https://bugs.webkit.org/show_bug.cgi?id=75126
> > 
> > Thanks!
> 
> That`s OK~

I tried the convolution layout test and failed with the result:
*******
Tests ConvolverNode processing a mono channel with mono impulse response.

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

PASS Initial latency of convolver is silent.
FAIL Triangular portion of convolution is not incorrect.  Max deviation = 2.0867834820236123e-7
FAIL First part of tail of convolution is not sufficiently small: 0.00244140625
PASS Rendered signal after tail of convolution is silent.
FAIL Test signal was not correctly convolved.
PASS successfullyParsed is true

TEST COMPLETE
******

I found the threshold values are experimental, the "Max deviation" and "small" I got are only larger than the values in test script a little. I wonder to know whether such values are tolerable?

[Chris Rogers]

(In reply to comment #9)
> 
> I tried the convolution layout test and failed with the result:
> *******
> Tests ConvolverNode processing a mono channel with mono impulse response.
> 
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> PASS Initial latency of convolver is silent.
> FAIL Triangular portion of convolution is not incorrect.  Max deviation = 2.0867834820236123e-7
> FAIL First part of tail of convolution is not sufficiently small: 0.00244140625
> PASS Rendered signal after tail of convolution is silent.
> FAIL Test signal was not correctly convolved.
> PASS successfullyParsed is true
> 
> TEST COMPLETE
> ******
> 
> I found the threshold values are experimental, the "Max deviation" and "small" I got are only larger than the values in test script a little. I wonder to know whether such values are tolerable?

I believe this is well within an acceptable tolerance, and the performance improvement is significant.  It would be great to go with the "fast" option.

Ray, can you sanity check this and refine the layout test by re-defining the "Max deviation" and "not sufficiently small" in terms of decibels. I can see that "Max deviation" in terms of dB deviation is very slight.  The "not sufficiently small" can be compared to the "peak" value of the triangle to see how far down "in the noise" we are.

[Raymond Toy]

(In reply to comment #10)
> (In reply to comment #9)
> > 
> > I tried the convolution layout test and failed with the result:
> > *******
> > Tests ConvolverNode processing a mono channel with mono impulse response.
> > 
> > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > 
> > PASS Initial latency of convolver is silent.
> > FAIL Triangular portion of convolution is not incorrect.  Max deviation = 2.0867834820236123e-7
> > FAIL First part of tail of convolution is not sufficiently small: 0.00244140625
> > PASS Rendered signal after tail of convolution is silent.
> > FAIL Test signal was not correctly convolved.
> > PASS successfullyParsed is true
> > 
> > TEST COMPLETE
> > ******
> > 
> > I found the threshold values are experimental, the "Max deviation" and "small" I got are only larger than the values in test script a little. I wonder to know whether such values are tolerable?
> 
> I believe this is well within an acceptable tolerance, and the performance improvement is significant.  It would be great to go with the "fast" option.
> 
> Ray, can you sanity check this and refine the layout test by re-defining the "Max deviation" and "not sufficiently small" in terms of decibels. I can see that "Max deviation" in terms of dB deviation is very slight.  The "not sufficiently small" can be compared to the "peak" value of the triangle to see how far down "in the noise" we are.

I believe the values returned are sufficiently close.  I will modify the current test to compute a "dB" value instead.  I will leave the constants as is.  When the new FFT is landed, the thresholds should be adjusted then so we have a record of what the new FFT did.  (I also need to test this on OSX in case the OSX FFT produces different results.)

[Xingnan Wang]

Created attachment 121794 [details]
Patch

Update the patch as the convolution layout test updated, new result:
Tests ConvolverNode processing a mono channel with mono impulse response.

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

PASS Initial latency of convolver is silent.
PASS Triangular portion of convolution is correct.
FAIL First part of tail of convolution is not sufficiently small: 0.00244140625 dB
FAIL Test signal was not correctly convolved.
PASS successfullyParsed is true

TEST COMPLETE

Also the FFTRrameIPP::multiply() is updated to use zvmul() and zsmul() as bug 74842 fixed.

[Raymond Toy]

(In reply to comment #12)
> Created an attachment (id=121794) [details]
> Patch
> 
> Update the patch as the convolution layout test updated, new result:
> Tests ConvolverNode processing a mono channel with mono impulse response.
> 
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> PASS Initial latency of convolver is silent.
> PASS Triangular portion of convolution is correct.
> FAIL First part of tail of convolution is not sufficiently small: 0.00244140625 dB

This is way too large.  This corresponds to a max tail value of about 44100.  (0 dB = 1, relative to max of the triangle.)  The expected value should be less than -146 dB.  Something is messed up.  Perhaps the test is wrong?

[Xingnan Wang]

(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=121794) [details] [details]
> > Patch
> > 
> > Update the patch as the convolution layout test updated, new result:
> > Tests ConvolverNode processing a mono channel with mono impulse response.
> > 
> > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > 
> > PASS Initial latency of convolver is silent.
> > PASS Triangular portion of convolution is correct.
> > FAIL First part of tail of convolution is not sufficiently small: 0.00244140625 dB
> 
> This is way too large.  This corresponds to a max tail value of about 44100.  (0 dB = 1, relative to max of the triangle.)  The expected value should be less than -146 dB.  Something is messed up.  Perhaps the test is wrong?

I think the "tail1Max" of the updated patch is exactly the same as last patch, with the threshold value 0.002 in the old layout test which should be -53.98 dB ( 20*log10(0.002) ) in dB format. But in your updated layout test the value is down to -146.7 dB,  any reason to change the threshold value?

Also, the error output in your updated test script :
testFailed("First part of tail of convolution is not sufficiently small: " + tail1Max + " dB");
should it be
testFailed("First part of tail of convolution is not sufficiently small: " + tail1MaxDecibels+ " dB");
?

[Chris Rogers]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Created an attachment (id=121794) [details] [details] [details]
> > > Patch
> > > 
> > > Update the patch as the convolution layout test updated, new result:
> > > Tests ConvolverNode processing a mono channel with mono impulse response.
> > > 
> > > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > > 
> > > PASS Initial latency of convolver is silent.
> > > PASS Triangular portion of convolution is correct.
> > > FAIL First part of tail of convolution is not sufficiently small: 0.00244140625 dB
> > 
> > This is way too large.  This corresponds to a max tail value of about 44100.  (0 dB = 1, relative to max of the triangle.)  The expected value should be less than -146 dB.  Something is messed up.  Perhaps the test is wrong?
> 
> I think the "tail1Max" of the updated patch is exactly the same as last patch, with the threshold value 0.002 in the old layout test which should be -53.98 dB ( 20*log10(0.002) ) in dB format. But in your updated layout test the value is down to -146.7 dB,  any reason to change the threshold value?
> 
> Also, the error output in your updated test script :
> testFailed("First part of tail of convolution is not sufficiently small: " + tail1Max + " dB");
> should it be
> testFailed("First part of tail of convolution is not sufficiently small: " + tail1MaxDecibels+ " dB");
> ?

Yes, it looks like the layout test itself needs to be fixed as you recommend (to report a failure with tail1MaxDecibels).  Once you fix that, then you can find the true error in dB.  It's not -53.98 dB as you mention because it's the dB difference computed from the refMax if you see how tail1MaxDecibels is computed.

I recommend you fix the layout test and also adjust the threshold so that your code passes (assuming it's better than -100 dB or so) and upload a new patch with that fix included.

[Xingnan Wang]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > Created an attachment (id=121794) [details] [details] [details] [details]
> > > > Patch
> > > > 
> > > > Update the patch as the convolution layout test updated, new result:
> > > > Tests ConvolverNode processing a mono channel with mono impulse response.
> > > > 
> > > > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > > > 
> > > > PASS Initial latency of convolver is silent.
> > > > PASS Triangular portion of convolution is correct.
> > > > FAIL First part of tail of convolution is not sufficiently small: 0.00244140625 dB
> > > 
> > > This is way too large.  This corresponds to a max tail value of about 44100.  (0 dB = 1, relative to max of the triangle.)  The expected value should be less than -146 dB.  Something is messed up.  Perhaps the test is wrong?
> > 
> > I think the "tail1Max" of the updated patch is exactly the same as last patch, with the threshold value 0.002 in the old layout test which should be -53.98 dB ( 20*log10(0.002) ) in dB format. But in your updated layout test the value is down to -146.7 dB,  any reason to change the threshold value?
> > 
> > Also, the error output in your updated test script :
> > testFailed("First part of tail of convolution is not sufficiently small: " + tail1Max + " dB");
> > should it be
> > testFailed("First part of tail of convolution is not sufficiently small: " + tail1MaxDecibels+ " dB");
> > ?
> 
> Yes, it looks like the layout test itself needs to be fixed as you recommend (to report a failure with tail1MaxDecibels).  Once you fix that, then you can find the true error in dB.  It's not -53.98 dB as you mention because it's the dB difference computed from the refMax if you see how tail1MaxDecibels is computed.
> 
> I recommend you fix the layout test and also adjust the threshold so that your code passes (assuming it's better than -100 dB or so) and upload a new patch with that fix included.

Got it. I will fix the test.

[Xingnan Wang]

Created attachment 121959 [details]
Patch

Modified the threshold1 from -146.7 to -145.0.
Fixed output issue.

[Raymond Toy]

(In reply to comment #17)
> Created an attachment (id=121959) [details]
> Patch
> 
> Modified the threshold1 from -146.7 to -145.0.
> Fixed output issue.

Thanks for fixing the output issue and updating the threshold.

[Chris Rogers]

Adam, could you please review the proposed .gyp file changes?

[Xingnan Wang]

(In reply to comment #19)
> Adam, could you please review the proposed .gyp file changes?

Hi Adam and Roger,
How about the process of .gyp file reviewing?

[Chris Rogers]

Added Tony Chang (in addition to Adam Barth) as potential .gyp / build-related reviewers.

Personally, I'm eager to get this reviewed and landed soon.  I think it will be a great option for high-performance audio rendering, especially on low-end devices.

[Tony Chang]

Comment on attachment 121959 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:51
> +    # If set to 1, use the system IPP library, else use the thirdparty/ipp.
> +    'use_system_ipp%': 1,

Do you mean for this to be enabled by default?  In what cases would we compile with WTF_USE_WEBAUDIO_IPP=1 and use_system_ipp = 0?

> Source/WebCore/WebCore.gyp/WebCore.gyp:1169
> +        ['OS=="linux" and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
> +          'conditions': [
> +              ['use_system_ipp == 1', {

If possible, I would prefer 1 flag rather than 2 (WTF_USE_WEBAUDIO_IPP and use_system_ipp).

> Source/WebCore/WebCore.gyp/WebCore.gyp:1171
> +                      '/opt/intel/ipp/include',

Is this always in the same location?  Does the library come with a pkgconfig file that we can query instead?  See other examples of how we check for system libraries in src/build/linux/system.gyp.

> Source/WebKit/chromium/features.gypi:154
>        ['OS!="mac"', {
>          'feature_defines': [
>            'WTF_USE_WEBAUDIO_FFMPEG=1',
> +          'WTF_USE_WEBAUDIO_IPP=0',

Will this be used on mac too?

[Tony Chang]

Comment on attachment 121959 [details]
Patch

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

>> Source/WebKit/chromium/features.gypi:154
>> +          'WTF_USE_WEBAUDIO_IPP=0',
> 
> Will this be used on mac too?

Err, I mean Windows.  Did you mean to put this in the not mac section?

[Xingnan Wang]

(In reply to comment #22)
> (From update of attachment 121959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121959&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:51
> > +    # If set to 1, use the system IPP library, else use the thirdparty/ipp.
> > +    'use_system_ipp%': 1,
> 
> Do you mean for this to be enabled by default?  In what cases would we compile with WTF_USE_WEBAUDIO_IPP=1 and use_system_ipp = 0?
Now I just enable the system ipp, but I think it makes sense to add IPP to third_party in future and "use_system_ipp=0" is for such condition. Also some FIXMEs about it are added.
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:1169
> > +        ['OS=="linux" and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
> > +          'conditions': [
> > +              ['use_system_ipp == 1', {
> 
> If possible, I would prefer 1 flag rather than 2 (WTF_USE_WEBAUDIO_IPP and use_system_ipp).
> 
If ipp could be enabled in third_party, the 2 flag are all needed. 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:1171
> > +                      '/opt/intel/ipp/include',
> 
> Is this always in the same location?  Does the library come with a pkgconfig file that we can query instead?  See other examples of how we check for system libraries in src/build/linux/system.gyp.
> 
That`s right, I`ll use pkg-config.
> > Source/WebKit/chromium/features.gypi:154
> >        ['OS!="mac"', {
> >          'feature_defines': [
> >            'WTF_USE_WEBAUDIO_FFMPEG=1',
> > +          'WTF_USE_WEBAUDIO_IPP=0',
> 
> Will this be used on mac too?
Now I just enabled IPP in linux and "OS==linux" should be considered.
IPP is not used as default so just removing this line is OK.

[Tony Chang]

Comment on attachment 121959 [details]
Patch

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

>>> Source/WebCore/WebCore.gyp/WebCore.gyp:51
>>> +    'use_system_ipp%': 1,
>> 
>> Do you mean for this to be enabled by default?  In what cases would we compile with WTF_USE_WEBAUDIO_IPP=1 and use_system_ipp = 0?
> 
> Now I just enable the system ipp, but I think it makes sense to add IPP to third_party in future and "use_system_ipp=0" is for such condition. Also some FIXMEs about it are added.

I would remove the use_system_ipp variable from this patch and just assume that if WTF_USE_WEBAUDIO_IPP=1, that we're using the system copy.  We can add the use_system_ipp variable after we've checked in a copy of ipp into third_party.

[Chris Rogers]

(In reply to comment #25)
> (From update of attachment 121959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121959&action=review
> 
> >>> Source/WebCore/WebCore.gyp/WebCore.gyp:51
> >>> +    'use_system_ipp%': 1,
> >> 
> >> Do you mean for this to be enabled by default?  In what cases would we compile with WTF_USE_WEBAUDIO_IPP=1 and use_system_ipp = 0?
> > 
> > Now I just enable the system ipp, but I think it makes sense to add IPP to third_party in future and "use_system_ipp=0" is for such condition. Also some FIXMEs about it are added.
> 
> I would remove the use_system_ipp variable from this patch and just assume that if WTF_USE_WEBAUDIO_IPP=1, that we're using the system copy.  We can add the use_system_ipp variable after we've checked in a copy of ipp into third_party.

Sorry to make the review a little more complicated, but I'm not sure if we will be able to check the libraries into third_party, depending on the commercial license Intel provides for this (don't worry it's not GPL).  Xingnan, do you know any more details, or if Intel would be willing to open IPP up as a free license for WebKit?  For Chrome, we'd have to keep it in "chrome internal" somewhere, if not.

But (I think) Intel is interested in using this in general chromium (not Chrome-branded), so I don't know what the right approach is...

[Xingnan Wang]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 121959 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121959&action=review
> > 
> > >>> Source/WebCore/WebCore.gyp/WebCore.gyp:51
> > >>> +    'use_system_ipp%': 1,
> > >> 
> > >> Do you mean for this to be enabled by default?  In what cases would we compile with WTF_USE_WEBAUDIO_IPP=1 and use_system_ipp = 0?
> > > 
> > > Now I just enable the system ipp, but I think it makes sense to add IPP to third_party in future and "use_system_ipp=0" is for such condition. Also some FIXMEs about it are added.
> > 
> > I would remove the use_system_ipp variable from this patch and just assume that if WTF_USE_WEBAUDIO_IPP=1, that we're using the system copy.  We can add the use_system_ipp variable after we've checked in a copy of ipp into third_party.
> 
> Sorry to make the review a little more complicated, but I'm not sure if we will be able to check the libraries into third_party, depending on the commercial license Intel provides for this (don't worry it's not GPL).  Xingnan, do you know any more details, or if Intel would be willing to open IPP up as a free license for WebKit?  For Chrome, we'd have to keep it in "chrome internal" somewhere, if not.
> 
> But (I think) Intel is interested in using this in general chromium (not Chrome-branded), so I don't know what the right approach is...

Hi Roger,

AFAIK, currently IPP is under commercial license, anyone uses it should buy it. So I don't think we can check it into third_party and share with community. 

But I think it is meaningful to put the IPP related information in gyp file. 

In this way, any user or company wants to build chromium with IPP can buy IPP and install it or put IPP library under third_party folder(just like MKL) and build chromium and then re-distribute the binary(maybe just like how chrome is build). 

As for free license for WebKit, it should be a business decision, which is out of our scope, we can contact with corresponding guys for this option, but currently I think the answer is no. thanks

[Chris Rogers]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 121959 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=121959&action=review
> > > 
> > > >>> Source/WebCore/WebCore.gyp/WebCore.gyp:51
> > > >>> +    'use_system_ipp%': 1,
> > > >> 
> > > >> Do you mean for this to be enabled by default?  In what cases would we compile with WTF_USE_WEBAUDIO_IPP=1 and use_system_ipp = 0?
> > > > 
> > > > Now I just enable the system ipp, but I think it makes sense to add IPP to third_party in future and "use_system_ipp=0" is for such condition. Also some FIXMEs about it are added.
> > > 
> > > I would remove the use_system_ipp variable from this patch and just assume that if WTF_USE_WEBAUDIO_IPP=1, that we're using the system copy.  We can add the use_system_ipp variable after we've checked in a copy of ipp into third_party.
> > 
> > Sorry to make the review a little more complicated, but I'm not sure if we will be able to check the libraries into third_party, depending on the commercial license Intel provides for this (don't worry it's not GPL).  Xingnan, do you know any more details, or if Intel would be willing to open IPP up as a free license for WebKit?  For Chrome, we'd have to keep it in "chrome internal" somewhere, if not.
> > 
> > But (I think) Intel is interested in using this in general chromium (not Chrome-branded), so I don't know what the right approach is...
> 
> Hi Roger,
> 
> AFAIK, currently IPP is under commercial license, anyone uses it should buy it. So I don't think we can check it into third_party and share with community. 
> 
> But I think it is meaningful to put the IPP related information in gyp file. 

Agreed!  I'm not a .gyp expert so hopefully Tony can help us figure out how to abstract this correctly...

> 
> In this way, any user or company wants to build chromium with IPP can buy IPP and install it or put IPP library under third_party folder(just like MKL) and build chromium and then re-distribute the binary(maybe just like how chrome is build). 
> 
> As for free license for WebKit, it should be a business decision, which is out of our scope, we can contact with corresponding guys for this option, but currently I think the answer is no. thanks

That's understandable - no worries.

[Xingnan Wang]

Created attachment 124867 [details]
Patch

Update the gyp file as comments, thanks.

[Tony Chang]

Comment on attachment 124867 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:1357
> +        ['OS=="linux" and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
> +          'cflags': [
> +            '<!@(pkg-config --cflags-only-I ipp)',

Does this need to be here in the webcore_prerequisite targets section?  Since this target doesn't compile any files, this probably doesn't do anything.  However, I think if you added a direct_dependent_settings section, the cflags will be used by the other webcore targets.  Which means you wouldn't need to have the other cflags sections below.  E.g., this could be:

['OS=="linux" and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
  'direct_dependent_settings': {
    'cflags': [
      '<!@(pkg-config --cflags-only-I ipp)',
    ],
  },
}],

> Source/WebCore/platform/audio/FFTFrame.h:57
> +#include "ipps.h"

Nit: Should we use <ipps.h> since it's a system header?

[Xingnan Wang]

Created attachment 125242 [details]
Patch

[Xingnan Wang]

(In reply to comment #30)
> (From update of attachment 124867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124867&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:1357
> > +        ['OS=="linux" and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
> > +          'cflags': [
> > +            '<!@(pkg-config --cflags-only-I ipp)',
> 
> Does this need to be here in the webcore_prerequisite targets section?  Since this target doesn't compile any files, this probably doesn't do anything.  However, I think if you added a direct_dependent_settings section, the cflags will be used by the other webcore targets.  Which means you wouldn't need to have the other cflags sections below.  E.g., this could be:
> 
> ['OS=="linux" and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
>   'direct_dependent_settings': {
>     'cflags': [
>       '<!@(pkg-config --cflags-only-I ipp)',
>     ],
>   },
> }],
> 
> > Source/WebCore/platform/audio/FFTFrame.h:57
> > +#include "ipps.h"
> 
> Nit: Should we use <ipps.h> since it's a system header?

Tony, thanks your comments, patch is updated.

[Tony Chang]

Comment on attachment 125242 [details]
Patch

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

Just some small nits left. Otherwise, the gyp changes look good to me.

Chris, do the code changes look good to you?

> Source/WebCore/WebCore.gyp/WebCore.gyp:2001
> +          'all_dependent_settings': {

I don't think you need all_dependent_settings here.  I think gyp is smart enough to copy link_settings down to the targets that need it.

> Source/WebCore/WebCore.gyp/WebCore.gyp:2004
> +            'ldflags': [
> +              '<!@(pkg-config --libs-only-L ipp)',
> +            ],

Doesn't this have to go in link_settings?  That's what I see for other uses of ldflags.  See Source/WebKit/chromium/build/linux/system.gyp for other examples.

[Chris Rogers]

Tony, the changes look good in the FFTFrame code.

Xingnan, I assume the convolution layout test is passing when you enable IPP?  Philippe Normand just made some adjustments to the threshold in convolution-testing.js:
http://trac.webkit.org/changeset/106537

[Xingnan Wang]

Created attachment 125587 [details]
Patch

[Xingnan Wang]

(In reply to comment #33)
> (From update of attachment 125242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125242&action=review
> 
> Just some small nits left. Otherwise, the gyp changes look good to me.
> 
> Chris, do the code changes look good to you?
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:2001
> > +          'all_dependent_settings': {
> 
> I don't think you need all_dependent_settings here.  I think gyp is smart enough to copy link_settings down to the targets that need it.
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:2004
> > +            'ldflags': [
> > +              '<!@(pkg-config --libs-only-L ipp)',
> > +            ],
> 
> Doesn't this have to go in link_settings?  That's what I see for other uses of ldflags.  See Source/WebKit/chromium/build/linux/system.gyp for other examples.

Agree, update the patch as your comments.

[Xingnan Wang]

(In reply to comment #34)
> Tony, the changes look good in the FFTFrame code.
> 
> Xingnan, I assume the convolution layout test is passing when you enable IPP?  Philippe Normand just made some adjustments to the threshold in convolution-testing.js:
> http://trac.webkit.org/changeset/106537

Yes Chris, the adjusted threshold value in 106537 is enough for my patch to pass the test. I double-checked it and it`s OK.

[Chris Rogers]

(In reply to comment #37)
> (In reply to comment #34)
> > Tony, the changes look good in the FFTFrame code.
> > 
> > Xingnan, I assume the convolution layout test is passing when you enable IPP?  Philippe Normand just made some adjustments to the threshold in convolution-testing.js:
> > http://trac.webkit.org/changeset/106537
> 
> Yes Chris, the adjusted threshold value in 106537 is enough for my patch to pass the test. I double-checked it and it`s OK.

Great!  Looks good - passing to Tony for final review.

[Tony Chang]

Comment on attachment 125587 [details]
Patch

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

> Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:44
> +const unsigned kMaxFFTPow2Size = 24;

Nit: We normally just use regular variable naming for consts in WebCore.  E.g., maximumFFTPower2Size.

> Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:85
> +    unsigned nbytes = sizeof(float) * m_FFTSize;

Nit: We try to avoid abbreviations in variable names. Maybe numberOfBytes or byteSize?

> Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:139
> +    ippsDFTFwd_RToPerm_32f((Ipp32f*)data, complexP, m_DFTSpec, m_buffer);

Nit: reinterpret_cast?

> Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:147
> +    ippsCplxToReal_32fc((Ipp32fc*)complexP, realP, imagP, m_FFTSize >> 1);

Nit: reinterpret_cast?

> Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:155
> +    ippsDFTInv_PermToR_32f(complexP, (Ipp32f*)data, m_DFTSpec, m_buffer);

Nit: reinterpret_cast?

> Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:160
> +    ippsMulC_32f_I(scale, (Ipp32f*)data, m_FFTSize);

Nit: reinterpret_cast?

[Xingnan Wang]

Created attachment 125753 [details]
Patch

[Xingnan Wang]

(In reply to comment #39)
> (From update of attachment 125587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125587&action=review
> 
> > Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:44
> > +const unsigned kMaxFFTPow2Size = 24;
> 
> Nit: We normally just use regular variable naming for consts in WebCore.  E.g., maximumFFTPower2Size.
> 
> > Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:85
> > +    unsigned nbytes = sizeof(float) * m_FFTSize;
> 
> Nit: We try to avoid abbreviations in variable names. Maybe numberOfBytes or byteSize?
> 
> > Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:139
> > +    ippsDFTFwd_RToPerm_32f((Ipp32f*)data, complexP, m_DFTSpec, m_buffer);
> 
> Nit: reinterpret_cast?
> 
> > Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:147
> > +    ippsCplxToReal_32fc((Ipp32fc*)complexP, realP, imagP, m_FFTSize >> 1);
> 
> Nit: reinterpret_cast?
> 
> > Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:155
> > +    ippsDFTInv_PermToR_32f(complexP, (Ipp32f*)data, m_DFTSpec, m_buffer);
> 
> Nit: reinterpret_cast?
> 
> > Source/WebCore/platform/audio/ipp/FFTFrameIPP.cpp:160
> > +    ippsMulC_32f_I(scale, (Ipp32f*)data, m_FFTSize);
> 
> Nit: reinterpret_cast?

Thanks Tony, patch is updated as comments.

[Tony Chang]

Do you want me to commit the patch?  If so, set commit-queue?

[WebKit Review Bot]

Comment on attachment 125753 [details]
Patch

Clearing flags on attachment: 125753

Committed r107025: <http://trac.webkit.org/changeset/107025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.