Re: [pulseaudio-discuss] New equalizer module (module-eqpro-sink), some questions
Tanu Kaskinen : > Thanks for contributing to this thread :) > On Mon, 2018-11-05 at 00:14 +0500, Alexander E. Patrakov wrote: > > 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. > > > > 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
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 : > > 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
Re: [pulseaudio-discuss] New equalizer module (module-eqpro-sink), some questions
Andrea A : > 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 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] New equalizer module (module-eqpro-sink), some questions
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" ha scritto: 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. 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
Re: [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. 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
