Due to e-mail size limitation, removals of the following files are not included: * src/modules/module-equalizer-sink.c * src/utils/qpaeq To apply the patch correctly, "git am --scissors" this e-mail and then amend the commit by removing these two files. Yes, I understand that it is generally a bad idea (and rude) to destroy other people's contributions. But in this case, I think it is justified, as enough time has passed and people only added FIXMEs instead of fixing this (unfixable except by a full rewrite?) module. Note: I cannot promise to submit a better replacement module. --------------------8<------------------------- Module-equalizer-sink is horrible both from the DSP viewpoint and from C code viewpoint. Here is an incomplete list of what is wrong with it. 1. The FFT size and the window size for overlap-add are chosen inconsistently. The window size is always 16000 samples. The FFT size depends on the sample rate. Thus, with sample rates of 16 kHz or below, there is a buffer overflow (window becomes larger than FFT). 2. There is no attempt to ensure that the impulse response of the equalizer is short enough to fit in the difference between the FFT size and the window size (a requirement for the correct operation of the overlap-add technique). See [1] for the FFT size consideration and [2] why it is important to regularize the desired frequency response. 3. The large window size (16000 samples) is unjustified. It only leads to excessively high latency. A "33 ms of audio" window and "66 ms of audio" FFT size would be sufficient for any practical implementation of the equalizer and would still allow one to control the attenuation at 30 Hz and 60 Hz separately (as commonly found in music players), all with only 33 ms of latency. 4. This latency is not properly reported (fdo bug 41465). 5. There are numerous issues with code style. A lot of commented-out code, and a "FIXME: Please clean this up" that was never fixed in 4 years. 6. There are memory management issues. E.g. the already-pointed-out leak at the end of sink_input_pop_cb(), and the fact that a buffer for the FFT is allocated by fftw_malloc() but freed by free(). 7. The use of the Hanning window is unjustified. A rectangular window works just as well for the purpose of overlap-add based convolution, but allows one to do less FFTs, i.e. go faster. In fact, the DSP Guide book does not even mention the variant of overlap-add with a non-rectangular window! 8. The module does not use any benefits (such as a chance to handle rewinds properly) of being a native PulseAudio module. 9. None of the known alternative "system-wide equalizer GUI" projects work on top of module-equalizer-sink. Google has no external hits on the [EqualizedSinks dbus] search query that would find any alternative GUIs that use module-equalizer-sink DBus interface. Both veromix [3] and pulseaudio-equalizer [4] work on top of module-ladspa-sink. And they are usable with video players, too, even though both are unmaintained. 10. It's more an optimization question, but I have a gut feeling that overlap-save [5] would be a better fit for PulseAudio buffering model than overlap-add. Namely, it would allow to abandon the need to store overlap_accum. Instead, the only buffer needed would be for looking back at the past portions of the input signal, and we already have that anyway due to the need to react to rewinds. In other words, from a qualified DSP engineer, I would rather expect a rewrite of the algorithm instead of any attempts to improve it gradually. In short, let's not pretend that PulseAudio has a working native equalizer module. It should have never been accepted in the current form in the first place. A better replacement already exists in the form of module-ladspa-sink + mbeq + veromix. [1] http://www.dspguide.com/ch18/2.htm [2] http://www.dspguide.com/ch17/1.htm [3] https://code.google.com/p/veromix-plasmoid/ [4] http://www.webupd8.org/2013/10/system-wide-pulseaudio-equalizer.html [5] http://en.wikipedia.org/wiki/Overlap-save_method --- LICENSE | 12 +- configure.ac | 16 - po/POTFILES.in | 1 - src/Makefile.am | 15 - src/modules/module-equalizer-sink.c | 2240 ----------------------------------- src/pulse/proplist.h | 4 +- src/utils/qpaeq | 575 --------- 7 files changed, 8 insertions(+), 2855 deletions(-) delete mode 100644 src/modules/module-equalizer-sink.c delete mode 100755 src/utils/qpaeq diff --git a/LICENSE b/LICENSE index 226c4ce..cb9addb 100644 --- a/LICENSE +++ b/LICENSE @@ -2,12 +2,12 @@ All PulseAudio source files are licensed under the GNU Lesser General Public License. (see file LGPL for details) However, the server side has optional GPL dependencies. These include the -libsamplerate and gdbm (core libraries), LIRC (lirc module) and FFTW (equalizer -module), although others may also be included in the future. If PulseAudio is -compiled with these optional components, this effectively downgrades the -license of the server part to GPL (see the file GPL for details), exercising -section 3 of the LGPL. In such circumstances, you should treat the client -library (libpulse) of PulseAudio as being LGPL licensed and the server part +libsamplerate and gdbm (core libraries) and LIRC (lirc module), although +others may also be included in the future. If PulseAudio is compiled with +these optional components, this effectively downgrades the license of the +server part to GPL (see the file GPL for details), exercising section 3 of +the LGPL. In such circumstances, you should treat the client library +(libpulse) of PulseAudio as being LGPL licensed and the server part (libpulsecore) as being GPL licensed. Since the PulseAudio daemon, tests, various utilities/helpers and the modules link to libpulsecore and/or the afore mentioned optional GPL dependencies they are of course also GPL licensed also diff --git a/configure.ac b/configure.ac index e75973f..cc105c1 100644 --- a/configure.ac +++ b/configure.ac @@ -1074,20 +1074,6 @@ AS_IF([test "x$enable_openssl" = "xyes" && test "x$HAVE_OPENSSL" = "x0"], AM_CONDITIONAL([HAVE_OPENSSL], [test "x$HAVE_OPENSSL" = x1]) AS_IF([test "x$HAVE_OPENSSL" = "x1"], AC_DEFINE([HAVE_OPENSSL], 1, [Have OpenSSL])) -#### FFTW (optional) #### - -AC_ARG_WITH([fftw], - AS_HELP_STRING([--without-fftw],[Omit FFTW-using modules (equalizer)])) - -AS_IF([test "x$with_fftw" != "xno"], - [PKG_CHECK_MODULES(FFTW, [ fftw3f ], HAVE_FFTW=1, HAVE_FFTW=0)], - HAVE_FFTW=0) - -AS_IF([test "x$with_fftw" = "xyes" && test "x$HAVE_FFTW" = "x0"], - [AC_MSG_ERROR([*** FFTW support not found])]) - -AM_CONDITIONAL([HAVE_FFTW], [test "x$HAVE_FFTW" = "x1"]) - #### speex (optional) #### AC_ARG_WITH([speex], @@ -1435,7 +1421,6 @@ AS_IF([test "x$HAVE_TCPWRAP" = "x1"], ENABLE_TCPWRAP=yes, ENABLE_TCPWRAP=no) AS_IF([test "x$HAVE_LIBSAMPLERATE" = "x1"], ENABLE_LIBSAMPLERATE=yes, ENABLE_LIBSAMPLERATE=no) AS_IF([test "x$HAVE_IPV6" = "x1"], ENABLE_IPV6=yes, ENABLE_IPV6=no) AS_IF([test "x$HAVE_OPENSSL" = "x1"], ENABLE_OPENSSL=yes, ENABLE_OPENSSL=no) -AS_IF([test "x$HAVE_FFTW" = "x1"], ENABLE_FFTW=yes, ENABLE_FFTW=no) AS_IF([test "x$HAVE_ORC" = "xyes"], ENABLE_ORC=yes, ENABLE_ORC=no) AS_IF([test "x$HAVE_ADRIAN_EC" = "x1"], ENABLE_ADRIAN_EC=yes, ENABLE_ADRIAN_EC=no) AS_IF([test "x$HAVE_SPEEX" = "x1"], ENABLE_SPEEX=yes, ENABLE_SPEEX=no) @@ -1491,7 +1476,6 @@ echo " Enable libsamplerate: ${ENABLE_LIBSAMPLERATE} Enable IPv6: ${ENABLE_IPV6} Enable OpenSSL (for Airtunes): ${ENABLE_OPENSSL} - Enable fftw: ${ENABLE_FFTW} Enable orc: ${ENABLE_ORC} Enable Adrian echo canceller: ${ENABLE_ADRIAN_EC} Enable speex (resampler, AEC): ${ENABLE_SPEEX} diff --git a/po/POTFILES.in b/po/POTFILES.in index f39abae..5eea106 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -28,7 +28,6 @@ src/modules/module-console-kit.c src/modules/module-default-device-restore.c src/modules/module-detect.c src/modules/module-device-restore.c -src/modules/module-equalizer-sink.c src/modules/module-esound-compat-spawnfd.c src/modules/module-esound-compat-spawnpid.c src/modules/module-esound-sink.c diff --git a/src/Makefile.am b/src/Makefile.am index 99d76ce..da02650 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -96,7 +96,6 @@ EXTRA_DIST = \ daemon/esdcompat.in \ daemon/start-pulseaudio-x11.in \ utils/padsp.in \ - utils/qpaeq \ modules/module-defs.h.m4 \ daemon/pulseaudio.desktop.in \ map-file \ @@ -1355,14 +1354,6 @@ modlibexec_LTLIBRARIES += \ endif endif -if HAVE_DBUS -if HAVE_FFTW -modlibexec_LTLIBRARIES += \ - module-equalizer-sink.la -bin_SCRIPTS += utils/qpaeq -endif -endif - # These are generated by an M4 script SYMDEF_FILES = \ module-cli-symdef.h \ @@ -1381,7 +1372,6 @@ SYMDEF_FILES = \ module-remap-sink-symdef.h \ module-remap-source-symdef.h \ module-ladspa-sink-symdef.h \ - module-equalizer-sink-symdef.h \ module-match-symdef.h \ module-tunnel-sink-new-symdef.h \ module-tunnel-source-new-symdef.h \ @@ -1648,11 +1638,6 @@ module_ladspa_sink_la_CFLAGS += $(DBUS_CFLAGS) module_ladspa_sink_la_LIBADD += $(DBUS_LIBS) endif -module_equalizer_sink_la_SOURCES = modules/module-equalizer-sink.c -module_equalizer_sink_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS) $(DBUS_CFLAGS) $(FFTW_CFLAGS) -module_equalizer_sink_la_LDFLAGS = $(MODULE_LDFLAGS) -module_equalizer_sink_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) $(FFTW_LIBS) - module_match_la_SOURCES = modules/module-match.c module_match_la_LDFLAGS = $(MODULE_LDFLAGS) module_match_la_LIBADD = $(MODULE_LIBADD) diff --git a/src/pulse/proplist.h b/src/pulse/proplist.h index e55a479..6a494dd 100644 --- a/src/pulse/proplist.h +++ b/src/pulse/proplist.h @@ -65,10 +65,10 @@ PA_C_DECL_BEGIN /** For streams: logic role of this media. One of the strings "video", "music", "game", "event", "phone", "animation", "production", "a11y", "test" */ #define PA_PROP_MEDIA_ROLE "media.role" -/** For streams: the name of a filter that is desired, e.g.\ "echo-cancel" or "equalizer-sink". PulseAudio may choose to not apply the filter if it does not make sense (for example, applying echo-cancellation on a Bluetooth headset probably does not make sense. \since 1.0 */ +/** For streams: the name of a filter that is desired, e.g.\ "echo-cancel". PulseAudio may choose to not apply the filter if it does not make sense (for example, applying echo-cancellation on a Bluetooth headset probably does not make sense. \since 1.0 */ #define PA_PROP_FILTER_WANT "filter.want" -/** For streams: the name of a filter that is desired, e.g.\ "echo-cancel" or "equalizer-sink". Differs from PA_PROP_FILTER_WANT in that it forces PulseAudio to apply the filter, regardless of whether PulseAudio thinks it makes sense to do so or not. If this is set, PA_PROP_FILTER_WANT is ignored. In other words, you almost certainly do not want to use this. \since 1.0 */ +/** For streams: the name of a filter that is desired, e.g.\ "echo-cancel". Differs from PA_PROP_FILTER_WANT in that it forces PulseAudio to apply the filter, regardless of whether PulseAudio thinks it makes sense to do so or not. If this is set, PA_PROP_FILTER_WANT is ignored. In other words, you almost certainly do not want to use this. \since 1.0 */ #define PA_PROP_FILTER_APPLY "filter.apply" /** For streams: the name of a filter that should specifically suppressed (i.e.\ overrides PA_PROP_FILTER_WANT). Useful for the times that PA_PROP_FILTER_WANT is automatically added (e.g. echo-cancellation for phone streams when $VOIP_APP does its own, internal AEC) \since 1.0 */ -- 1.8.5.2
