[pulseaudio-discuss] [PATCH] Remove module-equalizer-sink
Alexander E. Patrakov
patrakov at gmail.com
Fri Mar 7 11:43:34 PST 2014
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
More information about the pulseaudio-discuss
mailing list