Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 30.04.19 23:37, Alexander E. Patrakov wrote: вт, 30 апр. 2019 г. в 23:46, Georg Chini : On 30.04.19 19:23, Alexander E. Patrakov wrote: Please don't treat the FIR for module-virtual-surround-sink as a set of reconfigurable parameters. At least for now. Just hard-code the contents of http://stuff.salscheider-online.de/hrir_kemar.tar.gz or find a redistributable equivalent, preferably with more than 6 channels. Too late ... I know it is not a normal parameter comparable to the parameters of other filters, but the pulseaudio module page suggests to experiment with different files until you find the right one. To do so, I think it is more convenient if you can replace the file on the fly instead of having to unload and reload the module. Also, the code is intended as a demonstration of how to replace the parameter set by a new one (while the ladspa-sink demonstrates how to re-use the memory.) ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
вт, 30 апр. 2019 г. в 23:46, Georg Chini : > > On 30.04.19 19:23, Alexander E. Patrakov wrote: > >> /* If set, this function is called in thread context when an update > >> of the > >>* filter parameters is requested, may be NULL. The function must > >> replace > >>* the currently used parameter structure by the new structure and > >> return > >>* a pointer to the old structure so that it can be freed in the > >> main thread > >>* using parameter_free(). If the old structure can be re-used, the > >> function > >>* may return NULL. */ > >> void *(*update_filter_parameters)(void *parameters, void > >> *filter_handle); > > I don't think this is implementable. How are the parameters supposed > > to be initially allocated? > > > Why should this not be possible to implement? Meanwhile I have > it already implemented within the new virtual sink library and > I am using it for the virtual-surround-sink and also for > the ladspa-sink. Setting and querying parameters works > perfectly. For the virtual-surround-sink I allocate new memory > each time, while for the ladspa-sink I prepare a parameter set > in the main thread and copy it to the "real" parameter set > in the IO-thread. Please don't treat the FIR for module-virtual-surround-sink as a set of reconfigurable parameters. At least for now. Just hard-code the contents of http://stuff.salscheider-online.de/hrir_kemar.tar.gz or find a redistributable equivalent, preferably with more than 6 channels. > The initial parameter set must be allocated when the > filter is initialized. The parameter_set_all() function can be > used to do that or the filter_init() function could create a > default parameter set. OK, I retract the "not implementable" objection. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 30.04.19 20:46, Georg Chini wrote: On 30.04.19 19:23, Alexander E. Patrakov wrote: /* Callback to process a chunk of data by the filter. May be NULL */ void (*process_chunk)(float *src, float *dst, unsigned count, void *filter_handle); /* Callback to retrieve additional latency caused by the filter. May be NULL */ uint64_t (*get_extra_latency)(void *filter_handle); /* Initializes a new filter instance and returns a handle to it. */ void *(*init_filter)(unsigned input_channels, unsigned output_channels, unsigned sample_rate); /* Deletes filter instance. */ void (*exit_filter)(void *filter_handle); /* If set, this function is called in thread context when an update of the * filter parameters is requested, may be NULL. The function must replace * the currently used parameter structure by the new structure and return * a pointer to the old structure so that it can be freed in the main thread * using parameter_free(). If the old structure can be re-used, the function * may return NULL. */ void *(*update_filter_parameters)(void *parameters, void *filter_handle); I don't think this is implementable. How are the parameters supposed to be initially allocated? Why should this not be possible to implement? Meanwhile I have it already implemented within the new virtual sink library and I am using it for the virtual-surround-sink and also for the ladspa-sink. Setting and querying parameters works perfectly. For the virtual-surround-sink I allocate new memory each time, while for the ladspa-sink I prepare a parameter set in the main thread and copy it to the "real" parameter set in the IO-thread. The initial parameter set must be allocated when the filter is initialized. The parameter_set_all() function can be used to do that or the filter_init() function could create a default parameter set. To make it clear: The proposal for an external interface comes not out of the blue - I have all the bits already working for the virtual sinks. That is module-virtual-sink, module-virtual-surround-sink, module-remap-sink and module-ladspa-sink are fully converted to the internal version of the interface. For module-equalizer-sink I did not implement parameter changing because I could not even figure out what the parameters are, but otherwise it also uses the common infrastructure. Only module-echo-cancel is a special case but still partly uses the virtual sink library. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 30.04.19 19:23, Alexander E. Patrakov wrote: вт, 30 апр. 2019 г. в 16:31, Georg Chini : uint64_t min_latency; /* Minimum latency allowed for the sink, 0 if unused */ uint64_t max_latency; /* Maximum latency allowed for the sink, 0 if unused */ Why would these fields be needed? I simply followed what is currently used by the filters that are already implemented in PA. The maximum latency field was intended to be used if filters can't implement rewind and therefore have to limit the maximum latency. If we always disable rewinding, it still may be useful if you can specify an upper/lower limit, but in this case the variables could be dropped. bool disable_rewind; /* True when rewinding is disabled */ I'd say get rid of it either. It's too hard to implement rewinds correctly. Even in the simple cross-over filter that PulseAudio uses for LFE remixing, it took four tries to reach at something that doesn't produce clicks when the user e.g. changes the volume or otherwise triggers a rewind. Also, let's recollect the original purpose of the rewinds. They are, in the end, a way to save power. That is, to use a very long interval between wakeups if something non-interactive (like a music player) is doing its job, without the side effect of slow reaction to events like "new sink input wants to play something urgent". Please see my Linux Audio Conference presentation and video for more details: Paper: http://lac.linuxaudio.org/2015/papers/10.pdf Slides: http://lac.linuxaudio.org/2015/download/rewind-slides.pdf Video: http://lac.linuxaudio.org/2015/video.php?id=8 The main consideration here is that almost all filters are CPU-hungry enough to mask (i.e. make irrelevant) the savings obtained by dynamic latency and rewinds. So why give the filter implementer even an option to shoot themselves in the foot? I would not mind dropping this. /* Callback to reset the filter after rewind. May be NULL */ void (*rewind_filter)(void *filter_handle, size_t amount); Don't say "reset" here. Reset is not a proper way to handle a rewind, users do notice the resulting clicks, see https://bugs.freedesktop.org/show_bug.cgi?id=50113 (note: this is NOT reported by me, there are real users who hear and care about this imperfection!) Please make it absolutely clear that the filter state must not be reset, but seamlessly restored to the specified point in the past (which is the filter's responsibility to keep). If one can't do that, they shouldn't say that they support rewinds. And yes, I understand that we have no filters (other than the trivial one) which handle this correctly. That's actually one of the reasons why I am so opposed to supporting rewindable filters. One more comment - the "void" return type means that the filter must be able to rewind itself no matter what, i.e. that failure is not possible and not allowed. Assuming that my hint to stop pretending to support rewinds gets ignored, I'd say that's a good thing. If we drop disable_rewinding, this one can be dropped as well. Otherwise you are right, and I should not say reset. /* Callback to process a chunk of data by the filter. May be NULL */ void (*process_chunk)(float *src, float *dst, unsigned count, void *filter_handle); /* Callback to retrieve additional latency caused by the filter. May be NULL */ uint64_t (*get_extra_latency)(void *filter_handle); /* Initializes a new filter instance and returns a handle to it. */ void *(*init_filter)(unsigned input_channels, unsigned output_channels, unsigned sample_rate); /* Deletes filter instance. */ void (*exit_filter)(void *filter_handle); /* If set, this function is called in thread context when an update of the * filter parameters is requested, may be NULL. The function must replace * the currently used parameter structure by the new structure and return * a pointer to the old structure so that it can be freed in the main thread * using parameter_free(). If the old structure can be re-used, the function * may return NULL. */ void *(*update_filter_parameters)(void *parameters, void *filter_handle); I don't think this is implementable. How are the parameters supposed to be initially allocated? Why should this not be possible to implement? Meanwhile I have it already implemented within the new virtual sink library and I am using it for the virtual-surround-sink and also for the ladspa-sink. Setting and querying parameters works perfectly. For the virtual-surround-sink I allocate new memory each time, while for the ladspa-sink I prepare a parameter set in the main thread and copy it to the "real" parameter set in the IO-thread. The initial parameter set must be allocated when the filter is initialized. The parameter_set_all() function can be used to do that or the filter_init() function could create a default
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
вт, 30 апр. 2019 г. в 16:31, Georg Chini : > uint64_t min_latency; /* Minimum latency > allowed for the sink, 0 if unused */ > uint64_t max_latency; /* Maximum latency > allowed for the sink, 0 if unused */ Why would these fields be needed? > bool disable_rewind; /* True when rewinding is > disabled */ I'd say get rid of it either. It's too hard to implement rewinds correctly. Even in the simple cross-over filter that PulseAudio uses for LFE remixing, it took four tries to reach at something that doesn't produce clicks when the user e.g. changes the volume or otherwise triggers a rewind. Also, let's recollect the original purpose of the rewinds. They are, in the end, a way to save power. That is, to use a very long interval between wakeups if something non-interactive (like a music player) is doing its job, without the side effect of slow reaction to events like "new sink input wants to play something urgent". Please see my Linux Audio Conference presentation and video for more details: Paper: http://lac.linuxaudio.org/2015/papers/10.pdf Slides: http://lac.linuxaudio.org/2015/download/rewind-slides.pdf Video: http://lac.linuxaudio.org/2015/video.php?id=8 The main consideration here is that almost all filters are CPU-hungry enough to mask (i.e. make irrelevant) the savings obtained by dynamic latency and rewinds. So why give the filter implementer even an option to shoot themselves in the foot? > > /* Callback to reset the filter after rewind. May be NULL */ > void (*rewind_filter)(void *filter_handle, size_t amount); Don't say "reset" here. Reset is not a proper way to handle a rewind, users do notice the resulting clicks, see https://bugs.freedesktop.org/show_bug.cgi?id=50113 (note: this is NOT reported by me, there are real users who hear and care about this imperfection!) Please make it absolutely clear that the filter state must not be reset, but seamlessly restored to the specified point in the past (which is the filter's responsibility to keep). If one can't do that, they shouldn't say that they support rewinds. And yes, I understand that we have no filters (other than the trivial one) which handle this correctly. That's actually one of the reasons why I am so opposed to supporting rewindable filters. One more comment - the "void" return type means that the filter must be able to rewind itself no matter what, i.e. that failure is not possible and not allowed. Assuming that my hint to stop pretending to support rewinds gets ignored, I'd say that's a good thing. > > /* Callback to process a chunk of data by the filter. May be NULL */ > void (*process_chunk)(float *src, float *dst, unsigned count, void > *filter_handle); > > /* Callback to retrieve additional latency caused by the filter. > May be NULL */ > uint64_t (*get_extra_latency)(void *filter_handle); > > /* Initializes a new filter instance and returns a handle to it. */ > void *(*init_filter)(unsigned input_channels, unsigned > output_channels, unsigned sample_rate); > > /* Deletes filter instance. */ > void (*exit_filter)(void *filter_handle); > > /* If set, this function is called in thread context when an update > of the > * filter parameters is requested, may be NULL. The function must > replace > * the currently used parameter structure by the new structure and > return > * a pointer to the old structure so that it can be freed in the > main thread > * using parameter_free(). If the old structure can be re-used, the > function > * may return NULL. */ > void *(*update_filter_parameters)(void *parameters, void > *filter_handle); I don't think this is implementable. How are the parameters supposed to be initially allocated? -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 27.04.19 17:14, Georg Chini wrote:
On 27.04.19 12:55, Georg Chini wrote:
On 27.04.19 12:04, Tanu Kaskinen wrote:
On Fri, 2019-04-26 at 11:40 +0200, Georg Chini wrote:
On 26.04.19 10:56, Tanu Kaskinen wrote:
On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
The current scheme for
updating
parameters I have in mind should work for any of the existing
filters
and relies on
passing around parameter structures:
Based on my implementation, the interface to a PA external plugin could
look like this:
unsigned input_channels; /* Number of audio input
channels, 0 if variable */
unsigned output_channels; /* Number of audio output
channels, 0 if variable */
const char *desc_head; /* Leading part of
description string */
const char *filter_type; /* Name for the type of
filter */
size_t max_chunk_size; /* Maximum chunk size in
bytes that the filter will
* accept, 0 for default */
size_t fixed_block_size; /* Block size in frames
for fixed block size filters,
* 0 if block size is
variable */
uint64_t min_latency; /* Minimum latency
allowed for the sink, 0 if unused */
uint64_t max_latency; /* Maximum latency
allowed for the sink, 0 if unused */
bool disable_rewind; /* True when rewinding is
disabled */
/* Callback to reset the filter after rewind. May be NULL */
void (*rewind_filter)(void *filter_handle, size_t amount);
/* Callback to process a chunk of data by the filter. May be NULL */
void (*process_chunk)(float *src, float *dst, unsigned count, void
*filter_handle);
/* Callback to retrieve additional latency caused by the filter.
May be NULL */
uint64_t (*get_extra_latency)(void *filter_handle);
/* Initializes a new filter instance and returns a handle to it. */
void *(*init_filter)(unsigned input_channels, unsigned
output_channels, unsigned sample_rate);
/* Deletes filter instance. */
void (*exit_filter)(void *filter_handle);
/* If set, this function is called in thread context when an update
of the
* filter parameters is requested, may be NULL. The function must
replace
* the currently used parameter structure by the new structure and
return
* a pointer to the old structure so that it can be freed in the
main thread
* using parameter_free(). If the old structure can be re-used, the
function
* may return NULL. */
void *(*update_filter_parameters)(void *parameters, void
*filter_handle);
/* Frees a parameter structure. May only be NULL, if
update_filter_parameters()
* is also NULL or if update_filter_parameters() always returns
NULL. */
void (*parameter_free)(void *parameters);
/* The following functions can be provided to set and get
parameters. The parameter
* structure is defined by the filter and should contain all
parameters that are
* configurable by the user. The library code makes no assumption
on the contents
* of the structure but only passes references. The library
implements a message
* handler which supports the following messages that use the
functions below:
* parameter-get - Retrieve a single parameter.
* parameter-set - Set a single parameter.
* parameter-get-all - Retrieve all parameters as a list in message
format.
* parameter-set-all - Set all parameters simultaneously.
* parameter-get-description - Returns a filter description and a
list that describes
* all parameters. Example of the list
element format:
* {{Identifier}{Type}{default}{min}{max}}
* The last message can be used to get information about the filter
or to implement
* a filter control GUI that is independent of the filter type.
* If filter_message_handler() is defined, all other messages are
passed on to the
* filter. The get functions are expected to return a string in the
message handler
* format while the set functions receive plain strings. On
failure, all get/set
* functions will return NULL. */
/* Get the value of the parameter described by identifier. The
value shall be returned
* as a string enclosed in double curly braces. The identifier may
be any string that
* the filter recognizes like a name or index, depending of the
implementation of this
* function. */
char *(*parameter_get)(const char *identifier, void *filter_handle);
/* Sets the value of the parameter described by identifier. The
value is expected as
* a string. The identifier may be any string that the filter
recognizes like a name
* or index. Returns a parameter structure filled with all current
parameter values,
* reflecting the updated parameter,
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 27.04.19 12:55, Georg Chini wrote:
On 27.04.19 12:04, Tanu Kaskinen wrote:
On Fri, 2019-04-26 at 11:40 +0200, Georg Chini wrote:
On 26.04.19 10:56, Tanu Kaskinen wrote:
On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
The current scheme for
updating
parameters I have in mind should work for any of the existing filters
and relies on
passing around parameter structures:
After implementation, the actual interface looks like this:
/* If set, this function is called in thread context when an update
of the
* filter parameters is requested, may be NULL. The function must
replace
* the currently used parameter structure by the new structure and
return
* a pointer to the old structure so that it can be freed. If the old
* structure can be re-used, the function may return NULL. */
void *(*update_filter_parameters)(void *parameters, void *userdata);
/* Frees a parameter structure. May only be NULL, if
update_filter_parameters()
* is also NULL or if update_filter_parameters() always returns
NULL. */
void (*parameter_free)(void *parameters);
/* The following functions can be provided to set and get
parameters. The parameter
* structure is defined by the filter and should contain all
parameters that are
* configurable by the user. The library code makes no assumption
on the contents
* of the structure but only passes references. The library
implements a message
* handler which supports the following messages that use the
functions below:
* parameter-get - Retrieve a single parameter.
* parameter-set - Set a single parameter.
* parameter-get-all - Retrieve all parameters as a comma separated
list.
* parameter-set-all - Set all parameters simultaneously.
* parameter-get-description - Returns a filter description and a
list that describes
* all parameters. Format of the list
elements is:
* {{Identifier}{Type}{default}{min}{max}}
* The last message can be used to get information about the filter
or to implement
* a filter control GUI that is independent of the filter type.
* If filter_message_handler() is defined, all other messages are
passed on to the
* filter. The get functions are expected to return a string in the
message handler
* format while the set functions receive plain strings. */
/* Get the value of the parameter described by identifier. The
value shall be returned
* as a string in value. The identifier may be any string that the
filter recognizes
* like a name or index, depending of the implementation of this
function. */
int (*parameter_get)(const char *identifier, char **value, void
*userdata);
/* Sets the value of the parameter described by identifier. The
value is expected as
* a string. The identifier may be any string that the filter
recognizes like a name
* or index. Returns a parameter structure filled with all current
parameter values,
* reflecting the updated parameter, so that the structure can be
passed to
* update_filter_parameters(). update_filter_parameters() will
replace the current
* parameter set with the new structure. */
void *(*parameter_set)(const char *identifier, const char *value,
void *userdata);
/* Returns a list of the values of all filter parameters. Each
parameter must be enclosed
* in curly braces and there must be braces around the whole list. */
char *(*parameter_get_all)(void *userdata);
/* Expects an array of all filter parameter values as strings and
returns a parameter
* structure that can be passed to update_filter_parameters(). See
set_parameter(). */
void *(*parameter_set_all)(const char **all_params, int
param_count, void *userdata);
/* Returns a parameter description string as described above. */
char *(*parameter_get_description)(void *userdata);
/* Handler for additional messages. */
int (*filter_message_handler)(const char *message, char
*message_parameters, char **response, void *userdata);
It is not fixed yet what parameter_get_description() will exactly
return, so the list
above has to be viewed as an example. The set functions receive plain
strings
so that they need not be able to parse message handler parameter strings.
On the other hand, the get functions should return the result in the message
handler format.
Parameter sets can either be re-used or freshly allocated each time a
parameter
changes. If the update_filter_parameters() function returns a non-NULL
value,
the pointer will be freed in the main thread using the parameter_free()
function.
Does the interface look OK? Any suggestions?
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 27.04.19 12:04, Tanu Kaskinen wrote:
On Fri, 2019-04-26 at 11:40 +0200, Georg Chini wrote:
On 26.04.19 10:56, Tanu Kaskinen wrote:
On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
The current scheme for
updating
parameters I have in mind should work for any of the existing filters
and relies on
passing around parameter structures:
/* The following functions can be provided to set and get
parameters. The parameter
* structure is defined by the filter and should contain all
parameters that are
* configurable by the user. The library code makes no assumption
on the contents
* of the structure but only passes references. The library
implements a message
* handler which supports the following messages that use the
functions below:
* get_parameter - Retrieve a single parameter.
* set_parameter - Set a single parameter.
* get_all_parameters - Retrieve all parameters as a comma
separated list.
* set_all_parameters - Set all parameters simultaneously.
* get_parameter_description - Returns a list that describes all
parameters.
* Format of the list elements is:
* {{Identifier}{Type}{default}{min}{max}}
* The last message can be used to get information about the filter
or to implement
* a filter control GUI that is independent of the filter type. */
/* Get the value of the parameter described by identifier. The
value shall be returned
* as a string in value. The identifier may be any string that the
filter recognizes
* like a name or index, depending of the implementation of this
function. */
int (*parameter_get)(const char *identifier, char **value, void
*userdata);
/* Sets the value of the parameter described by identifier. The
value is expected as
* a string. The identifier may be any string that the filter
recognizes like a name
* or index. Returns a parameter structure filled with all current
parameter values,
* reflecting the updated parameter, so that the structure can be
passed to
* update_filter_parameter(). update_filter_parameter() will
replace the current
* parameter set with the new structure. */
void *(*parameter_set)(const char *identifier, const char *value,
void *userdata);
/* Returns a comma separated list of the values of all filter
parameters. */
char *(*parameter_get_all)(void *userdata);
/* Expects a comma separated list of all filter parameter values
and returns a parameter
* structure that can be passed to update_filter_parameters(). See
set_parameter(). */
void *(*parameter_set_all(const char *all_params, void *userdata);
/* Returns a parameter description string as described above. */
Some comments on the interface:
Thanks, that was what I hoped for.
"Parameters as a comma separated list" sounds potentially problematic
if the specification extends to the public API as well, unless all
possible parameter types are defined to not contain commas. Why is the
list not encoded in the same way other lists are encoded in the message
API?
It surely can be encoded the same way, but currently the
parameters for the LADSPA sink can be specified on the
command line as a comma separated list, so I thought it
would be a good idea to keep that format and allow other
filters to do the same thing. I do not think it is a big restriction
to not allow commas in the parameters. I can think of
following parameter types: Int (signed and unsigned), float,
bool and string. Only the string type would have a problem
and we could use escaping there. Or do I miss something?
The comma separated list is easier to understand for users
of the interface.
Hmm, maybe you're right that it's a bit more user friendly format. It
feels somewhat ugly to send a list as a string when the transport has
native support for lists, but I think I can live with that.
I guess the best we can do is to use a comma separated list
on the command line and use the serialization when going
through the message API. It should not make that much
difference if you have to encode it as {{1}{2}{3}} instead
of {{1,2,3}}.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Fri, 2019-04-26 at 11:40 +0200, Georg Chini wrote:
> On 26.04.19 10:56, Tanu Kaskinen wrote:
> > On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
> > > The current scheme for
> > > updating
> > > parameters I have in mind should work for any of the existing filters
> > > and relies on
> > > passing around parameter structures:
> > >
> > >
> > > /* The following functions can be provided to set and get
> > > parameters. The parameter
> > >* structure is defined by the filter and should contain all
> > > parameters that are
> > >* configurable by the user. The library code makes no assumption
> > > on the contents
> > >* of the structure but only passes references. The library
> > > implements a message
> > >* handler which supports the following messages that use the
> > > functions below:
> > >* get_parameter - Retrieve a single parameter.
> > >* set_parameter - Set a single parameter.
> > >* get_all_parameters - Retrieve all parameters as a comma
> > > separated list.
> > >* set_all_parameters - Set all parameters simultaneously.
> > >* get_parameter_description - Returns a list that describes all
> > > parameters.
> > >* Format of the list elements is:
> > >* {{Identifier}{Type}{default}{min}{max}}
> > >* The last message can be used to get information about the filter
> > > or to implement
> > >* a filter control GUI that is independent of the filter type. */
> > >
> > > /* Get the value of the parameter described by identifier. The
> > > value shall be returned
> > >* as a string in value. The identifier may be any string that the
> > > filter recognizes
> > >* like a name or index, depending of the implementation of this
> > > function. */
> > > int (*parameter_get)(const char *identifier, char **value, void
> > > *userdata);
> > >
> > > /* Sets the value of the parameter described by identifier. The
> > > value is expected as
> > >* a string. The identifier may be any string that the filter
> > > recognizes like a name
> > >* or index. Returns a parameter structure filled with all current
> > > parameter values,
> > >* reflecting the updated parameter, so that the structure can be
> > > passed to
> > >* update_filter_parameter(). update_filter_parameter() will
> > > replace the current
> > >* parameter set with the new structure. */
> > > void *(*parameter_set)(const char *identifier, const char *value,
> > > void *userdata);
> > >
> > > /* Returns a comma separated list of the values of all filter
> > > parameters. */
> > > char *(*parameter_get_all)(void *userdata);
> > >
> > > /* Expects a comma separated list of all filter parameter values
> > > and returns a parameter
> > >* structure that can be passed to update_filter_parameters(). See
> > > set_parameter(). */
> > > void *(*parameter_set_all(const char *all_params, void *userdata);
> > >
> > > /* Returns a parameter description string as described above. */
> > Some comments on the interface:
>
> Thanks, that was what I hoped for.
>
> > "Parameters as a comma separated list" sounds potentially problematic
> > if the specification extends to the public API as well, unless all
> > possible parameter types are defined to not contain commas. Why is the
> > list not encoded in the same way other lists are encoded in the message
> > API?
>
> It surely can be encoded the same way, but currently the
> parameters for the LADSPA sink can be specified on the
> command line as a comma separated list, so I thought it
> would be a good idea to keep that format and allow other
> filters to do the same thing. I do not think it is a big restriction
> to not allow commas in the parameters. I can think of
> following parameter types: Int (signed and unsigned), float,
> bool and string. Only the string type would have a problem
> and we could use escaping there. Or do I miss something?
> The comma separated list is easier to understand for users
> of the interface.
Hmm, maybe you're right that it's a bit more user friendly format. It
feels somewhat ugly to send a list as a string when the transport has
native support for lists, but I think I can live with that.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 26.04.19 11:40, Georg Chini wrote:
On 26.04.19 10:56, Tanu Kaskinen wrote:
On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
The current scheme for
updating
parameters I have in mind should work for any of the existing filters
and relies on
passing around parameter structures:
/* The following functions can be provided to set and get
parameters. The parameter
* structure is defined by the filter and should contain all
parameters that are
* configurable by the user. The library code makes no assumption
on the contents
* of the structure but only passes references. The library
implements a message
* handler which supports the following messages that use the
functions below:
* get_parameter - Retrieve a single parameter.
* set_parameter - Set a single parameter.
* get_all_parameters - Retrieve all parameters as a comma
separated list.
* set_all_parameters - Set all parameters simultaneously.
* get_parameter_description - Returns a list that describes all
parameters.
* Format of the list elements is:
* {{Identifier}{Type}{default}{min}{max}}
* The last message can be used to get information about the
filter
or to implement
* a filter control GUI that is independent of the filter
type. */
/* Get the value of the parameter described by identifier. The
value shall be returned
* as a string in value. The identifier may be any string that
the
filter recognizes
* like a name or index, depending of the implementation of this
function. */
int (*parameter_get)(const char *identifier, char **value, void
*userdata);
/* Sets the value of the parameter described by identifier. The
value is expected as
* a string. The identifier may be any string that the filter
recognizes like a name
* or index. Returns a parameter structure filled with all
current
parameter values,
* reflecting the updated parameter, so that the structure can be
passed to
* update_filter_parameter(). update_filter_parameter() will
replace the current
* parameter set with the new structure. */
void *(*parameter_set)(const char *identifier, const char *value,
void *userdata);
/* Returns a comma separated list of the values of all filter
parameters. */
char *(*parameter_get_all)(void *userdata);
/* Expects a comma separated list of all filter parameter values
and returns a parameter
* structure that can be passed to update_filter_parameters().
See
set_parameter(). */
void *(*parameter_set_all(const char *all_params, void
*userdata);
/* Returns a parameter description string as described above. */
Some comments on the interface:
Thanks, that was what I hoped for.
"Parameters as a comma separated list" sounds potentially problematic
if the specification extends to the public API as well, unless all
possible parameter types are defined to not contain commas. Why is the
list not encoded in the same way other lists are encoded in the message
API?
It surely can be encoded the same way, but currently the
parameters for the LADSPA sink can be specified on the
command line as a comma separated list, so I thought it
would be a good idea to keep that format and allow other
filters to do the same thing. I do not think it is a big restriction
to not allow commas in the parameters. I can think of
following parameter types: Int (signed and unsigned), float,
bool and string. Only the string type would have a problem
and we could use escaping there. Or do I miss something?
The comma separated list is easier to understand for users
of the interface.
While trying to implement it, I see that you are right and using the
serialization of the message API is the best way to do it.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 26.04.19 10:56, Tanu Kaskinen wrote:
On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
The current scheme for
updating
parameters I have in mind should work for any of the existing filters
and relies on
passing around parameter structures:
/* The following functions can be provided to set and get
parameters. The parameter
* structure is defined by the filter and should contain all
parameters that are
* configurable by the user. The library code makes no assumption
on the contents
* of the structure but only passes references. The library
implements a message
* handler which supports the following messages that use the
functions below:
* get_parameter - Retrieve a single parameter.
* set_parameter - Set a single parameter.
* get_all_parameters - Retrieve all parameters as a comma
separated list.
* set_all_parameters - Set all parameters simultaneously.
* get_parameter_description - Returns a list that describes all
parameters.
* Format of the list elements is:
* {{Identifier}{Type}{default}{min}{max}}
* The last message can be used to get information about the filter
or to implement
* a filter control GUI that is independent of the filter type. */
/* Get the value of the parameter described by identifier. The
value shall be returned
* as a string in value. The identifier may be any string that the
filter recognizes
* like a name or index, depending of the implementation of this
function. */
int (*parameter_get)(const char *identifier, char **value, void
*userdata);
/* Sets the value of the parameter described by identifier. The
value is expected as
* a string. The identifier may be any string that the filter
recognizes like a name
* or index. Returns a parameter structure filled with all current
parameter values,
* reflecting the updated parameter, so that the structure can be
passed to
* update_filter_parameter(). update_filter_parameter() will
replace the current
* parameter set with the new structure. */
void *(*parameter_set)(const char *identifier, const char *value,
void *userdata);
/* Returns a comma separated list of the values of all filter
parameters. */
char *(*parameter_get_all)(void *userdata);
/* Expects a comma separated list of all filter parameter values
and returns a parameter
* structure that can be passed to update_filter_parameters(). See
set_parameter(). */
void *(*parameter_set_all(const char *all_params, void *userdata);
/* Returns a parameter description string as described above. */
Some comments on the interface:
Thanks, that was what I hoped for.
"Parameters as a comma separated list" sounds potentially problematic
if the specification extends to the public API as well, unless all
possible parameter types are defined to not contain commas. Why is the
list not encoded in the same way other lists are encoded in the message
API?
It surely can be encoded the same way, but currently the
parameters for the LADSPA sink can be specified on the
command line as a comma separated list, so I thought it
would be a good idea to keep that format and allow other
filters to do the same thing. I do not think it is a big restriction
to not allow commas in the parameters. I can think of
following parameter types: Int (signed and unsigned), float,
bool and string. Only the string type would have a problem
and we could use escaping there. Or do I miss something?
The comma separated list is easier to understand for users
of the interface.
"{{Identifier}{Type}{default}{min}{max}}" is probably too strict for
the parameter description specification, if it extends to the public
API as well. Min/max only works for range values, but I can imagine
some filter having a "choose one from a list of non-numeric options". I
think the structure after {default} should be dependent on the
parameter type.
It was just the first idea. I am aware that what exactly is passed
in the description list should be discussed carefully. The basic
idea is that in the end there can be a GUI that checks which
message handlers are installed, gets the parameter description
from all loaded filters and displays multiple tabs with the controls
for each filter, regardless which type of filter is loaded. Once signals
are available as well, the GUI could subscribe to creation/removal
signals of message handlers to keep track of loaded filters. ( I am
not very good at writing GUI's, so I would leave that task to somebody
else.)
This would allocate the memory in the main thread but would require that
the I/O-thread frees the old parameter structure when it is replaced.
Alternatively
the function that replaces the old structure with the new one could return a
pointer to the old structure, so that it can be freed in the main thread.
That should work. The structures probably have
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Tue, 2019-04-23 at 21:20 +0200, Georg Chini wrote:
> On 22.04.19 09:34, Tanu Kaskinen wrote:
> > On Sat, 2019-04-20 at 11:38 +0200, Georg Chini wrote:
> > > PA uses malloc() in the IO-thread, so are we doing things wrong?
> > > I think using malloc() when a parameter changes is not interfering
> > > with real-time operation because the filter must be reset after
> > > a parameter change anyway.
> Am I allowed to use free() from the I/O-thread?
No. To my knowledge a lot of the memory management work may actually
happen in free() rather than malloc().
> The current scheme for
> updating
> parameters I have in mind should work for any of the existing filters
> and relies on
> passing around parameter structures:
>
>
> /* The following functions can be provided to set and get
> parameters. The parameter
> * structure is defined by the filter and should contain all
> parameters that are
> * configurable by the user. The library code makes no assumption
> on the contents
> * of the structure but only passes references. The library
> implements a message
> * handler which supports the following messages that use the
> functions below:
> * get_parameter - Retrieve a single parameter.
> * set_parameter - Set a single parameter.
> * get_all_parameters - Retrieve all parameters as a comma
> separated list.
> * set_all_parameters - Set all parameters simultaneously.
> * get_parameter_description - Returns a list that describes all
> parameters.
> * Format of the list elements is:
> * {{Identifier}{Type}{default}{min}{max}}
> * The last message can be used to get information about the filter
> or to implement
> * a filter control GUI that is independent of the filter type. */
>
> /* Get the value of the parameter described by identifier. The
> value shall be returned
> * as a string in value. The identifier may be any string that the
> filter recognizes
> * like a name or index, depending of the implementation of this
> function. */
> int (*parameter_get)(const char *identifier, char **value, void
> *userdata);
>
> /* Sets the value of the parameter described by identifier. The
> value is expected as
> * a string. The identifier may be any string that the filter
> recognizes like a name
> * or index. Returns a parameter structure filled with all current
> parameter values,
> * reflecting the updated parameter, so that the structure can be
> passed to
> * update_filter_parameter(). update_filter_parameter() will
> replace the current
> * parameter set with the new structure. */
> void *(*parameter_set)(const char *identifier, const char *value,
> void *userdata);
>
> /* Returns a comma separated list of the values of all filter
> parameters. */
> char *(*parameter_get_all)(void *userdata);
>
> /* Expects a comma separated list of all filter parameter values
> and returns a parameter
> * structure that can be passed to update_filter_parameters(). See
> set_parameter(). */
> void *(*parameter_set_all(const char *all_params, void *userdata);
>
> /* Returns a parameter description string as described above. */
Some comments on the interface:
"Parameters as a comma separated list" sounds potentially problematic
if the specification extends to the public API as well, unless all
possible parameter types are defined to not contain commas. Why is the
list not encoded in the same way other lists are encoded in the message
API?
"{{Identifier}{Type}{default}{min}{max}}" is probably too strict for
the parameter description specification, if it extends to the public
API as well. Min/max only works for range values, but I can imagine
some filter having a "choose one from a list of non-numeric options". I
think the structure after {default} should be dependent on the
parameter type.
> This would allocate the memory in the main thread but would require that
> the I/O-thread frees the old parameter structure when it is replaced.
> Alternatively
> the function that replaces the old structure with the new one could return a
> pointer to the old structure, so that it can be freed in the main thread.
That should work. The structures probably have fixed size, though, in
which case malloc()/free() can be avoided altogether: the filters can
allocate two instances of the internal structures during intialization
and switch between them when parameters are updated.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Mon, 2019-04-22 at 11:23 +0200, Georg Chini wrote: > On 22.04.19 09:34, Tanu Kaskinen wrote: > > On Sat, 2019-04-20 at 11:38 +0200, Georg Chini wrote: > > > PA uses malloc() in the IO-thread, so are we doing things wrong? > > > I think using malloc() when a parameter changes is not interfering > > > with real-time operation because the filter must be reset after > > > a parameter change anyway. > > Where exactly is malloc() used? Sounds like a bug. > > Well, for example pa_memblock_new() potentially uses malloc and > this is surely used from the I/O thread. I did not check, but I guess > there are also places where something like pa_xstrdup() or pa_xnew() > is used from the I/O thread. At least I was not aware that there > are any restrictions. > pa_rtpoll_item_new() also potentially uses malloc(). > > All in all there are many places where malloc() might be used. pa_memblock_new() and pa_rtpoll_item_new() both use malloc() only if they run out of preallocated blocks/items (and the preallocation is done precisely to avoid calling malloc()). > > > > > Well, after evaluation of the feedback I have been getting so far, > > > I do not think I will make an attempt to create some plugin-sink. > > > The existing standards do not fit to what I have in mind and > > > if I read Alexander right they even intentionally do not support > > > those features. > > > > > > Inventing a PA internal standard does not make sense, because > > > your main argument against implementing the equalizer as a module > > > was that you did not want to host the DSP code inside PA. If we > > > did our own standard, new filters would again be bound to PA. > > New filters would be specific to PA, yes, but not maintained by us, and > > new filters wouldn't have to go through the review process, so there > > would still be some benefit in having a PA specific stable plugin API. > > > I don't understand. What is the difference between having DSP > code in a module or in a filter plugin? The difference is that the module API is not stable, so people can't maintain their filters outside the upstream PulseAudio repository. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 22.04.19 09:34, Tanu Kaskinen wrote: On Sat, 2019-04-20 at 11:38 +0200, Georg Chini wrote: PA uses malloc() in the IO-thread, so are we doing things wrong? I think using malloc() when a parameter changes is not interfering with real-time operation because the filter must be reset after a parameter change anyway. Where exactly is malloc() used? Sounds like a bug. Well, for example pa_memblock_new() potentially uses malloc and this is surely used from the I/O thread. I did not check, but I guess there are also places where something like pa_xstrdup() or pa_xnew() is used from the I/O thread. At least I was not aware that there are any restrictions. pa_rtpoll_item_new() also potentially uses malloc(). All in all there are many places where malloc() might be used. Well, after evaluation of the feedback I have been getting so far, I do not think I will make an attempt to create some plugin-sink. The existing standards do not fit to what I have in mind and if I read Alexander right they even intentionally do not support those features. Inventing a PA internal standard does not make sense, because your main argument against implementing the equalizer as a module was that you did not want to host the DSP code inside PA. If we did our own standard, new filters would again be bound to PA. New filters would be specific to PA, yes, but not maintained by us, and new filters wouldn't have to go through the review process, so there would still be some benefit in having a PA specific stable plugin API. I don't understand. What is the difference between having DSP code in a module or in a filter plugin? After consolidation of the virtual sinks, there is nearly nothing left in the modules that would need our attention. It is mostly pure DSP code. I think now writing a module is the quickest way to get a filter working and using another plugin format would only complicate things. So from my point of view, there already is a PA specific plugin format and I am not going to invent another one. When the messaging API patches are reviewed, I will write an addition to the virtual sink library which makes setting parameters easy. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Sat, 2019-04-20 at 11:38 +0200, Georg Chini wrote: > On 20.04.19 11:06, Tanu Kaskinen wrote: > > On Fri, 2019-04-19 at 17:52 +0200, Georg Chini wrote: > > > On 19.04.19 16:56, Tanu Kaskinen wrote: > > > > Changing the number of eq bands isn't quite like changing regular > > > > control values. The plugin probably has some per-band data, which has > > > > to be reallocated when the number of bands changes. malloc() isn't > > > > allowed in the IO thread. Also, all gain values assigned to the bands > > > > previously are likely useless, because the band frequencies change. The > > > > host will likely have to set all bands to the default gain value, so in > > > > effect changing the band count is like starting from scratch, which is > > > > very different from changing a regular control value. > > > Yes, you are right, it would be like starting from scratch. It > > > would however be not the primary goal to change the number > > > of bands at run-time, but to be able to define the number of > > > bands dynamically when the instance is created. Because this > > > would affect the number of necessary ports (per band gains) > > > it is not possible with the current definition unless you have > > > a control port that can be an array. As a side effect, the band > > > count could also be changed dynamically at run-time. > > > > > > Why would malloc() not be allowed in the IO-thread? It's not > > > allowed within the run() function, but that's a different thing. > > Why is it a different thing? malloc() is not allowed in the run() > > function, because the function is expected to be run in a realtime > > thread, and malloc() is not realtime-safe. The IO thread is a realtime > > thread, so the same limitations apply also outside the run() function. > > PA uses malloc() in the IO-thread, so are we doing things wrong? > I think using malloc() when a parameter changes is not interfering > with real-time operation because the filter must be reset after > a parameter change anyway. Where exactly is malloc() used? Sounds like a bug. Using malloc() when changing parameters does interfere with realtime operation, because there may be more audio paths to the hardware sink than just the ladspa filter. There may be streams that aren't filtered. > > > > If the eq band count was an initialization parameter rather than a > > > > control port, the IO thread limitations wouldn't become an issue, and > > > > it would be explicit that changing the eq band count means starting > > > > from scratch. It should still be possible to change initialization > > > > parameters at runtime, that would just mean that a new plugin instance > > > > is created and the old instance is removed. > > > > > > > It is not possible to have that kind of initialization parameter. > > > That is the main problem. As explained above, changing the > > > number of bands would require changing the number of control > > > ports. > > If we're adding new stuff to the LADSPA interface anyway, we can surely > > add a function that sets initialization parameters (and a function for > > querying what initialization parameters the plugin has). We can then > > specify that the control ports are to be created only after the > > initialization parameters have been set. > > > Well, after evaluation of the feedback I have been getting so far, > I do not think I will make an attempt to create some plugin-sink. > The existing standards do not fit to what I have in mind and > if I read Alexander right they even intentionally do not support > those features. > > Inventing a PA internal standard does not make sense, because > your main argument against implementing the equalizer as a module > was that you did not want to host the DSP code inside PA. If we > did our own standard, new filters would again be bound to PA. New filters would be specific to PA, yes, but not maintained by us, and new filters wouldn't have to go through the review process, so there would still be some benefit in having a PA specific stable plugin API. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
сб, 20 апр. 2019 г. в 15:20, Georg Chini : > > On 20.04.19 12:14, Alexander E. Patrakov wrote: > > [forgot to CC the list - sorry for any duplicates] > > > > сб, 20 апр. 2019 г. в 14:39, Georg Chini : > > > >> I think using malloc() when a parameter changes is not interfering > >> with real-time operation because the filter must be reset after > >> a parameter change anyway. > > The parameter changes allowed during the runtime are not expected to > > need a filter reset. In-place rebuild - yes, reset - no. The filtered > > audio should smoothly follow gradual changes of the filter parameters. > > Many LADSPA plugins actually implement internal smoothing of parameter > > value changes, or even internally keep two filter versions and > > interpolate between them, in order to avoid clicks, because such > > gradual changes are an expected thing in pro audio workflows. > > > Within PA, the filter must at least be rewound when a parameter changes. > Because rewinding is not part of the standard, the best we can do is to > reset the filter in that case by calling deactivate() and activate(). This is a bug in PA. Non-rewindable sinks are supported, actually exist (e.g. alsa-sink when the ALSA device is an ioplug, e.g. an AC3 encoder) and make sense if the latency is artificially limited to, say, 20-50 ms. So a LADSPA sink must just say "I can rewind zero samples", limit the latency, and ignore all rewind requests. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 20.04.19 12:14, Alexander E. Patrakov wrote: [forgot to CC the list - sorry for any duplicates] сб, 20 апр. 2019 г. в 14:39, Georg Chini : I think using malloc() when a parameter changes is not interfering with real-time operation because the filter must be reset after a parameter change anyway. The parameter changes allowed during the runtime are not expected to need a filter reset. In-place rebuild - yes, reset - no. The filtered audio should smoothly follow gradual changes of the filter parameters. Many LADSPA plugins actually implement internal smoothing of parameter value changes, or even internally keep two filter versions and interpolate between them, in order to avoid clicks, because such gradual changes are an expected thing in pro audio workflows. Out of curiosity: How does that work? I do not see any function in the LADSPA standard which notifies the plugin of parameter changes. So how does the plugin know that a parameter has changed? Does it have to copy the parameter values to its own internal structures and compare them each time during run()? Or does it rely on a new connect() call? ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 20.04.19 12:14, Alexander E. Patrakov wrote: [forgot to CC the list - sorry for any duplicates] сб, 20 апр. 2019 г. в 14:39, Georg Chini : I think using malloc() when a parameter changes is not interfering with real-time operation because the filter must be reset after a parameter change anyway. The parameter changes allowed during the runtime are not expected to need a filter reset. In-place rebuild - yes, reset - no. The filtered audio should smoothly follow gradual changes of the filter parameters. Many LADSPA plugins actually implement internal smoothing of parameter value changes, or even internally keep two filter versions and interpolate between them, in order to avoid clicks, because such gradual changes are an expected thing in pro audio workflows. Within PA, the filter must at least be rewound when a parameter changes. Because rewinding is not part of the standard, the best we can do is to reset the filter in that case by calling deactivate() and activate(). ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
[forgot to CC the list - sorry for any duplicates] сб, 20 апр. 2019 г. в 14:39, Georg Chini : > I think using malloc() when a parameter changes is not interfering > with real-time operation because the filter must be reset after > a parameter change anyway. The parameter changes allowed during the runtime are not expected to need a filter reset. In-place rebuild - yes, reset - no. The filtered audio should smoothly follow gradual changes of the filter parameters. Many LADSPA plugins actually implement internal smoothing of parameter value changes, or even internally keep two filter versions and interpolate between them, in order to avoid clicks, because such gradual changes are an expected thing in pro audio workflows. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 20.04.19 11:06, Tanu Kaskinen wrote: On Fri, 2019-04-19 at 17:52 +0200, Georg Chini wrote: On 19.04.19 16:56, Tanu Kaskinen wrote: On Fri, 2019-04-19 at 12:03 +0200, Georg Chini wrote: On 19.04.19 11:13, Tanu Kaskinen wrote: On Tue, 2019-04-16 at 21:40 +0200, Georg Chini wrote: On 16.04.19 19:19, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: On 11.04.19 19:36, Tanu Kaskinen wrote: If you want a better plugin standard, are you aware of LV2 and PipeWire's SPA (the latter doesn't seem to be properly documented yet, but to my understanding it's supposed to have a stable and flexible API)? Arun already suggested the pipewire SPA. I took a look, but it seems not very simple compared to LADSPA. I could not really understand how it works and it appears to support a lot more than just filters. LV2 would also be an option, although it too is pretty complex compared to LADSPA. But at least it's documented and has examples. I just took a look and on the first glance LV2 seems similar to LADSPA. I have to dig into the details though, maybe control arrays and interleaved audio ports are possible there. I'm pretty sure they are possible, but neither of those features are necessary. If the plugin gets the number of bands during the initialization, it can create the appropriate number of non-array control ports. Interleaved audio ports aren't needed either, because PulseAudio can do the deinterleaving before passing the audio to the plugin (like module-ladspa-sink already does). If one's going to write an LV2 plugin, it's best to use standard port types so that all hosts will be able to use the plugin. The problem here is that the number of ports must be known before the initialization because it is something which is already provided by the plugin descriptor. So there seems to be no way to change that number dynamically unless I misread the documentation. But looking at the LV2 standard, it supplies the port type lv2:CVPort (see https://gitlab.com/lv2/lv2/blob/master/lv2/core/lv2core.ttl, line 256) which is what I have been looking for if I read the documentation right. I don't think CVPorts are relevant for this discussion. As far as I can tell, they just provide a different kind of control port, one that can use audio signal as input. You wanted a port that can take an array of values, CVPorts isn't that. The documentation basically says it is an array of floats which is not audio but control data. So it is very relevant here. This is exactly what is needed if you want to support a variable number of bands because you need a variable number of gains. It's not just any random array, it's an array that the host can feed control values at the same rate as it feeds audio to the audio ports. So if the host sees an input CVPort, it will expect that the port can be connected to an output CVPort or to an audio output port. You seem to be right about the requirement to declare all ports in advance. I thought dynamic ports would be the primary benefit of using LV2 rather than creating a custom API based on LADSPA. Concerning interleaved audio format: Up to now I found nothing that explicitly forbids interleaved audio though it appears that the plugins usually provide one audio port per channel. You can't use a plain AudioPort for interleaved audio, because hosts will assume that it operates with mono audio. You can probably define a subclass of AudioPort with different semantics, but then hosts other than PulseAudio won't be able to use the plugin (unless they adopt your extension). Other hosts could still use the plugin because mono would be perfectly acceptable. But I agree that we should not implement something that is not in the specification. What should be possible however is writing an LV2 extension that allows interleaved ports. If hosts do not support this extension, the plugins would be considered mono but could still work. Yes, it's one option to write a plugin that provides both mono and interleaved ports. The extension should then also specify a way to indicate which mono audio ports are not to be used if the host uses the interleaved port. PA can surely deinterleave the input and interleave the output but to me it looks like completely unnecessary copying around of buffers within a real time thread. With interleaved channels, you could directly pass the input and output buffers. Why should we copy the data twice if it can be avoided? Additionally, using one port per channel makes it impossible to adapt the number of channels dynamically when loading the plugin for the reason given above. The reason I suggested deinterleaving in PulseAudio was to allow the plugin to be compatible with other hosts. Without native support for dynamic ports in LV2, such compatibility seems to be hard/impossible to achieve, however. My intention would have been to support both, plugins that use one audio port per channel and plugins that use interleaved channels. Most of the
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
сб, 20 апр. 2019 г. в 05:54, Georg Chini : > But what you are basically saying is that the equalizer does > not hold what Andrea claims it can do and that it is only useful > as a 10-band equalizer. I wonder why her work was then > accepted as a master thesis. I don't know the Italian requirements for a master thesis. In Russia, rejections of this kind of thesis are rare: it's a time-boxed activity without a second chance to choose a topic (unlike the real scientific work), and rejections would make it a not-worth-it gamble in the students' eyes. And the same kind of number-of-pages stuffing (by including chapters on "considered but rejected" techniques) is common, I did it too. We do need a recheck from Italian experts not related to the government issued degrees. Probably on JACK mailing lists? And hopefully you now understand why I am inclined to reject even objectively-good scientific code submissions, if they are making something a part of PulseAudio: insufficient number of local experts that would have rejected bad submissions. OTOH, a plugin architecture that allows us to tap into the work by an existing pool of experts, without taking any responsibility, would be awesome. > >>> Besides, consumer electronic devices (TVs, HDMI receivers, Hi-Fi > >>> amplifiers) just do not have equalizers with a variable number of > >>> bands, for usability reasons. I don't see a reason for PulseAudio to > >>> be different. > >>> > >> You can't add sliders on the fly on a consumer electronic > >> device but you can do so in software. Why should we be guided > >> by a behavior that is governed by physical limits? If the algorithm > >> supports it, I see no reason why it should not be implemented. > >> (As said above naturally within some sane limits, you are right > >> that it would make no sense to have for example 50 bands.) > > Sliders in a consumer electronics device such as a TV are just virtual > > widgets on the screen, i.e. software, not physical knobs. Again, their > > number is fixed because of usable and "intuitive" UI. > I guess there are enough people who would wish for more > flexibility. > > > >> Surely you could go for several different plugins, but I do not > >> see the reason why the code should be duplicated a couple > >> of times. The goal is to have one plugin and not a collection. > >> I just don't like the idea that you have to duplicate things only > >> because of the limitation of the surrounding framework. > >> That's why I am looking for a way to dynamically adapt the > >> number of control ports or - which seems to be supported > >> by LV2 - use a control port that is an array. > > And my point is exactly that there is a reason, filter order, not > > related to any framework limitations, for different code for different > > number of bands. > > > >> Another example where an array would be useful as a control > >> port is the virtual-surround-sink. The HRIR data used by the filter > >> is an array of floats which could vary in size. > > I also disagree here. Yes, it can vary in size, but should not be > > treated as a control port at all. There is no way to usefully control > > it for a user, that's why. It should therefore be either hard-coded > > (as done in Windows) or loaded through a file. > > If you load it through a file like pulseaudio does, how do > you pass the values to the filter? Or should the plugin itself > load the file? The plugin itself should load the file. The fact that the file does not come with the plugin itself is a major bug that unfortunately exists for licensing reasons. I think we should find a free-enough data source for HRIR/HRTF and just hard-code the impulse responses, though. We can investigate reusing the data from CSound. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Fri, 2019-04-19 at 17:52 +0200, Georg Chini wrote: > On 19.04.19 16:56, Tanu Kaskinen wrote: > > On Fri, 2019-04-19 at 12:03 +0200, Georg Chini wrote: > > > On 19.04.19 11:13, Tanu Kaskinen wrote: > > > > On Tue, 2019-04-16 at 21:40 +0200, Georg Chini wrote: > > > > > On 16.04.19 19:19, Tanu Kaskinen wrote: > > > > > > On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: > > > > > > > On 11.04.19 19:36, Tanu Kaskinen wrote: > > > > > > > > If you want a better plugin standard, are you aware of LV2 > > > > > > > > and PipeWire's SPA (the latter doesn't seem to be properly > > > > > > > > documented > > > > > > > > yet, but to my understanding it's supposed to have a stable and > > > > > > > > flexible API)? > > > > > > > Arun already suggested the pipewire SPA. I took a look, but it > > > > > > > seems not very simple compared to LADSPA. I could not really > > > > > > > understand how it works and it appears to support a lot more > > > > > > > than just filters. > > > > > > LV2 would also be an option, although it too is pretty complex > > > > > > compared > > > > > > to LADSPA. But at least it's documented and has examples. > > > > > I just took a look and on the first glance LV2 seems similar > > > > > to LADSPA. I have to dig into the details though, maybe control > > > > > arrays and interleaved audio ports are possible there. > > > > I'm pretty sure they are possible, but neither of those features are > > > > necessary. If the plugin gets the number of bands during the > > > > initialization, it can create the appropriate number of non-array > > > > control ports. Interleaved audio ports aren't needed either, because > > > > PulseAudio can do the deinterleaving before passing the audio to the > > > > plugin (like module-ladspa-sink already does). If one's going to write > > > > an LV2 plugin, it's best to use standard port types so that all hosts > > > > will be able to use the plugin. > > > The problem here is that the number of ports must be known before > > > the initialization because it is something which is already provided by > > > the plugin descriptor. So there seems to be no way to change that > > > number dynamically unless I misread the documentation. But looking > > > at the LV2 standard, it supplies the port type lv2:CVPort (see > > > https://gitlab.com/lv2/lv2/blob/master/lv2/core/lv2core.ttl, line 256) > > > which is what I have been looking for if I read the documentation > > > right. > > I don't think CVPorts are relevant for this discussion. As far as I can > > tell, they just provide a different kind of control port, one that can > > use audio signal as input. You wanted a port that can take an array of > > values, CVPorts isn't that. > > The documentation basically says it is an array of floats which is > not audio but control data. So it is very relevant here. This is > exactly what is needed if you want to support a variable number > of bands because you need a variable number of gains. It's not just any random array, it's an array that the host can feed control values at the same rate as it feeds audio to the audio ports. So if the host sees an input CVPort, it will expect that the port can be connected to an output CVPort or to an audio output port. > > You seem to be right about the requirement to declare all ports in > > advance. I thought dynamic ports would be the primary benefit of using > > LV2 rather than creating a custom API based on LADSPA. > > > > > Concerning interleaved audio format: Up to now I found nothing > > > that explicitly forbids interleaved audio though it appears that the > > > plugins usually provide one audio port per channel. > > You can't use a plain AudioPort for interleaved audio, because hosts > > will assume that it operates with mono audio. You can probably define a > > subclass of AudioPort with different semantics, but then hosts other > > than PulseAudio won't be able to use the plugin (unless they adopt your > > extension). > > Other hosts could still use the plugin because mono would > be perfectly acceptable. But I agree that we should not > implement something that is not in the specification. What > should be possible however is writing an LV2 extension that > allows interleaved ports. If hosts do not support this extension, > the plugins would be considered mono but could still work. Yes, it's one option to write a plugin that provides both mono and interleaved ports. The extension should then also specify a way to indicate which mono audio ports are not to be used if the host uses the interleaved port. > > > PA can surely deinterleave the input and interleave the output > > > but to me it looks like completely unnecessary copying around > > > of buffers within a real time thread. With interleaved channels, > > > you could directly pass the input and output buffers. Why should > > > we copy the data twice if it can be avoided? Additionally, using > > > one port per channel makes it impossible to adapt the number > > > of
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 20.04.19 01:02, Alexander E. Patrakov wrote: сб, 20 апр. 2019 г. в 00:15, Georg Chini : On 19.04.19 18:23, Alexander E. Patrakov wrote: пт, 19 апр. 2019 г. в 14:13, Tanu Kaskinen : If the plugin gets the number of bands during the initialization, it can create the appropriate number of non-array control ports. Interleaved audio ports aren't needed either, because PulseAudio can do the deinterleaving before passing the audio to the plugin (like module-ladspa-sink already does). If one's going to write an LV2 plugin, it's best to use standard port types so that all hosts will be able to use the plugin. In addition, I think that "if the plugin gets the number of bands [at runtime]" is a very big "if" for an IIR-based equalizer. So big that I would rather hard-code it, or treat equalizers with a different number of bands as completely different plugins. The reason is that the required slope of inter-band transition (i.e., effectively, the filter order) is a function of the width of each band. Implement a filter with a too-low order, and it won't be able to isolate (and thus control thegain of) a frequency band selectively enough. Implement a filter with a too-high order, and its frequency response will be too steppy. My understanding is that Andrea's equalizer algorithm supports an arbitrary number of bands (certainly within some sane limits). Please correct me if I am wrong. It supports arbitrary amount of bands, but not arbitrary filter order for a band. Just to reiterate, the whole filter is a cascade of some per-band filters of the same order. Each filter in the cascade is implemented, according to the dissertation, as a cut-and-boost filter, which is based on a band-pass filter, which is in turn based on a shelving Butterworth filter of order 2M. Hopefully I haven't missed any order-doubling here. I have not thoroughly checked whether the implementation actually follows the dissertation - this is complicated by the Italian language that I don't know, and cryptic variable names. The majority of the dissertation does contain formulas for an arbitrary even order (2M), but on page 41, it starts talking about filters of order 8 only. And, if I understand correctly, it doesn't match the code, which is hard-coded to implement only filters of order 4, and it is not only a matter of increasing M and M2 in the file, because then xn[i] must be set in eq_filter() for i >= 4. Another sign of this mismatch between the dissertation and the code is the different length of the "c" array - 10 in the code and 21 in algorithm 6.2 on page 43 (page numbers here are presented as printed at the bottom, not as displayed in PDF viewers' left pane). Because of the fixed order, there are indeed limits on the useful number of bands. E.g., given that the minimum and maximum supported gains in the band are -12 and 12 dB (at least this is what's mentioned in the dissertation), it means that the gain must increase by 24 dB between the centers of the neighboring bands. With the default 10-band filter, i.e. with one octave per band, this means that 24 dB per octave is the maximum supported slope, which is roughly right for a 4th order filter. In other words, more than 10 bands cannot be useful with 4th order filters and 24 dB of gain difference between the neighboring bands. I understand that it is odd to set the gain in two neighboring octaves to differ by that much. For a 5-band equalizer with the same gain limits, filters of 2nd order would be sufficient and preferable. Also, the order of the filter could be lowered if the gain limits are set lower. I can't really comment on this, my knowledge is not sufficient. But what you are basically saying is that the equalizer does not hold what Andrea claims it can do and that it is only useful as a 10-band equalizer. I wonder why her work was then accepted as a master thesis. Besides, consumer electronic devices (TVs, HDMI receivers, Hi-Fi amplifiers) just do not have equalizers with a variable number of bands, for usability reasons. I don't see a reason for PulseAudio to be different. You can't add sliders on the fly on a consumer electronic device but you can do so in software. Why should we be guided by a behavior that is governed by physical limits? If the algorithm supports it, I see no reason why it should not be implemented. (As said above naturally within some sane limits, you are right that it would make no sense to have for example 50 bands.) Sliders in a consumer electronics device such as a TV are just virtual widgets on the screen, i.e. software, not physical knobs. Again, their number is fixed because of usable and "intuitive" UI. I guess there are enough people who would wish for more flexibility. Surely you could go for several different plugins, but I do not see the reason why the code should be duplicated a couple of times. The goal is to have one plugin and not a collection. I just don't like the idea that you have to duplicate things only because
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
сб, 20 апр. 2019 г. в 00:15, Georg Chini : > > On 19.04.19 18:23, Alexander E. Patrakov wrote: > > пт, 19 апр. 2019 г. в 14:13, Tanu Kaskinen : > >> If the plugin gets the number of bands during the > >> initialization, it can create the appropriate number of non-array > >> control ports. Interleaved audio ports aren't needed either, because > >> PulseAudio can do the deinterleaving before passing the audio to the > >> plugin (like module-ladspa-sink already does). If one's going to write > >> an LV2 plugin, it's best to use standard port types so that all hosts > >> will be able to use the plugin. > > In addition, I think that "if the plugin gets the number of bands [at > > runtime]" is a very big "if" for an IIR-based equalizer. So big that I > > would rather hard-code it, or treat equalizers with a different number > > of bands as completely different plugins. The reason is that the > > required slope of inter-band transition (i.e., effectively, the filter > > order) is a function of the width of each band. Implement a filter > > with a too-low order, and it won't be able to isolate (and thus > > control thegain of) a frequency band selectively enough. Implement a > > filter with a too-high order, and its frequency response will be too > > steppy. > > My understanding is that Andrea's equalizer algorithm supports > an arbitrary number of bands (certainly within some sane limits). > Please correct me if I am wrong. It supports arbitrary amount of bands, but not arbitrary filter order for a band. Just to reiterate, the whole filter is a cascade of some per-band filters of the same order. Each filter in the cascade is implemented, according to the dissertation, as a cut-and-boost filter, which is based on a band-pass filter, which is in turn based on a shelving Butterworth filter of order 2M. Hopefully I haven't missed any order-doubling here. I have not thoroughly checked whether the implementation actually follows the dissertation - this is complicated by the Italian language that I don't know, and cryptic variable names. The majority of the dissertation does contain formulas for an arbitrary even order (2M), but on page 41, it starts talking about filters of order 8 only. And, if I understand correctly, it doesn't match the code, which is hard-coded to implement only filters of order 4, and it is not only a matter of increasing M and M2 in the file, because then xn[i] must be set in eq_filter() for i >= 4. Another sign of this mismatch between the dissertation and the code is the different length of the "c" array - 10 in the code and 21 in algorithm 6.2 on page 43 (page numbers here are presented as printed at the bottom, not as displayed in PDF viewers' left pane). Because of the fixed order, there are indeed limits on the useful number of bands. E.g., given that the minimum and maximum supported gains in the band are -12 and 12 dB (at least this is what's mentioned in the dissertation), it means that the gain must increase by 24 dB between the centers of the neighboring bands. With the default 10-band filter, i.e. with one octave per band, this means that 24 dB per octave is the maximum supported slope, which is roughly right for a 4th order filter. In other words, more than 10 bands cannot be useful with 4th order filters and 24 dB of gain difference between the neighboring bands. I understand that it is odd to set the gain in two neighboring octaves to differ by that much. For a 5-band equalizer with the same gain limits, filters of 2nd order would be sufficient and preferable. Also, the order of the filter could be lowered if the gain limits are set lower. > > Besides, consumer electronic devices (TVs, HDMI receivers, Hi-Fi > > amplifiers) just do not have equalizers with a variable number of > > bands, for usability reasons. I don't see a reason for PulseAudio to > > be different. > > > You can't add sliders on the fly on a consumer electronic > device but you can do so in software. Why should we be guided > by a behavior that is governed by physical limits? If the algorithm > supports it, I see no reason why it should not be implemented. > (As said above naturally within some sane limits, you are right > that it would make no sense to have for example 50 bands.) Sliders in a consumer electronics device such as a TV are just virtual widgets on the screen, i.e. software, not physical knobs. Again, their number is fixed because of usable and "intuitive" UI. > Surely you could go for several different plugins, but I do not > see the reason why the code should be duplicated a couple > of times. The goal is to have one plugin and not a collection. > I just don't like the idea that you have to duplicate things only > because of the limitation of the surrounding framework. > That's why I am looking for a way to dynamically adapt the > number of control ports or - which seems to be supported > by LV2 - use a control port that is an array. And my point is exactly that there is a reason, filter
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
пн, 8 апр. 2019 г. в 15:05, Alexander E. Patrakov : > > пн, 8 апр. 2019 г. в 13:08, Alexander E. Patrakov : > > > I have looked again at the paper, and have a DSP question. > > > > The thesis starts with designing a shelf filter. That is, a filter > > that contains a flat area at low frequencies, a flat area at high > > frequencies (but with a different gain), and some transition in > > between. Then, for some unknown reason, it is transformed into a > > band-pass shelving filter. Then there is some talk how to "stitch" > > together several band-pass shelving filters. > > > > Why would one need to do this at all? I.e., why can't one just cascade > > several shelf filters, designed according to the correct transition > > frequencies, with the height of the shelf being the dB difference of > > the desired gains of the neighboring bands? > > Just for some context: the reason why I asked is the ripple on Figure > 2.5 that seems to be avoidable in my approach. Sorry for the stupid question. This is answered in Chapter 5. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 19.04.19 18:23, Alexander E. Patrakov wrote: пт, 19 апр. 2019 г. в 14:13, Tanu Kaskinen : If the plugin gets the number of bands during the initialization, it can create the appropriate number of non-array control ports. Interleaved audio ports aren't needed either, because PulseAudio can do the deinterleaving before passing the audio to the plugin (like module-ladspa-sink already does). If one's going to write an LV2 plugin, it's best to use standard port types so that all hosts will be able to use the plugin. In addition, I think that "if the plugin gets the number of bands [at runtime]" is a very big "if" for an IIR-based equalizer. So big that I would rather hard-code it, or treat equalizers with a different number of bands as completely different plugins. The reason is that the required slope of inter-band transition (i.e., effectively, the filter order) is a function of the width of each band. Implement a filter with a too-low order, and it won't be able to isolate (and thus control thegain of) a frequency band selectively enough. Implement a filter with a too-high order, and its frequency response will be too steppy. My understanding is that Andrea's equalizer algorithm supports an arbitrary number of bands (certainly within some sane limits). Please correct me if I am wrong. Besides, consumer electronic devices (TVs, HDMI receivers, Hi-Fi amplifiers) just do not have equalizers with a variable number of bands, for usability reasons. I don't see a reason for PulseAudio to be different. You can't add sliders on the fly on a consumer electronic device but you can do so in software. Why should we be guided by a behavior that is governed by physical limits? If the algorithm supports it, I see no reason why it should not be implemented. (As said above naturally within some sane limits, you are right that it would make no sense to have for example 50 bands.) Surely you could go for several different plugins, but I do not see the reason why the code should be duplicated a couple of times. The goal is to have one plugin and not a collection. I just don't like the idea that you have to duplicate things only because of the limitation of the surrounding framework. That's why I am looking for a way to dynamically adapt the number of control ports or - which seems to be supported by LV2 - use a control port that is an array. Another example where an array would be useful as a control port is the virtual-surround-sink. The HRIR data used by the filter is an array of floats which could vary in size. The focus of this mail thread is no longer the equalizer, even though the subject may suggest that and it is often used as an example. The starting point of the discussion was that I did a consolidation of the virtual sinks we have in pulseaudio (see !88) and the result shows that many of the filters we have could be further consolidated to a single plugin sink. The question then was which kind of plugin format we should use? Naturally it would be better to use some standard API instead of inventing our own. And if we move to a plugin-sink, the plugin API should also support the features of the new equalizer, so that this can be integrated as well if Andrea is willing to contribute to a plugin. Tanu suggested the LV2 standard, so I took a look into it. My understanding is, that the CVPort is the type of control structure I have been looking for, though Tanu says otherwise. There is however another limitation I see with the LV2 standard, which is that it does not support interleaved audio channels. This is not fatal, but at least inconvenient. It looks like massive overhead to me if you have to de-interlace the audio data, then run multiple instances of the filter and finally have to interleave the data again. I think a plugin standard for pulseaudio should be able to handle interleaved data. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
пт, 19 апр. 2019 г. в 14:13, Tanu Kaskinen : > If the plugin gets the number of bands during the > initialization, it can create the appropriate number of non-array > control ports. Interleaved audio ports aren't needed either, because > PulseAudio can do the deinterleaving before passing the audio to the > plugin (like module-ladspa-sink already does). If one's going to write > an LV2 plugin, it's best to use standard port types so that all hosts > will be able to use the plugin. In addition, I think that "if the plugin gets the number of bands [at runtime]" is a very big "if" for an IIR-based equalizer. So big that I would rather hard-code it, or treat equalizers with a different number of bands as completely different plugins. The reason is that the required slope of inter-band transition (i.e., effectively, the filter order) is a function of the width of each band. Implement a filter with a too-low order, and it won't be able to isolate (and thus control thegain of) a frequency band selectively enough. Implement a filter with a too-high order, and its frequency response will be too steppy. Besides, consumer electronic devices (TVs, HDMI receivers, Hi-Fi amplifiers) just do not have equalizers with a variable number of bands, for usability reasons. I don't see a reason for PulseAudio to be different. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 19.04.19 16:56, Tanu Kaskinen wrote: On Fri, 2019-04-19 at 12:03 +0200, Georg Chini wrote: On 19.04.19 11:13, Tanu Kaskinen wrote: On Tue, 2019-04-16 at 21:40 +0200, Georg Chini wrote: On 16.04.19 19:19, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: On 11.04.19 19:36, Tanu Kaskinen wrote: If you want a better plugin standard, are you aware of LV2 and PipeWire's SPA (the latter doesn't seem to be properly documented yet, but to my understanding it's supposed to have a stable and flexible API)? Arun already suggested the pipewire SPA. I took a look, but it seems not very simple compared to LADSPA. I could not really understand how it works and it appears to support a lot more than just filters. LV2 would also be an option, although it too is pretty complex compared to LADSPA. But at least it's documented and has examples. I just took a look and on the first glance LV2 seems similar to LADSPA. I have to dig into the details though, maybe control arrays and interleaved audio ports are possible there. I'm pretty sure they are possible, but neither of those features are necessary. If the plugin gets the number of bands during the initialization, it can create the appropriate number of non-array control ports. Interleaved audio ports aren't needed either, because PulseAudio can do the deinterleaving before passing the audio to the plugin (like module-ladspa-sink already does). If one's going to write an LV2 plugin, it's best to use standard port types so that all hosts will be able to use the plugin. The problem here is that the number of ports must be known before the initialization because it is something which is already provided by the plugin descriptor. So there seems to be no way to change that number dynamically unless I misread the documentation. But looking at the LV2 standard, it supplies the port type lv2:CVPort (see https://gitlab.com/lv2/lv2/blob/master/lv2/core/lv2core.ttl, line 256) which is what I have been looking for if I read the documentation right. I don't think CVPorts are relevant for this discussion. As far as I can tell, they just provide a different kind of control port, one that can use audio signal as input. You wanted a port that can take an array of values, CVPorts isn't that. The documentation basically says it is an array of floats which is not audio but control data. So it is very relevant here. This is exactly what is needed if you want to support a variable number of bands because you need a variable number of gains. You seem to be right about the requirement to declare all ports in advance. I thought dynamic ports would be the primary benefit of using LV2 rather than creating a custom API based on LADSPA. Concerning interleaved audio format: Up to now I found nothing that explicitly forbids interleaved audio though it appears that the plugins usually provide one audio port per channel. You can't use a plain AudioPort for interleaved audio, because hosts will assume that it operates with mono audio. You can probably define a subclass of AudioPort with different semantics, but then hosts other than PulseAudio won't be able to use the plugin (unless they adopt your extension). Other hosts could still use the plugin because mono would be perfectly acceptable. But I agree that we should not implement something that is not in the specification. What should be possible however is writing an LV2 extension that allows interleaved ports. If hosts do not support this extension, the plugins would be considered mono but could still work. PA can surely deinterleave the input and interleave the output but to me it looks like completely unnecessary copying around of buffers within a real time thread. With interleaved channels, you could directly pass the input and output buffers. Why should we copy the data twice if it can be avoided? Additionally, using one port per channel makes it impossible to adapt the number of channels dynamically when loading the plugin for the reason given above. The reason I suggested deinterleaving in PulseAudio was to allow the plugin to be compatible with other hosts. Without native support for dynamic ports in LV2, such compatibility seems to be hard/impossible to achieve, however. My intention would have been to support both, plugins that use one audio port per channel and plugins that use interleaved channels. Most of the plugins can be implemented as mono plugins and instantiated according to the number of channels, so compatibility would be possible. The question is, which part of LV2 should PA support? Only the core specification or extensions as well? If yes, which extensions? As for dynamically changing channels, I don't see the use case for that. With dynamically I meant choosing the number of channels at the time of instance creation. I don't want to change it at run-time. You say that your extension allows full integration of Andrea's equalizer, but I don't see how it allows
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Fri, 2019-04-19 at 12:03 +0200, Georg Chini wrote: > On 19.04.19 11:13, Tanu Kaskinen wrote: > > On Tue, 2019-04-16 at 21:40 +0200, Georg Chini wrote: > > > On 16.04.19 19:19, Tanu Kaskinen wrote: > > > > On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: > > > > > On 11.04.19 19:36, Tanu Kaskinen wrote: > > > > > > If you want a better plugin standard, are you aware of LV2 > > > > > > and PipeWire's SPA (the latter doesn't seem to be properly > > > > > > documented > > > > > > yet, but to my understanding it's supposed to have a stable and > > > > > > flexible API)? > > > > > Arun already suggested the pipewire SPA. I took a look, but it > > > > > seems not very simple compared to LADSPA. I could not really > > > > > understand how it works and it appears to support a lot more > > > > > than just filters. > > > > LV2 would also be an option, although it too is pretty complex compared > > > > to LADSPA. But at least it's documented and has examples. > > > I just took a look and on the first glance LV2 seems similar > > > to LADSPA. I have to dig into the details though, maybe control > > > arrays and interleaved audio ports are possible there. > > I'm pretty sure they are possible, but neither of those features are > > necessary. If the plugin gets the number of bands during the > > initialization, it can create the appropriate number of non-array > > control ports. Interleaved audio ports aren't needed either, because > > PulseAudio can do the deinterleaving before passing the audio to the > > plugin (like module-ladspa-sink already does). If one's going to write > > an LV2 plugin, it's best to use standard port types so that all hosts > > will be able to use the plugin. > > The problem here is that the number of ports must be known before > the initialization because it is something which is already provided by > the plugin descriptor. So there seems to be no way to change that > number dynamically unless I misread the documentation. But looking > at the LV2 standard, it supplies the port type lv2:CVPort (see > https://gitlab.com/lv2/lv2/blob/master/lv2/core/lv2core.ttl, line 256) > which is what I have been looking for if I read the documentation > right. I don't think CVPorts are relevant for this discussion. As far as I can tell, they just provide a different kind of control port, one that can use audio signal as input. You wanted a port that can take an array of values, CVPorts isn't that. You seem to be right about the requirement to declare all ports in advance. I thought dynamic ports would be the primary benefit of using LV2 rather than creating a custom API based on LADSPA. > Concerning interleaved audio format: Up to now I found nothing > that explicitly forbids interleaved audio though it appears that the > plugins usually provide one audio port per channel. You can't use a plain AudioPort for interleaved audio, because hosts will assume that it operates with mono audio. You can probably define a subclass of AudioPort with different semantics, but then hosts other than PulseAudio won't be able to use the plugin (unless they adopt your extension). > PA can surely deinterleave the input and interleave the output > but to me it looks like completely unnecessary copying around > of buffers within a real time thread. With interleaved channels, > you could directly pass the input and output buffers. Why should > we copy the data twice if it can be avoided? Additionally, using > one port per channel makes it impossible to adapt the number > of channels dynamically when loading the plugin for the reason > given above. The reason I suggested deinterleaving in PulseAudio was to allow the plugin to be compatible with other hosts. Without native support for dynamic ports in LV2, such compatibility seems to be hard/impossible to achieve, however. As for dynamically changing channels, I don't see the use case for that. > > > > > > You say that your extension allows full integration of Andrea's > > > > > > equalizer, but I don't see how it allows the host to tell the plugin > > > > > > how many channels and how many frequency bands it should initialize. > > > > > For an interleaved audio port, there would be another control > > > > > port which holds the number of (interleaved) channels. So > > > > > this port would allow you to change the number of channels. > > > > > You could for example have an audio port named "Input" > > > > > and a control port "Number of input channels". Then the > > > > > get_info_port() function would return the index of the > > > > > "Number of input channels" control when called with the > > > > > "Input" port as argument. Or the other way round: If you > > > > > set "Number of input channels" to 6 the plugin will expect > > > > > 6 channels in the interleaved audio port (and you know > > > > > which control port sets the number of channels because > > > > > you can get it via the get_info_port() function. > > > > > > > > > > The same applies to the number of
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 19.04.19 11:13, Tanu Kaskinen wrote: On Tue, 2019-04-16 at 21:40 +0200, Georg Chini wrote: On 16.04.19 19:19, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: On 11.04.19 19:36, Tanu Kaskinen wrote: If you want a better plugin standard, are you aware of LV2 and PipeWire's SPA (the latter doesn't seem to be properly documented yet, but to my understanding it's supposed to have a stable and flexible API)? Arun already suggested the pipewire SPA. I took a look, but it seems not very simple compared to LADSPA. I could not really understand how it works and it appears to support a lot more than just filters. LV2 would also be an option, although it too is pretty complex compared to LADSPA. But at least it's documented and has examples. I just took a look and on the first glance LV2 seems similar to LADSPA. I have to dig into the details though, maybe control arrays and interleaved audio ports are possible there. I'm pretty sure they are possible, but neither of those features are necessary. If the plugin gets the number of bands during the initialization, it can create the appropriate number of non-array control ports. Interleaved audio ports aren't needed either, because PulseAudio can do the deinterleaving before passing the audio to the plugin (like module-ladspa-sink already does). If one's going to write an LV2 plugin, it's best to use standard port types so that all hosts will be able to use the plugin. The problem here is that the number of ports must be known before the initialization because it is something which is already provided by the plugin descriptor. So there seems to be no way to change that number dynamically unless I misread the documentation. But looking at the LV2 standard, it supplies the port type lv2:CVPort (see https://gitlab.com/lv2/lv2/blob/master/lv2/core/lv2core.ttl, line 256) which is what I have been looking for if I read the documentation right. Concerning interleaved audio format: Up to now I found nothing that explicitly forbids interleaved audio though it appears that the plugins usually provide one audio port per channel. PA can surely deinterleave the input and interleave the output but to me it looks like completely unnecessary copying around of buffers within a real time thread. With interleaved channels, you could directly pass the input and output buffers. Why should we copy the data twice if it can be avoided? Additionally, using one port per channel makes it impossible to adapt the number of channels dynamically when loading the plugin for the reason given above. You say that your extension allows full integration of Andrea's equalizer, but I don't see how it allows the host to tell the plugin how many channels and how many frequency bands it should initialize. For an interleaved audio port, there would be another control port which holds the number of (interleaved) channels. So this port would allow you to change the number of channels. You could for example have an audio port named "Input" and a control port "Number of input channels". Then the get_info_port() function would return the index of the "Number of input channels" control when called with the "Input" port as argument. Or the other way round: If you set "Number of input channels" to 6 the plugin will expect 6 channels in the interleaved audio port (and you know which control port sets the number of channels because you can get it via the get_info_port() function. The same applies to the number of bands. There must be a control port which reflects the number of elements in the control array which is the same as the number of bands. Both values can be set to convenient defaults if the host does not supply them (like 5 bands and 2 channels). Ok, so the idea is to do the configuration while the filter is running. I think it would be better to do the configuration in the plugin setup phase. I imagine that would simplify the control port allocoation and management, since the setup doesn't have to run in the IO thread where malloc() is not allowed. I don't see much benefit in doing this kind of configuration while the filter is running, since the filter state most likely has to be reset anyway when the number of EQ bands is changed. There could be a function for getting a description of what options the plugin accepts, and a setup function for setting the options. Why do you think that the filter must be configured while it is running? In case of the equalizer the number of channels and also the number of bands are known before the filter is run. The LADSPA standard says all control ports must be connected (and valid) before you can run the filter. If a parameter changes at runtime, the filter must be reset like the current ladspa-sink does. Control ports are used for realtime parameter changes, so that's why I thought the intention was to set the parameters while the filter is running. I think it would be much clearer and easier to document the expected
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Tue, 2019-04-16 at 21:40 +0200, Georg Chini wrote: > On 16.04.19 19:19, Tanu Kaskinen wrote: > > On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: > > > On 11.04.19 19:36, Tanu Kaskinen wrote: > > > > If you want a better plugin standard, are you aware of LV2 > > > > and PipeWire's SPA (the latter doesn't seem to be properly documented > > > > yet, but to my understanding it's supposed to have a stable and > > > > flexible API)? > > > Arun already suggested the pipewire SPA. I took a look, but it > > > seems not very simple compared to LADSPA. I could not really > > > understand how it works and it appears to support a lot more > > > than just filters. > > LV2 would also be an option, although it too is pretty complex compared > > to LADSPA. But at least it's documented and has examples. > > I just took a look and on the first glance LV2 seems similar > to LADSPA. I have to dig into the details though, maybe control > arrays and interleaved audio ports are possible there. I'm pretty sure they are possible, but neither of those features are necessary. If the plugin gets the number of bands during the initialization, it can create the appropriate number of non-array control ports. Interleaved audio ports aren't needed either, because PulseAudio can do the deinterleaving before passing the audio to the plugin (like module-ladspa-sink already does). If one's going to write an LV2 plugin, it's best to use standard port types so that all hosts will be able to use the plugin. > > > > You say that your extension allows full integration of Andrea's > > > > equalizer, but I don't see how it allows the host to tell the plugin > > > > how many channels and how many frequency bands it should initialize. > > > For an interleaved audio port, there would be another control > > > port which holds the number of (interleaved) channels. So > > > this port would allow you to change the number of channels. > > > You could for example have an audio port named "Input" > > > and a control port "Number of input channels". Then the > > > get_info_port() function would return the index of the > > > "Number of input channels" control when called with the > > > "Input" port as argument. Or the other way round: If you > > > set "Number of input channels" to 6 the plugin will expect > > > 6 channels in the interleaved audio port (and you know > > > which control port sets the number of channels because > > > you can get it via the get_info_port() function. > > > > > > The same applies to the number of bands. There must be a > > > control port which reflects the number of elements in the > > > control array which is the same as the number of bands. > > > > > > Both values can be set to convenient defaults if the host does > > > not supply them (like 5 bands and 2 channels). > > Ok, so the idea is to do the configuration while the filter is running. > > I think it would be better to do the configuration in the plugin setup > > phase. I imagine that would simplify the control port allocoation and > > management, since the setup doesn't have to run in the IO thread where > > malloc() is not allowed. I don't see much benefit in doing this kind of > > configuration while the filter is running, since the filter state most > > likely has to be reset anyway when the number of EQ bands is changed. > > > > There could be a function for getting a description of what options the > > plugin accepts, and a setup function for setting the options. > > > Why do you think that the filter must be configured while it is > running? In case of the equalizer the number of channels and > also the number of bands are known before the filter is run. > The LADSPA standard says all control ports must be connected > (and valid) before you can run the filter. If a parameter changes > at runtime, the filter must be reset like the current ladspa-sink > does. Control ports are used for realtime parameter changes, so that's why I thought the intention was to set the parameters while the filter is running. I think it would be much clearer and easier to document the expected host behaviour if the parameter configuration was not implemented via control ports. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 16.04.19 21:40, Georg Chini wrote: On 16.04.19 19:19, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: On 11.04.19 19:36, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 15:16 +0200, Georg Chini wrote: On 08.04.19 09:27, Georg Chini wrote: On 05.04.19 13:29, Tanu Kaskinen wrote: On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: On 06.11.18 22:14, Andrea A wrote: Hi Andrea, You say that your extension allows full integration of Andrea's equalizer, but I don't see how it allows the host to tell the plugin how many channels and how many frequency bands it should initialize. For an interleaved audio port, there would be another control port which holds the number of (interleaved) channels. So this port would allow you to change the number of channels. You could for example have an audio port named "Input" and a control port "Number of input channels". Then the get_info_port() function would return the index of the "Number of input channels" control when called with the "Input" port as argument. Or the other way round: If you set "Number of input channels" to 6 the plugin will expect 6 channels in the interleaved audio port (and you know which control port sets the number of channels because you can get it via the get_info_port() function. The same applies to the number of bands. There must be a control port which reflects the number of elements in the control array which is the same as the number of bands. Both values can be set to convenient defaults if the host does not supply them (like 5 bands and 2 channels). Ok, so the idea is to do the configuration while the filter is running. I think it would be better to do the configuration in the plugin setup phase. I imagine that would simplify the control port allocoation and management, since the setup doesn't have to run in the IO thread where malloc() is not allowed. I don't see much benefit in doing this kind of configuration while the filter is running, since the filter state most likely has to be reset anyway when the number of EQ bands is changed. There could be a function for getting a description of what options the plugin accepts, and a setup function for setting the options. Why do you think that the filter must be configured while it is running? In case of the equalizer the number of channels and also the number of bands are known before the filter is run. The LADSPA standard says all control ports must be connected (and valid) before you can run the filter. If a parameter changes at runtime, the filter must be reset like the current ladspa-sink does. One more comment: Parameter changes must in the end be done in the IO thread anyway because the run() function is called there. Only the initial setup can be done in the main thread when the IO thread is not yet active. The sequence of a parameter change is that you 1) Receive a parameter change in the main thread 2) Send a message to the IO thread to notify of the change 3) Copy the changed parameter(s) from the main thread 4) Reset the filter (by issuing a rewind request) After that the filter can continue running. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 16.04.19 19:19, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: On 11.04.19 19:36, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 15:16 +0200, Georg Chini wrote: On 08.04.19 09:27, Georg Chini wrote: On 05.04.19 13:29, Tanu Kaskinen wrote: On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: On 06.11.18 22:14, Andrea A wrote: Hi Andrea, maybe there is a chance now to have your equalizer included as a module. The messaging API patches should have their final form (at least I do not think the public functions will change anymore) and today I submitted a patch series that consolidates the code of the current virtual sinks and moves the common code to a separate file. Using the common code should significantly reduce the maintenance cost of an additional sink. So if you are still interested to have it included, at least I would welcome a new patch. Arun, Tanu, what do you think? I think it would anyway make sense to make one or more LADSPA plugins out of the equalizer code (I say one or more, because of the lack of parametrization support in LADSPA). That way the equalizer would be available also to other software than just PulseAudio (I'm thinking PipeWire in particular). If a suitable LADSPA plugin existed, we might or might not still need a separate equalizer module, but in any case we wouldn't need to maintain the DSP code in PulseAudio. If there's some reason why module-ladspa- sink isn't (and can't become) suitable for implementing the integration in PulseAudio, then a specialized module is fine. I'm not saying that I'm dead against hosting the DSP code in PulseAudio, but I'd certainly prefer not to. It surely would make sense to have one or several LADSPA plugins, but for me a good equalizer should be an integral part of pulseaudio. And as you say yourself, the full flexibility cannot be achieved by a single LADSPA plugin. The equalizer we are currently providing is buggy and completely unsupported. The new equalizer would at least be fully documented, so that it is possible to maintain. Additionally I agree with Andrea that handling LADSPA plugins is somewhat cumbersome. From a user point of view, a module is much easier to handle. I have taken a more detailed look on the LADSPA standard and to me it appears like you would not only need different plugins for different numbers of equalizer channels but in addition also for different number of audio channels. Both, the number of single-value parameters and number of input-/output-channels seem to be fixed, so producing a bunch of plugins is rather impractical. An equalizer plugin doesn't need multiple channels, one mono plugin can be instantiated for each channel. Or does this equalizer have some cross-channel effects? You are right, that would not be necessary. Only some plugin that does up- or down-mixing of channels would need that. I wonder if there is a chance to extend the standard a bit to allow a variable number of audio channels and allow control ports to be arrays. It can be done with two more constants and one additional function, see attached diff. This extension would allow to reduce many of our virtual sinks to one plugin-sink and also allow full integration of Andrea's equalizer. Do you have in mind actually extending the LADSPA standard (i.e. something to be promoted to other projects as well), or just creating a new custom (i.e. non-standard) plugin API for PulseAudio that is based on LADSPA? It would be actually best if the standard could be extended, but I do not know how feasible this is. The extension I provided will not break any old plugin and would simplify the handling in many cases. The next best thing would be to do it only for PA. I doubt the wider Linux audio community is very interested in extending LADSPA, since LV2 already exists as a successor. Defining a PA specific filter API would be worth doing, IMO, if it can be kept simple (compared to the alternatives). Of course anyone would be welcome to use it in other projects too (the API should be stable, so there should be no problems with maintaining plugin compatibility). If you want a better plugin standard, are you aware of LV2 and PipeWire's SPA (the latter doesn't seem to be properly documented yet, but to my understanding it's supposed to have a stable and flexible API)? Arun already suggested the pipewire SPA. I took a look, but it seems not very simple compared to LADSPA. I could not really understand how it works and it appears to support a lot more than just filters. LV2 would also be an option, although it too is pretty complex compared to LADSPA. But at least it's documented and has examples. I just took a look and on the first glance LV2 seems similar to LADSPA. I have to dig into the details though, maybe control arrays and interleaved audio ports are possible there. You say that your extension allows full integration of Andrea's equalizer, but I don't see how it allows the host to tell the
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Thu, 2019-04-11 at 20:42 +0200, Georg Chini wrote: > On 11.04.19 19:36, Tanu Kaskinen wrote: > > On Thu, 2019-04-11 at 15:16 +0200, Georg Chini wrote: > > > On 08.04.19 09:27, Georg Chini wrote: > > > > On 05.04.19 13:29, Tanu Kaskinen wrote: > > > > > On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: > > > > > > On 06.11.18 22:14, Andrea A wrote: > > > > > > Hi Andrea, > > > > > > > > > > > > > > > > > > maybe there is a chance now to have your equalizer included as a > > > > > > module. > > > > > > The messaging API patches > > > > > > should have their final form (at least I do not think the public > > > > > > functions will change anymore) and today > > > > > > I submitted a patch series that consolidates the code of the current > > > > > > virtual sinks and moves the common > > > > > > code to a separate file. Using the common code should significantly > > > > > > reduce the maintenance cost of an > > > > > > additional sink. > > > > > > > > > > > > So if you are still interested to have it included, at least I would > > > > > > welcome a new patch. > > > > > > > > > > > > > > > > > > Arun, Tanu, what do you think? > > > > > I think it would anyway make sense to make one or more LADSPA plugins > > > > > out of the equalizer code (I say one or more, because of the lack of > > > > > parametrization support in LADSPA). That way the equalizer would be > > > > > available also to other software than just PulseAudio (I'm thinking > > > > > PipeWire in particular). > > > > > > > > > > If a suitable LADSPA plugin existed, we might or might not still need > > > > > a > > > > > separate equalizer module, but in any case we wouldn't need to > > > > > maintain > > > > > the DSP code in PulseAudio. If there's some reason why module-ladspa- > > > > > sink isn't (and can't become) suitable for implementing the > > > > > integration > > > > > in PulseAudio, then a specialized module is fine. > > > > > > > > > > I'm not saying that I'm dead against hosting the DSP code in > > > > > PulseAudio, but I'd certainly prefer not to. > > > > > > > > > It surely would make sense to have one or several LADSPA > > > > plugins, but for me a good equalizer should be an integral > > > > part of pulseaudio. And as you say yourself, the full flexibility > > > > cannot be achieved by a single LADSPA plugin. The equalizer > > > > we are currently providing is buggy and completely unsupported. > > > > The new equalizer would at least be fully documented, so that > > > > it is possible to maintain. Additionally I agree with Andrea that > > > > handling LADSPA plugins is somewhat cumbersome. From a > > > > user point of view, a module is much easier to handle. > > > I have taken a more detailed look on the LADSPA standard and > > > to me it appears like you would not only need different plugins for > > > different numbers of equalizer channels but in addition also > > > for different number of audio channels. Both, the number of > > > single-value parameters and number of input-/output-channels > > > seem to be fixed, so producing a bunch of plugins is rather > > > impractical. > > An equalizer plugin doesn't need multiple channels, one mono plugin can > > be instantiated for each channel. Or does this equalizer have some > > cross-channel effects? > > You are right, that would not be necessary. Only some plugin > that does up- or down-mixing of channels would need that. > > > > I wonder if there is a chance to extend the standard a bit to > > > allow a variable number of audio channels and allow control > > > ports to be arrays. It can be done with two more constants and > > > one additional function, see attached diff. This extension would > > > allow to reduce many of our virtual sinks to one plugin-sink and > > > also allow full integration of Andrea's equalizer. > > Do you have in mind actually extending the LADSPA standard (i.e. > > something to be promoted to other projects as well), or just creating a > > new custom (i.e. non-standard) plugin API for PulseAudio that is based > > on LADSPA? > > It would be actually best if the standard could be extended, but > I do not know how feasible this is. The extension I provided will > not break any old plugin and would simplify the handling in many > cases. > The next best thing would be to do it only for PA. I doubt the wider Linux audio community is very interested in extending LADSPA, since LV2 already exists as a successor. Defining a PA specific filter API would be worth doing, IMO, if it can be kept simple (compared to the alternatives). Of course anyone would be welcome to use it in other projects too (the API should be stable, so there should be no problems with maintaining plugin compatibility). > > If you want a better plugin standard, are you aware of LV2 > > and PipeWire's SPA (the latter doesn't seem to be properly documented > > yet, but to my understanding it's supposed to have a stable and > > flexible API)? > > Arun already suggested the pipewire
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 11.04.19 19:36, Tanu Kaskinen wrote: On Thu, 2019-04-11 at 15:16 +0200, Georg Chini wrote: On 08.04.19 09:27, Georg Chini wrote: On 05.04.19 13:29, Tanu Kaskinen wrote: On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: On 06.11.18 22:14, Andrea A wrote: Hi Andrea, maybe there is a chance now to have your equalizer included as a module. The messaging API patches should have their final form (at least I do not think the public functions will change anymore) and today I submitted a patch series that consolidates the code of the current virtual sinks and moves the common code to a separate file. Using the common code should significantly reduce the maintenance cost of an additional sink. So if you are still interested to have it included, at least I would welcome a new patch. Arun, Tanu, what do you think? I think it would anyway make sense to make one or more LADSPA plugins out of the equalizer code (I say one or more, because of the lack of parametrization support in LADSPA). That way the equalizer would be available also to other software than just PulseAudio (I'm thinking PipeWire in particular). If a suitable LADSPA plugin existed, we might or might not still need a separate equalizer module, but in any case we wouldn't need to maintain the DSP code in PulseAudio. If there's some reason why module-ladspa- sink isn't (and can't become) suitable for implementing the integration in PulseAudio, then a specialized module is fine. I'm not saying that I'm dead against hosting the DSP code in PulseAudio, but I'd certainly prefer not to. It surely would make sense to have one or several LADSPA plugins, but for me a good equalizer should be an integral part of pulseaudio. And as you say yourself, the full flexibility cannot be achieved by a single LADSPA plugin. The equalizer we are currently providing is buggy and completely unsupported. The new equalizer would at least be fully documented, so that it is possible to maintain. Additionally I agree with Andrea that handling LADSPA plugins is somewhat cumbersome. From a user point of view, a module is much easier to handle. I have taken a more detailed look on the LADSPA standard and to me it appears like you would not only need different plugins for different numbers of equalizer channels but in addition also for different number of audio channels. Both, the number of single-value parameters and number of input-/output-channels seem to be fixed, so producing a bunch of plugins is rather impractical. An equalizer plugin doesn't need multiple channels, one mono plugin can be instantiated for each channel. Or does this equalizer have some cross-channel effects? You are right, that would not be necessary. Only some plugin that does up- or down-mixing of channels would need that. I wonder if there is a chance to extend the standard a bit to allow a variable number of audio channels and allow control ports to be arrays. It can be done with two more constants and one additional function, see attached diff. This extension would allow to reduce many of our virtual sinks to one plugin-sink and also allow full integration of Andrea's equalizer. Do you have in mind actually extending the LADSPA standard (i.e. something to be promoted to other projects as well), or just creating a new custom (i.e. non-standard) plugin API for PulseAudio that is based on LADSPA? It would be actually best if the standard could be extended, but I do not know how feasible this is. The extension I provided will not break any old plugin and would simplify the handling in many cases. The next best thing would be to do it only for PA. If you want a better plugin standard, are you aware of LV2 and PipeWire's SPA (the latter doesn't seem to be properly documented yet, but to my understanding it's supposed to have a stable and flexible API)? Arun already suggested the pipewire SPA. I took a look, but it seems not very simple compared to LADSPA. I could not really understand how it works and it appears to support a lot more than just filters. You say that your extension allows full integration of Andrea's equalizer, but I don't see how it allows the host to tell the plugin how many channels and how many frequency bands it should initialize. For an interleaved audio port, there would be another control port which holds the number of (interleaved) channels. So this port would allow you to change the number of channels. You could for example have an audio port named "Input" and a control port "Number of input channels". Then the get_info_port() function would return the index of the "Number of input channels" control when called with the "Input" port as argument. Or the other way round: If you set "Number of input channels" to 6 the plugin will expect 6 channels in the interleaved audio port (and you know which control port sets the number of channels because you can get it via the get_info_port() function. The same applies to the number of bands. There must be
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Thu, 2019-04-11 at 15:16 +0200, Georg Chini wrote: > On 08.04.19 09:27, Georg Chini wrote: > > On 05.04.19 13:29, Tanu Kaskinen wrote: > > > On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: > > > > On 06.11.18 22:14, 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. > > > > > > > > > > > Iwould 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 > > > > > > > > > > > > > > Hi Andrea, > > > > > > > > > > > > maybe there is a chance now to have your equalizer included as a > > > > module. > > > > The messaging API patches > > > > should have their final form (at least I do not think the public > > > > functions will change anymore) and today > > > > I submitted a patch series that consolidates the code of the current > > > > virtual sinks and moves the common > > > > code to a separate file. Using the common code should significantly > > > > reduce the maintenance cost of an > > > > additional sink. > > > > > > > > So if you are still interested to have it included, at least I would > > > > welcome a new patch. > > > > > > > > > > > > Arun, Tanu, what do you think? > > > I think it would anyway make sense to make one or more LADSPA plugins > > > out of the equalizer code (I say one or more, because of the lack of > > > parametrization support in LADSPA). That way the equalizer would be > > > available also to other software than just PulseAudio (I'm thinking > > > PipeWire in particular). > > > > > > If a suitable LADSPA plugin existed, we might or might not still need a > > > separate equalizer module, but in any case we wouldn't need to maintain > > > the DSP code in PulseAudio. If there's some reason why module-ladspa- > > > sink isn't (and can't become) suitable for implementing the integration > > > in PulseAudio, then a specialized module is fine. > > > > > > I'm not saying that I'm dead against hosting the DSP code in > > > PulseAudio, but I'd certainly prefer not to. > > > > > It surely would make sense to have one or several LADSPA > > plugins, but for me a good equalizer should be an integral > > part of pulseaudio. And as you say yourself, the full flexibility > > cannot be achieved by a single LADSPA plugin. The equalizer > > we are currently providing is buggy and completely unsupported. > > The new equalizer would at least be fully documented, so that > > it is possible to maintain. Additionally I agree with Andrea that > > handling LADSPA plugins is somewhat cumbersome. From a > > user point of view, a module is much easier to handle. > I have taken a more detailed look on the LADSPA standard and > to me it appears like you would not only need different plugins for > different numbers of equalizer channels but in addition also > for different number of audio channels. Both, the number of > single-value parameters and number of input-/output-channels > seem to be fixed, so producing a bunch of plugins is rather > impractical. An equalizer plugin doesn't need multiple channels, one mono plugin can be instantiated for each channel. Or does this equalizer have some cross-channel effects? > I wonder if there is a chance to extend the standard a bit to > allow a variable number of audio channels and allow control > ports to be arrays. It can be
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
пн, 8 апр. 2019 г. в 13:08, Alexander E. Patrakov : > I have looked again at the paper, and have a DSP question. > > The thesis starts with designing a shelf filter. That is, a filter > that contains a flat area at low frequencies, a flat area at high > frequencies (but with a different gain), and some transition in > between. Then, for some unknown reason, it is transformed into a > band-pass shelving filter. Then there is some talk how to "stitch" > together several band-pass shelving filters. > > Why would one need to do this at all? I.e., why can't one just cascade > several shelf filters, designed according to the correct transition > frequencies, with the height of the shelf being the dB difference of > the desired gains of the neighboring bands? Just for some context: the reason why I asked is the ripple on Figure 2.5 that seems to be avoidable in my approach. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
пн, 8 апр. 2019 г. в 12:27, Georg Chini : > > On 05.04.19 13:29, Tanu Kaskinen wrote: > > On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: > >> On 06.11.18 22:14, 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. > >>> > Iwould 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 > >>> > >>> > >> Hi Andrea, > >> > >> > >> maybe there is a chance now to have your equalizer included as a module. > >> The messaging API patches > >> should have their final form (at least I do not think the public > >> functions will change anymore) and today > >> I submitted a patch series that consolidates the code of the current > >> virtual sinks and moves the common > >> code to a separate file. Using the common code should significantly > >> reduce the maintenance cost of an > >> additional sink. > >> > >> So if you are still interested to have it included, at least I would > >> welcome a new patch. > >> > >> > >> Arun, Tanu, what do you think? > > I think it would anyway make sense to make one or more LADSPA plugins > > out of the equalizer code (I say one or more, because of the lack of > > parametrization support in LADSPA). That way the equalizer would be > > available also to other software than just PulseAudio (I'm thinking > > PipeWire in particular). > > > > If a suitable LADSPA plugin existed, we might or might not still need a > > separate equalizer module, but in any case we wouldn't need to maintain > > the DSP code in PulseAudio. If there's some reason why module-ladspa- > > sink isn't (and can't become) suitable for implementing the integration > > in PulseAudio, then a specialized module is fine. > > > > I'm not saying that I'm dead against hosting the DSP code in > > PulseAudio, but I'd certainly prefer not to. > > > It surely would make sense to have one or several LADSPA > plugins, but for me a good equalizer should be an integral > part of pulseaudio. And as you say yourself, the full flexibility > cannot be achieved by a single LADSPA plugin. The equalizer > we are currently providing is buggy and completely unsupported. > The new equalizer would at least be fully documented, so that > it is possible to maintain. Additionally I agree with Andrea that > handling LADSPA plugins is somewhat cumbersome. From a > user point of view, a module is much easier to handle. I have looked again at the paper, and have a DSP question. The thesis starts with designing a shelf filter. That is, a filter that contains a flat area at low frequencies, a flat area at high frequencies (but with a different gain), and some transition in between. Then, for some unknown reason, it is transformed into a band-pass shelving filter. Then there is some talk how to "stitch" together several band-pass shelving filters. Why would one need to do this at all? I.e., why can't one just cascade several shelf filters, designed according to the correct transition frequencies, with the height of the shelf being the dB difference of the desired gains of the neighboring bands? -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 05.04.19 13:29, Tanu Kaskinen wrote: On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: On 06.11.18 22:14, 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. Iwould 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 Hi Andrea, maybe there is a chance now to have your equalizer included as a module. The messaging API patches should have their final form (at least I do not think the public functions will change anymore) and today I submitted a patch series that consolidates the code of the current virtual sinks and moves the common code to a separate file. Using the common code should significantly reduce the maintenance cost of an additional sink. So if you are still interested to have it included, at least I would welcome a new patch. Arun, Tanu, what do you think? I think it would anyway make sense to make one or more LADSPA plugins out of the equalizer code (I say one or more, because of the lack of parametrization support in LADSPA). That way the equalizer would be available also to other software than just PulseAudio (I'm thinking PipeWire in particular). If a suitable LADSPA plugin existed, we might or might not still need a separate equalizer module, but in any case we wouldn't need to maintain the DSP code in PulseAudio. If there's some reason why module-ladspa- sink isn't (and can't become) suitable for implementing the integration in PulseAudio, then a specialized module is fine. I'm not saying that I'm dead against hosting the DSP code in PulseAudio, but I'd certainly prefer not to. It surely would make sense to have one or several LADSPA plugins, but for me a good equalizer should be an integral part of pulseaudio. And as you say yourself, the full flexibility cannot be achieved by a single LADSPA plugin. The equalizer we are currently providing is buggy and completely unsupported. The new equalizer would at least be fully documented, so that it is possible to maintain. Additionally I agree with Andrea that handling LADSPA plugins is somewhat cumbersome. From a user point of view, a module is much easier to handle. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Tue, 2019-04-02 at 20:28 +0200, Georg Chini wrote: > On 06.11.18 22:14, 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. > > > > > Iwould 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 > > > > > > Hi Andrea, > > > maybe there is a chance now to have your equalizer included as a module. > The messaging API patches > should have their final form (at least I do not think the public > functions will change anymore) and today > I submitted a patch series that consolidates the code of the current > virtual sinks and moves the common > code to a separate file. Using the common code should significantly > reduce the maintenance cost of an > additional sink. > > So if you are still interested to have it included, at least I would > welcome a new patch. > > > Arun, Tanu, what do you think? I think it would anyway make sense to make one or more LADSPA plugins out of the equalizer code (I say one or more, because of the lack of parametrization support in LADSPA). That way the equalizer would be available also to other software than just PulseAudio (I'm thinking PipeWire in particular). If a suitable LADSPA plugin existed, we might or might not still need a separate equalizer module, but in any case we wouldn't need to maintain the DSP code in PulseAudio. If there's some reason why module-ladspa- sink isn't (and can't become) suitable for implementing the integration in PulseAudio, then a specialized module is fine. I'm not saying that I'm dead against hosting the DSP code in PulseAudio, but I'd certainly prefer not to. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On 06.11.18 22:14, 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. >Iwould 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 Hi Andrea, maybe there is a chance now to have your equalizer included as a module. The messaging API patches should have their final form (at least I do not think the public functions will change anymore) and today I submitted a patch series that consolidates the code of the current virtual sinks and moves the common code to a separate file. Using the common code should significantly reduce the maintenance cost of an additional sink. So if you are still interested to have it included, at least I would welcome a new patch. Arun, Tanu, what do you think? Regards Georg *Da:* pulseaudio-discuss per conto di Tanu Kaskinen *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 : > > 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
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
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 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
On Tue, 2018-11-06 at 21:14 +, 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 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
Andrea A : > 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 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] R: New equalizer module (module-eqpro-sink), some questions
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 Thank you very much, I will look. It will be slower than usual because I will have to use Google Translate. -- Alexander E. Patrakov ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
