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

Wed Nov 7 02:58:10 UTC 2018

Tanu Kaskinen <tanuk at iki.fi>:
>

Thanks for contributing to this thread :)

> On Mon, 2018-11-05 at 00:14 +0500, Alexander E. Patrakov wrote:
> > 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.
>
> There's no need to worry about loops, pa_sink_input_may_move_to()
> already checks that (except loops built using module-loopback aren't
> checked, but Andrea probably isn't going to solve that problem anyway,
> or if he is, it's better to solve that in pa_sink_input_move_to()
> rather than in individual modules).
>
> > > 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.
>
> GUIs should go to external repositories. qpaeq is an exception, and
> probably not that well justified exception. One reason qpaeq made its
> way to the main pulseaudio repo was that it's just a simple python
> script that doesn't need much support from the build system.
>
> > > 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'm not sure what "patch comment" means, but the information doesn't
> belong in the commit message. If the module is submitted as a merge
> request in GitLab, the information can be written to the merge request
> description or added as a separate comment in the discussion section.
> If the module is submitted via email, the information can be added
> below the "---" line in the patch (this stuff is explained at
> https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
> ).

The stuff below the "---" line is what I meant. I am not entirely
comfortable with GitLab merge requests, but that's a problem with me,
not with GitLab. And in this case a comment on the merge request would
definitely work.

> > > 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.
>
> If the preset files are expected to be shared between users, then the
> database.h stuff isn't good, because different users can have their
> pulseaudio configured with different database formats. I like the "ini-
> style" configuration file style that pulseaudio uses for .conf files.
> There are no helpers for writing those files, though, but that's
> probably not a big issue.
>
> You mentioned presets only as an example, do you have other kinds of
> configuration in mind? I'd expect the module arguments to provide all
> the necessary configuration.
>
> > > 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.
>
> In the name of simplicity, I'd strongly 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?
>
> I would love to have the equalizer as a LADSPA plugin rather than yet
> another separate filter sink. It's not very uncommon that some core
> change requires changes in all sinks, so even if the module is perfect
> and doesn't require maintenance in form of bug fixes, there are other
> kinds of real maintenance costs.
>
> The LADSPA plugin approach isn't without issues, however:
>
> LADSPA doesn't seem to support parametrized plugin instantiation,
> meaning that the number of bands needs to be fixed. This can be
> mitigated by creating a few separate plugins with different band
> counts, but that of course can't scale to support arbitrary band
> counts. But maybe a few common cases is good enough?

Actually at one of the past GUADECs in Sweden we have discussed this
with Arun, and his opinion is to have a fixed number of bands, "the
same that you have in your smart TV, because this way we won't deviate
much from something already checked by usability experts". For me,
that's 5 bands, with center frequencies on 0.1, 0.3, 1, 3 and 10 kHz.

> There is a D-Bus interface for changing the parameters, but it's rather
> limited. It may very well be enough for a specialized GUI that knows
> exactly what plugin it's dealing with, though. But the D-Bus protocol
> has some serious issues, such as having no API stability guarantees. I
> think the D-Bus protocol can be regarded kind of deprecated, it's not
> likely to ever become the preferred protocol as originally envisioned.
> Implementing a control interface using the message API from Georg for
> the LADSPA sink would be an awesome contribution in itself, especially
> if it's more complete than the D-Bus one. "More complete" means
> supporting introspection: applications should be able to enumerate the
> LADSPA sinks, find out which plugins they have loaded (including label
> and name information) and what controls the plugins have (including the
> control name and type).

I agree here.

-- 
Alexander E. Patrakov