Bug 73163 - Remove platform/audio/fftw
Summary: Remove platform/audio/fftw
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-26 22:16 PST by Adam Barth
Modified: 2011-12-12 14:13 PST (History)
8 users (show)

See Also:


Attachments
Patch (17.37 KB, patch)
2011-11-26 22:18 PST, Adam Barth
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-11-26 22:16:32 PST
Remove platform/audio/fftw
Comment 1 Adam Barth 2011-11-26 22:18:08 PST
Created attachment 116666 [details]
Patch
Comment 2 Philippe Normand 2011-11-28 00:53:50 PST
Is there a discussion about a possible alternative to it?
Comment 3 Philippe Normand 2011-11-28 00:56:34 PST
It'd be great if the patch updates configure.ac as well, no need to check for libfftw there if support for it is removed.
Comment 4 Adam Barth 2011-11-28 00:59:09 PST
> Is there a discussion about a possible alternative to it?

Are you using this code?  If so, I'm happy to leave it in the tree, but you should check whether you're complying with the various licenses.  IANAL, but s.choi pointed out that there might be some license compatibility problems between FFTW and WebKit.
Comment 5 Philippe Normand 2011-11-28 01:17:53 PST
(In reply to comment #4)
> > Is there a discussion about a possible alternative to it?
> 
> Are you using this code?  If so, I'm happy to leave it in the tree, but you should check whether you're complying with the various licenses.  IANAL, but s.choi pointed out that there might be some license compatibility problems between FFTW and WebKit.

I'll test your patch in my webaudio branch and report back :)
If needed we can hook in the FFT GStreamer lib anyway, it's LGPL.
Comment 6 Soo-Hyun Choi 2011-11-28 01:37:13 PST
(In reply to comment #5)
> (In reply to comment #4)
> > > Is there a discussion about a possible alternative to it?
> > 
> > Are you using this code?  If so, I'm happy to leave it in the tree, but you should check whether you're complying with the various licenses.  IANAL, but s.choi pointed out that there might be some license compatibility problems between FFTW and WebKit.
> 
> I'll test your patch in my webaudio branch and report back :)
> If needed we can hook in the FFT GStreamer lib anyway, it's LGPL.

I would think FFT GStreamer can be a nice alternative. FFTW.org is GPL, so it may lead into potential license conflict with WebKit.
Comment 7 Philippe Normand 2011-11-28 03:32:20 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > > Is there a discussion about a possible alternative to it?
> > > 
> > > Are you using this code?  If so, I'm happy to leave it in the tree, but you should check whether you're complying with the various licenses.  IANAL, but s.choi pointed out that there might be some license compatibility problems between FFTW and WebKit.
> > 
> > I'll test your patch in my webaudio branch and report back :)
> > If needed we can hook in the FFT GStreamer lib anyway, it's LGPL.
> 
> I would think FFT GStreamer can be a nice alternative.

Yes, for the ports already using GStreamer :)
We indeed need a replacement for FFTW, I'll try to provide one using gstfft for the folks interested.

> FFTW.org is GPL, so it may lead into potential license conflict with WebKit.

Yes. I think Chris Rogers chosed FFTW initially because it's very very fast :)
It seems we could contact MIT and get a unlimited-use license, see http://www.fftw.org/faq/section1.html#isfftwfree
Comment 8 Kenneth Russell 2011-11-28 13:03:10 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Is there a discussion about a possible alternative to it?
> > > > 
> > > > Are you using this code?  If so, I'm happy to leave it in the tree, but you should check whether you're complying with the various licenses.  IANAL, but s.choi pointed out that there might be some license compatibility problems between FFTW and WebKit.
> > > 
> > > I'll test your patch in my webaudio branch and report back :)
> > > If needed we can hook in the FFT GStreamer lib anyway, it's LGPL.
> > 
> > I would think FFT GStreamer can be a nice alternative.
> 
> Yes, for the ports already using GStreamer :)
> We indeed need a replacement for FFTW, I'll try to provide one using gstfft for the folks interested.
> 
> > FFTW.org is GPL, so it may lead into potential license conflict with WebKit.
> 
> Yes. I think Chris Rogers chosed FFTW initially because it's very very fast :)
> It seems we could contact MIT and get a unlimited-use license, see http://www.fftw.org/faq/section1.html#isfftwfree

There were some attempts to secure a non-GPL license for the use of FFTW in WebKit, but these were so far unsuccessful. There's been no activity on this front for several months.

Chromium is currently using ffmpeg's FFT on Windows and Linux, because that library is already being linked in to the browser, so there was no increase in code size by doing so.

I personally don't mind r+'ing this patch, though think it would be best if Chris Rogers would be given the chance to comment. He's currently on vacation but due back in a week or two.
Comment 9 Adam Barth 2011-11-28 13:58:33 PST
We can wait for him to get back.  There's no particular rush on this patch.  It just seems like a good cleanup so folks don't accidentally walk into a licensing trap.
Comment 10 Chris Rogers 2011-11-28 20:37:20 PST
Adam, please don't remove this code as the GTK port relies on it.  Presumably their license obligations are less stringent than the other ports since much of their code is GPL.

The mac port uses Accelerate.framework, while chromium uses FFmpeg
Comment 11 Adam Barth 2011-11-28 23:53:04 PST
Thanks Chris.
Comment 12 Philippe Normand 2011-11-29 01:30:24 PST
(In reply to comment #10)
> Adam, please don't remove this code as the GTK port relies on it.  Presumably their license obligations are less stringent than the other ports since much of their code is GPL.
> 

Our code is LGPL actually.
We are in violation of the LGPL if we link against FFTW indeed. But this is unlikely to happen currently because our WebAudio support is not fully landed and not yet enabled by default.

Reopening then!
Comment 13 Philippe Normand 2011-11-29 01:57:27 PST
(In reply to comment #12)
> (In reply to comment #10)
> > Adam, please don't remove this code as the GTK port relies on it.  Presumably their license obligations are less stringent than the other ports since much of their code is GPL.
> > 
> 
> Our code is LGPL actually.
> We are in violation of the LGPL if we link against FFTW indeed. But this is unlikely to happen currently because our WebAudio support is not fully landed and not yet enabled by default.
> 
> Reopening then!

I created bug 73295 to disable WebAudio on GTK. We'll re-enable it when the gstfftw-based implementation lands.
So GTK is not blocking this patch anymore!
Comment 14 Adam Barth 2011-11-29 08:59:51 PST
Ok.  It sounds like we should land this patch then (possibly including a change to configure.ac).
Comment 15 Philippe Normand 2011-11-29 09:45:38 PST
(In reply to comment #14)
> Ok.  It sounds like we should land this patch then (possibly including a change to configure.ac).

The configure.ac change is not needed I think. I can just fix it up when we add the GStreamer fft implementation.
Comment 16 Kenneth Russell 2011-11-29 18:31:06 PST
Comment on attachment 116666 [details]
Patch

I'd still like crogers to give a thumbs up to this removal, but otherwise seems fine to me.
Comment 17 Chris Rogers 2011-12-10 19:03:20 PST
MIT *does* have commercial license options for FFTW which make it compatible with LGPL, so in theory a port could use FFTW.  And since, performance-wise, FFTW is one of the better options we have, it would be nice to avoid removing this code.  But, I'm not a lawyer and I don't know how best to call out this code as needing special care.  At the least, maybe we should add some precautionary comments in the header file?

Maybe we can bring this discussion up on webkit-dev, so that the more legal-oriented folks can weigh in.  It would be nice to keep the code if possible, for potential use by ports with the appropriate commercial license.
Comment 18 Adam Barth 2011-12-11 09:11:06 PST
Rather than having a bunch of code that someone might use one day, we can add the code back if someone actually wants to use it.
Comment 19 Chris Rogers 2011-12-11 13:21:04 PST
That seems reasonable, although it makes it difficult for ports to know that the code is available if it's not actually visible in the tree.  I don't want to hold this back if everybody else wants it removed.
Comment 20 Soo-Hyun Choi 2011-12-11 16:34:43 PST
As said in their homepage, FFTW can be purchased to be compatible with LGPL, and we are aware of it. But as I pointed earlier in the chrome-dev, this could potentially introduce a license trap, leading to a serous license violation.

I would like it to be removed from WebKit source tree, leaving it to a browser vendor's decision whether or not to implement a necessary port as their necessary.

I don't mind bring this issue to webkit-dev, though.
Comment 21 Adam Barth 2011-12-11 17:14:29 PST
Comment on attachment 116666 [details]
Patch

Given that no one is using this code today, I think we're better off removing it.  If someone wants to use it in the future, then we can figure out how to let them do that without creating a license trap for others.
Comment 22 WebKit Review Bot 2011-12-11 17:17:52 PST
Comment on attachment 116666 [details]
Patch

Rejecting attachment 116666 [details] from commit-queue.

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

Last 500 characters of output:
LED at 1305.
Hunk #3 FAILED at 1920.
3 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/WebCore.gyp/WebCore.gyp.rej
patching file Source/WebCore/platform/audio/FFTFrame.h
patching file Source/WebCore/platform/audio/FFTFrameStub.cpp
patching file Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp
rm 'Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp'

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10827924
Comment 23 Adam Barth 2011-12-12 14:13:37 PST
Committed r102622: <http://trac.webkit.org/changeset/102622>