Bug 75522 - Enable IPP for FFTFrame
Summary: Enable IPP for FFTFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 77509
  Show dependency treegraph
 
Reported: 2012-01-03 21:44 PST by Xingnan Wang
Modified: 2012-02-07 18:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (7.87 KB, patch)
2012-01-03 21:56 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (deleted)
2012-01-03 22:03 PST, Xingnan Wang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (13.79 KB, patch)
2012-01-03 23:08 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (13.65 KB, patch)
2012-01-09 22:21 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (14.69 KB, patch)
2012-01-10 19:01 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (12.44 KB, patch)
2012-01-31 19:08 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (10.95 KB, patch)
2012-02-02 19:32 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (10.86 KB, patch)
2012-02-06 00:38 PST, Xingnan Wang
tony: review+
Details | Formatted Diff | Diff
Patch (11.00 KB, patch)
2012-02-06 19:06 PST, Xingnan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xingnan Wang 2012-01-03 21:44:29 PST
Add FFTFrameIPP as another backend of FFT in web audio.
Comment 1 Xingnan Wang 2012-01-03 21:56:53 PST
Created attachment 121060 [details]
Patch
Comment 2 Xingnan Wang 2012-01-03 22:03:35 PST
Created attachment 121062 [details]
Patch
Comment 3 WebKit Review Bot 2012-01-03 22:13:52 PST
Comment on attachment 121062 [details]
Patch

Attachment 121062 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10954796
Comment 4 Xingnan Wang 2012-01-03 23:08:37 PST
Created attachment 121075 [details]
Patch
Comment 5 Chris Rogers 2012-01-04 13:41:16 PST
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
Comment 6 Xingnan Wang 2012-01-04 17:49:25 PST
(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
Comment 7 Chris Rogers 2012-01-04 18:12:00 PST
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!
Comment 8 Xingnan Wang 2012-01-04 18:18:32 PST
(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~
Comment 9 Xingnan Wang 2012-01-05 20:46:06 PST
(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?
Comment 10 Chris Rogers 2012-01-05 21:01:09 PST
(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.
Comment 11 Raymond Toy 2012-01-06 09:02:31 PST
(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.)
Comment 12 Xingnan Wang 2012-01-09 22:21:28 PST
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.
Comment 13 Raymond Toy 2012-01-10 10:52:48 PST
(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?
Comment 14 Xingnan Wang 2012-01-10 17:57:35 PST
(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");
?
Comment 15 Chris Rogers 2012-01-10 18:10:16 PST
(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.
Comment 16 Xingnan Wang 2012-01-10 18:20:13 PST
(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.
Comment 17 Xingnan Wang 2012-01-10 19:01:06 PST
Created attachment 121959 [details]
Patch

Modified the threshold1 from -146.7 to -145.0.
Fixed output issue.
Comment 18 Raymond Toy 2012-01-11 08:57:11 PST
(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.
Comment 19 Chris Rogers 2012-01-11 11:34:51 PST
Adam, could you please review the proposed .gyp file changes?
Comment 20 Xingnan Wang 2012-01-28 17:24:19 PST
(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?
Comment 21 Chris Rogers 2012-01-28 19:35:58 PST
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.
Comment 22 Tony Chang 2012-01-29 18:41:37 PST
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?
Comment 23 Tony Chang 2012-01-29 18:43:05 PST
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?
Comment 24 Xingnan Wang 2012-01-30 00:41:50 PST
(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.
Comment 25 Tony Chang 2012-01-30 10:03:37 PST
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.
Comment 26 Chris Rogers 2012-01-30 10:27:41 PST
(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...
Comment 27 Xingnan Wang 2012-01-30 18:08:04 PST
(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
Comment 28 Chris Rogers 2012-01-30 19:13:26 PST
(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.
Comment 29 Xingnan Wang 2012-01-31 19:08:30 PST
Created attachment 124867 [details]
Patch

Update the gyp file as comments, thanks.
Comment 30 Tony Chang 2012-02-02 18:09:21 PST
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?
Comment 31 Xingnan Wang 2012-02-02 19:32:05 PST
Created attachment 125242 [details]
Patch
Comment 32 Xingnan Wang 2012-02-02 19:34:56 PST
(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.
Comment 33 Tony Chang 2012-02-03 10:58:55 PST
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.
Comment 34 Chris Rogers 2012-02-03 11:58:56 PST
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
Comment 35 Xingnan Wang 2012-02-06 00:38:39 PST
Created attachment 125587 [details]
Patch
Comment 36 Xingnan Wang 2012-02-06 00:39:33 PST
(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.
Comment 37 Xingnan Wang 2012-02-06 00:43:30 PST
(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.
Comment 38 Chris Rogers 2012-02-06 09:37:16 PST
(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.
Comment 39 Tony Chang 2012-02-06 10:47:06 PST
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?
Comment 40 Xingnan Wang 2012-02-06 19:06:31 PST
Created attachment 125753 [details]
Patch
Comment 41 Xingnan Wang 2012-02-06 19:09:09 PST
(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.
Comment 42 Tony Chang 2012-02-07 10:00:05 PST
Do you want me to commit the patch?  If so, set commit-queue?
Comment 43 WebKit Review Bot 2012-02-07 18:52:12 PST
Comment on attachment 125753 [details]
Patch

Clearing flags on attachment: 125753

Committed r107025: <http://trac.webkit.org/changeset/107025>
Comment 44 WebKit Review Bot 2012-02-07 18:52:22 PST
All reviewed patches have been landed.  Closing bug.