On Mon, 2018-11-05 at 00:14 +0500,
Alexander E. Patrakov wrote:
> Andrea A
<Andrea69x@xxxxxxxxxx>:
> > 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/
).
> > 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?
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).
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
