[pulseaudio-discuss] New equalizer module (module-eqpro-sink), some questions

Sun Nov 4 19:14:39 UTC 2018

Andrea A <Andrea69x at hotmail.it>:
>
> I'm writing a new equalizer module for PA,
> https://github.com/andrea993/audioeqpro/blob/master/pulsemodule/module-eqpro-sink.c
> I've almost done but I need some information from developer about how to proceed.

Thanks for attempting a contribution. I have attempted to answer your
questions regarding the integration, please read below. However, see
the end of this email for the biggest reason why I am against
accepting this module or any future form of it (but my "no" holds very
little weight, so feel free to ignore it).

However, in order for the module to be accepted (barring the big
objection at the bottom of this email), we need to review the DSP
part, and not just the integration part. It would help if you provide,
in the form of comments in the source code, some references where the
math comes from. And use more descriptive variable names, such as K ->
extra_gain. Also, I think it would make sense to use a struct of 10
well-named floats instead of eqp->c.

> First of all, I see that current equalizer module manages "autoloaded" have I to manage it? What it does exactly? Old equalizer check "autoloaded" state in a callback "may_move_to", what is it? Have I to implement this callback and manage "autoloaded" like the current equalizer module?

It is set by module_filter_apply. The intended effect is to prevent
moving the output of the equalizer to a different sink - i.e. if it
was autoloaded for "Built-in Audio Analog Stereo" then you cannot move
it to "HDMI Digital Stereo" using pavucontrol. See
module-virtual-surround-sink.c for known-good usage. Although, I don't
know any user of module_filter_apply.

Regarding the may_move_to callback, it is called when a user tries to
move the equalizer output to a different sink. Please at least prevent
creating a loop, i.e. moving the output to the equalizer itself.

> After the "autoloaded" management I can send the equalizer as a patch, however I've some questions about how to add my equalizer GUI to the PA branch. Should the GUI remains an external program or the GUI will be inserted to the mainline sources? In the second scenario how the GUI should be inserted? Which is the correct directory in the sources tree and what about the GUI makefile and the GUI installation directory?

PulseAudio currently does not depend on any GUI toolkit (well, except
the old equalizer GUI). Personally, I am fine with this GUI (or maybe
two GUIs - one for GNOME and MATE and XFCE, one for KDE) being in
external repositories.

> The equalizer needs the messages patches from George Chini
> https://patchwork.freedesktop.org/series/41390/
> Have I to write this information as a patch comment or other?

Patch comment.

> I would like to add some configuration files to my module, for example to load and store equalizer preset, is there a PA specific file format (and directory to store file) to do this?

There are database files in ~/.config/pulse. There are multiple
backends supported, see the --with-database=... configure argument.
The abstraction layer is in src/pulsecore/database.h. Not sure if this
is suitable for your needs.

> Execuse me for the wall of questions and thanks in advance.

You are welcome.

Anyway, just a small nitpick: the rewind callback is implemented
incorrectly. The real problem is - nobody implements it correctly,
especially because the comment in the template module-virtual-sink.c
suggest doing such a stupid thing as resetting the filter. And, at
least for the case of a resampler, users other than me do notice the
imperfection, see https://bugs.freedesktop.org/show_bug.cgi?id=50113 .
There are two solutions that I would accept as "proper". 1 - store the
history of your input and/or state, restore it when asked to rewind. 2
- do not pretend to support rewinds (but in this case, please limit
the latency to something like 20-30 ms, so hat PulseAudio reacts
quickly to the new streams). In the name of simplicity, and because
the power-saving argument behind the original rewind operation does
not hold if there is non-trivial processing, I would prefer option 2.

Big warning: I have not tested the module.

And here at the bottom of the email, let me explain why I think
keeping this module outside of PulseAudio, in a different form, may be
a better idea.

The reason is that, by accepting this module, we are implicitly taking
the responsibility to support it inside the tree. And, you are the
best person to support it. So there is an additional (avoidable!)
latency between the time when you develop improvements and the time
when users see them: namely, the time for someone else to understand
and review your code, for PulseAudio team to make a release, and for
distributions to package it.

A better alternative, in my opinion, would be to create a LADSPA
plugin instead. PulseAudio already has module-lasdpa-sink since ages
(even with D-Bus interface to change parameters at runtime), so your
filter will be available also to all users of old PulseAudio versions.
It will be also available to users of pure ALSA-based setup, if they
still exist. You can publish improvements any time you want, without
needing any potentially slow review from PulseAudio maintainers (but
feel free to contact me privately if you do need a review), and your
module will be quick to compile, because it is separated from the rest
of PulseAudio. You can then publish a GUI application that loads the
module into PulseAudio and then controls its filter via D-Bus. And you
don't need to care about rewinds and may_move_to and all other
pulseaudio-specific boilerplate. Sounds like a win-win situation.
Could you please investigate this approach?

Thanks again for attempting to replace the current equalizer with
something better.

-- 
Alexander E. Patrakov