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

[Georg Chini]

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 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.
>
Yes, probably in most cases the size will not change but I think it
makes sense to keep the interface as flexible as possible. I will see
what I can do to support both cases.