Tanu Kaskinen <tanuk@xxxxxx>: > Thanks for contributing to this thread :) > 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/ > ). 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 _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
