[pulseaudio-discuss] [PATCH] Warn on attempts to build and load module-equalizer-sink

[Jason Newton]

Hm, how do you feel about leaving it default on?  I don't want people
on ubuntu or other distros who are using it (and they exist!) being
suddenly islanded again like they have been repeatedly with the other
ladspa equalizer frontends.  I think that would lesson those users'
opinion of pulse.  If you snoop around you'll unfortunately find large
numbers of repeating threads with people confused about how to get an
equalizer and breakage of those has been a frequent and painful thing
on distribution update - these people seem to get exactly one of these
equalizers (including module-equalizer-sink) working due to the
complexity of module configuration then use that for the rest of time
until it stops working for them.  Definitely more people have ended up
using the ladspa based sinks due to some unfortunate naming and
confusion of users but I'd say probably at least hundreds are using
module-equalizer-sink (across gentoo, arch, ubuntu, and opensuse) and
a lower ratio of people have problems with it due to the unmaintained
ladspa frontends falling out of favour with packagers or runtime
issues - but that's just based on surveying forum threads over the
years - I have no hard evidence of this.

I don't think I can get enough time by 5.0 but maybe I can sneak in
some small fixes before or just after such as correcting the phase
issue we mentioned previously and the non-alsa sink crash.  I'd be
pushing it promising more than that for now and re-working the
buffering would need to come after studying how buffering is working
at runtime (with things like flash) and other things would follow
after that including refining other dsp issues and making latency
/filter resolution changeable via parameter to make the latency less
noticable.

For movies by the way, work arounds to compensate for the latency for
mplayer like media players - the -delay,audio-delay allows specifying
a value one can put in their media player configuration.  I've used
this in the past for mistimed audio-video (off by several seconds, not
the equalizer, I assure you ;-) and can be tweaked while watching
media via keyboard to find the magic value.  Not that we shouldn't try
choosing a less noticeable number but the latency is always there and
this might be the best way of dealing with it for the user who wants
to know there is 0 perceived latency (to the best scheduling allows).
I will add a note on that bug entry in a bit.

-Jason

On Mon, Dec 1, 2014 at 4:14 PM, Alexander E. Patrakov
<patrakov at gmail.com> wrote:
> Also, don't build it by default.
>
> Signed-off-by: Alexander E. Patrakov <patrakov at gmail.com>
> ---
>  configure.ac                        | 24 +++++++++++++++++++++---
>  src/modules/module-equalizer-sink.c |  4 ++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index fe8423f..930593b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1113,9 +1113,9 @@ 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_HELP_STRING([--with-fftw],[Build FFTW-using module (equalizer). Note: the module has known-wrong DSP logic]))
>
> -AS_IF([test "x$with_fftw" != "xno"],
> +AS_IF([test "x$with_fftw" = "xyes"],
>      [PKG_CHECK_MODULES(FFTW, [ fftw3f ], HAVE_FFTW=1, HAVE_FFTW=0)],
>      HAVE_FFTW=0)
>
> @@ -1526,7 +1526,7 @@ AS_IF([test "x$HAVE_TCPWRAP" = "x1"], ENABLE_TCPWRAP=yes, ENABLE_TCPWRAP=no)
>  AS_IF([test "x$HAVE_LIBSAMPLERATE" = "x1"], ENABLE_LIBSAMPLERATE="yes (DEPRECATED)", 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_FFTW" = "x1"], ENABLE_FFTW="yes (BAD IDEA)", 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)
> @@ -1647,3 +1647,21 @@ part of PulseAudio on that platform.
>  ===== WARNING WARNING WARNING WARNING WARNING WARNING WARNING =====
>  "
>  fi
> +
> +if test "${ENABLE_FFTW}" != "no" ; then
> +    echo "
> +===== WARNING WARNING WARNING WARNING WARNING WARNING WARNING =====
> +You have fftw support enabled. fftw is used only by
> +module-equalizer-sink, which has many bugs, including wrong DSP
> +logic. See details at:
> +
> +http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-March/020174.html
> +https://bugs.freedesktop.org/show_bug.cgi?id=41465
> +https://bugs.freedesktop.org/show_bug.cgi?id=54881
> +https://bugs.freedesktop.org/show_bug.cgi?id=69229
> +
> +It is suggested that you reconsider, unless you are a DSP
> +specialist willing to fix or rewrite the code and submit patches.
> +===== WARNING WARNING WARNING WARNING WARNING WARNING WARNING =====
> +"
> +fi
> diff --git a/src/modules/module-equalizer-sink.c b/src/modules/module-equalizer-sink.c
> index 811bbc2..8963e36 100644
> --- a/src/modules/module-equalizer-sink.c
> +++ b/src/modules/module-equalizer-sink.c
> @@ -1096,6 +1096,10 @@ int pa__init(pa_module*m) {
>
>      pa_assert(m);
>
> +    pa_log_warn("A number of serious DSP issues have been found in module-equalizer-sink.");
> +    pa_log_warn("See http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-March/020174.html");
> +    pa_log_warn("Patches (or a rewrite) from DSP specialists are welcome.");
> +
>      if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
>          pa_log("Failed to parse module arguments.");
>          goto fail;
> --
> 2.1.3
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss