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

[Andrea A]

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.

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?

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?

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?

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?

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

Regards,
Andrea993

[Alexander E. Patrakov]

Andrea A <***@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

[Tanu Kaskinen]

On Mon, 2018-11-05 at 00:14 +0500, Alexander E. Patrakov wrote:
> Andrea A <***@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/
).

> > 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

[Andrea A]

Thanks a lot for the reply

>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.

I can write a parser for ini-style file however since PA is multiplatform I need some information about how to store user and system settings. System settings can be hardcoded at least, but the directory of user config depends on the platform I think.

>I would love to have the equalizer as a LADSPA plugin

My fear is that a LADSPA plugin will be too hard to use for a lot of desktop users. I think that a GNU desktop user would like to have a fully working audio equalizer in his distribution and PA is default in almost all GNU distributions. Configuring a LADSPA plugin may be hard and boring for the average user and GNU will continue to don't have a standard equalizer. Beyond the issues you've already listed.

> 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.

As far as I know the actual equalizer is deprecated so if this mine equalizer will be adequate I think that the actual can be substitute and the number of modules to maintain will not change.

Andrea993




________________________________
Da: pulseaudio-discuss <pulseaudio-discuss-***@lists.freedesktop.org> per conto di Tanu Kaskinen <***@iki.fi>
Inviato: martedì 6 novembre 2018 21:18
A: General PulseAudio Discussion
Oggetto: Re: [pulseaudio-discuss] New equalizer module (module-eqpro-sink), some questions

On Mon, 2018-11-05 at 00:14 +0500, Alexander E. Patrakov wrote:
> Andrea A <***@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/
).

> > 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

[Alexander E. Patrakov]

Andrea A <***@hotmail.it>:
> My fear is that a LADSPA plugin will be too hard to use for a lot of desktop users. I think that a GNU desktop user would like to have a fully working audio equalizer in his distribution and PA is default in almost all GNU distributions. Configuring a LADSPA plugin may be hard and boring for the average user and GNU will continue to don't have a standard equalizer. Beyond the issues you've already listed.

No, it won't. It will be compiled as a part of your application, and
your GUI application, at startup, would do the equivalent of "pactl
load-module module-ladspa-sink" with the correct parameters for each
existing sink. I.e. the user will only have to install your
application from a distribution package and logout/login again.

And we have a good precedent here: veromix _was_ packaged in Debian
(now it isn't because it is not up to speed with recent KDE and
GNOME), and it uses exactly the proposed architecture.

--
Alexander E. Patrakov

[Tanu Kaskinen]

On Tue, 2018-11-06 at 21:14 +0000, Andrea A wrote:
> Thanks a lot for the reply
>
> > 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.
>
> I can write a parser for ini-style file however since PA is
> multiplatform I need some information about how to store user and
> system settings. System settings can be hardcoded at least, but the
> directory of user config depends on the platform I think.

pa_open_config_file() can be used to open a file from the per-user
configuration directory, or if the file doesn't exist there, then
pa_open_config_file() will open the file from the system configuration
directory.

src/pulsecore/conf-parser.h provides an API for reading ini-style
files.

By the way, can you switch to plain text emails? With your quoting
style it's a bit hard to separate your own text from the quoted text.

> > I would love to have the equalizer as a LADSPA plugin
>
> My fear is that a LADSPA plugin will be too hard to use for a lot of
> desktop users. I think that a GNU desktop user would like to have a
> fully working audio equalizer in his distribution and PA is default
> in almost all GNU distributions. Configuring a LADSPA plugin may be
> hard and boring for the average user and GNU will continue to don't
> have a standard equalizer. Beyond the issues you've already listed.

(Alexander addressed this concern.)

> > 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.
>
> As far as I know the actual equalizer is deprecated so if this mine
> equalizer will be adequate I think that the actual can be substitute
> and the number of modules to maintain will not change.

Ok, but then I can make the argument that we have a chance to reduce
the number of filter sinks by one :)

--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk

[Jason Newton]

Just some comments on this thread, mainly towards Andrea.

I tried looking over the thesis, but Italian is not my language, and
google translate seems to give up midway through.

Certainly welcome new blood and a new project to make equalization a
better experience. Extra points for making it a masters thesis, and
doing a (qt) GUI along with it.

I think this thread shows there's some bits of work you have in front
of you for various concerns, but I think you could be weeks out from
getting signoff from Tanu and kind. When that time comes, you have my
signoff by proxy as well. I would think about profiles (as csv's,
json?) and potentially per-channel filters as features to have before
merge, but if you stay active, I wouldn't necessarily get held up on
those - I can tell you people will request them for features, though.

One of the things that hasn't changed in PA in all these years is how
to accomplish getting advanced modules like this working without
complexity both for the inside of these modules and for users. Alot
of people managed to get the old equalizer to work for them, but there
were those that didn't. That was a shared problem with the LADSPA
equalizer pathway. You also have to deal with saving states/profiles
to the tools PA gives you which can be alot of support code in what
amounts to a module that is already burdened with complex boilerplate,
and the complexity of how to make a GUI work live. An equalizer
brings up alot of issues inside of pulse's architecture, for sure. I
see alot of suggestion and some promise of going the LADSPA pathway
but its got problems, as has been alluded to - these make me recommend
not going that direction, because unless you change LADSPA to suite
pulse's needs, you are sealed to its limitations (you could modify it,
though, but it may remain kludgy overall). The value that pathway
offers is reducing the number of virtual sinks with duplicated
boilerplate, while the other proposed solutions just goes back to
things like filters and controls apis, creating complex
implementations of abstractions that are likely beyond time resources
in combination with not enough incentive to get the work done.

So my expectations are low that these problems will be dealt with
seamlessly, if ever. It's still my belief that all filter modules
should be first class modules (even if a new kind), and not LADSPA,
but the DRY violations should to be dealt with to lower maintenance
burden and even make the implementation straight forward and reduce
maintenance issues. My aim of attack would be on cracking how to
inherit and override module-virtual-sink, which is a much reduced
scope problem, and flexible - it seems like the lowest hanging fruit
to improve the situation significantly, and probably provides a
pathway to "the next step", if still needed.

I'll also note the GUI is going to be in a tricky place. Hosting it
is problem #1, it is awkward at best to host it inside the PA project,
but it will be coupled to the versions - this was one reason why I
made qpaeq a python script. It will also take a while before distros
ship another package, too, which in the short term makes things less
easy for users, especially if they have to compile - you will have to
be the catalyst that changes that. Further, everyone wants one for
their desktop environment and toolkit and seamless integration -
whatever you end up doing, think carefully on this and balance burden
with solution.

Finally, on release/merge, I would think about how to get the word
out. Google, non-unique/weak terms, and forums lead to alot of
re-circulation of confusion on this subject that dominated useful
information. It sounds funny to mention SEO, but it is relevant.

-Jason

[Alexander E. Patrakov]

Tanu Kaskinen <***@iki.fi>:
>

Thanks for contributing to this thread :)

> On Mon, 2018-11-05 at 00:14 +0500, Alexander E. Patrakov wrote:
> > Andrea A <***@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

[Andrea A]

Thanks for the reply I can try to answer to yours objections

>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.

My equalizer is my thesis and math model comes from about 70 pages of calculations. It is not possible to add them as source comments. In my thesis I have inserted and explained only relevant result, and to do only this I've spent about 40 pages.
All math is formally demonstraded so there are not bug in the math model, maybe in implementation at least (but I have done and I will do a lot of tests).
Not to brag but I've to say that I've compared the filtered sound of my equalizer with a lot of others free and commercial sw equalizers and I've not found better, in particular about selectivity of the bands, often equalizers mix the nearest bands, not mine (or at least much less than the other).
If you want I can share my thesis, it's in Italian but I think that a translator can do the rest to make it understandable.

> Also, I think it would make sense to use a struct of 10
well-named floats instead of eqp->c.

Without read the thesis or my calculations it is very hard to understand the role of struct variables, for example "c" are coefficients of the "cut boost" filter, "b" and "a" are standard names for the IIR coefficients, X is the standard name (especially in system theory) for the state, and so on but also if I use more explicative names, from the source, will be hard to understand how it works.

> rewind callback is implemented
incorrectly. The real problem is - nobody implements it correctly

It is true, now I do nothing to manage rewind, but it is not a very bad thing. I had already though to and how manage it correctly, I can revert exactly the filter state "X" storing the buffer. But I think that is not a priority issue and can be performed in future versions.

> 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

Until I die I'm here, I can help you with the math model but how I've write few lines ago the math model is formally demonstrated there are not reason to change it. Yes, it is possible to do some implementation changes, for example I've also a math model that is the same equalizer but parametric that is also interesting, but increase a lot the code complexity and the GUI complexity.

> A better alternative, in my opinion, would be to create a LADSPA
plugin instead

Yes, I can also implement a LADSPA plugin, but PA needs a new equalizer, I've found a lot of discussions about the deprecation of the current equalizer, I tried it and read its source and in my honest opinion it is a very lower level than mine, also for CPU time as well as sound quality.
Me and a lot of GNU/Linux users need a good and professional equalizer solution. My equalizer module is suitable, can be configured to normal desktop user (eg 10 bands) or to professional users, with 32 bands or more and can also respect sound equalizers standard from "american national standards institute".

I don't want to boast but I've worked on the DSP part for a long time and I've read and improved a lot of paper to realize the math model. You can try it if you want, I would like to have your feedback.

Thanks for the time, if you have any other doubts I'm happy to reply.

Andrea993

Il 04 nov 2018 20:14, "Alexander E. Patrakov" <***@gmail.com> ha scritto:

Andrea A <***@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
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-***@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[Alexander E. Patrakov]

Andrea A <***@hotmail.it>:

> My equalizer is my thesis and math model comes from about 70 pages of calculations. It is not possible to add them as source comments. In my thesis I have inserted and explained only relevant result, and to do only this I've spent about 40 pages.

Then please publish your thesis online somewhere as a pdf and insert a
link to it in the source code as a comment.

P.S. in some countries, including Russia, such publication is required anyway.

--
Alexander E. Patrakov

[Andrea A]

I’ve just added the thesis link at the end of the first module comment, I can change its position if you want:

https://github.com/andrea993/audioeqpro/blob/afb986711f5125083351cc4f7dac1ca84409f501/pulsemodule/module-eqpro-sink.c#L21

Regards,
Andrea993
Da: Alexander E. Patrakov<mailto:***@gmail.com>
Inviato: lunedì 5 novembre 2018 03:02
A: General PulseAudio Discussion<mailto:pulseaudio-***@lists.freedesktop.org>
Oggetto: Re: [pulseaudio-discuss] New equalizer module (module-eqpro-sink), some questions

Andrea A <***@hotmail.it>:

> My equalizer is my thesis and math model comes from about 70 pages of calculations. It is not possible to add them as source comments. In my thesis I have inserted and explained only relevant result, and to do only this I've spent about 40 pages.

Then please publish your thesis online somewhere as a pdf and insert a
link to it in the source code as a comment.

P.S. in some countries, including Russia, such publication is required anyway.

--
Alexander E. Patrakov

[Alexander E. Patrakov]

Andrea A <***@hotmail.it>:
>
> I’ve just added the thesis link at the end of the first module comment, I can change its position if you want:
>
> https://github.com/andrea993/audioeqpro/blob/afb986711f5125083351cc4f7dac1ca84409f501/pulsemodule/module-eqpro-sink.c#L21

Thank you very much, I will look. It will be slower than usual because
I will have to use Google Translate.

--
Alexander E. Patrakov

[Andrea A]

You are welcome, if I will have free time I will try to translate important parts.

> - 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)

How can I limit the latency?


Andrea993

________________________________
Da: pulseaudio-discuss <pulseaudio-discuss-***@lists.freedesktop.org> per conto di Alexander E. Patrakov <***@gmail.com>
Inviato: lunedì 5 novembre 2018 22:46
A: General PulseAudio Discussion
Oggetto: Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions

Andrea A <***@hotmail.it>:
>
> I’ve just added the thesis link at the end of the first module comment, I can change its position if you want:
>
> https://github.com/andrea993/audioeqpro/blob/afb986711f5125083351cc4f7dac1ca84409f501/pulsemodule/module-eqpro-sink.c#L21

Thank you very much, I will look. It will be slower than usual because
I will have to use Google Translate.

--
Alexander E. Patrakov

[Tanu Kaskinen]

On Tue, 2018-11-06 at 12:29 +0000, Andrea A wrote:
> You are welcome, if I will have free time I will try to translate important parts.
>
> > - 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)
>
> How can I limit the latency?

This is how I imagine it should be done, not guaranteed to be 100%
correct:

The pa_sink_set_latency_range_within_thread() calls should set the
maximum latency to the desired limit, unless the master sink's
min_latency is higher than the limit, in which case the master sink's
min_latency should be used.

sink_update_requested_latency_cb() should first check what
pa_sink_get_requested_latency_within_thread(s) returns. If it's -1 or
higher than s->thread_info.max_latency, then the requested latency
should be set to s->thread_info.max_latency.

--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk