[pulseaudio-discuss] new module module-plugin-sink
Georg Chini
georg at chini.tk
Mon May 13 08:19:23 UTC 2019
On 10.05.19 19:27, Tanu Kaskinen wrote:
> On Sat, 2019-05-04 at 22:41 +0200, Georg Chini wrote:
> I don't see the need for copying the channel position and error enums.
> We can #include the relevant headers.
I thought the point was to create a header file that allows development
of filters completely independent from pulseaudio, that is you can compile
the code without having PA installed. If that was not the intention, we can
include the headers. Can we then also link with libpulse?
>
>> /* Function called when the filter detects a condition to unload the filter. */
>> typedef void (*Kill_Filter_Function)(void *module);
> What's the use case for for filter self-destruction?
A filter might detect that it is no longer appropriate based on the
sink/port information it receives. Then it can either disable itself
internally or just kill itself, so that PA will take over processing.
>
> I think we should use the normal PA naming conventions, so this would
> be named pa_filter_plugin_kill_cb_t.
Why should we follow PA naming conventions? Again, I thought
it should be rather independent from PA and authors of filters
do not need to know PA internals. That's one of the reasons I
dropped the PA_ prefix for channel map and error values.
If you prefer, I can nevertheless change back to PA conventions.
>
>> /* Filter plugin structure */
>> typedef struct filter_plugin {
>> const char *name; /* Name of the filter. Used to identify the filter. */
> Is this expected to be globally unique? Are there limitations on length
> or accepted characters? Would there be problems with using the .so file
> name as the name? I expect the .so file name to be needed for
> identification anyway, and it's not clear to me what benefit another
> layer of naming brings.
It would be nice to have this globally unique, but I think that is difficult
to ensure. The concept of loading filters follows the LADSPA scheme,
so one file can contain multiple filters. See comment to the
get_Filter_Info() function.
>
>> const char *desc_head; /* Leading part of description string used for the
>> * sink and sink input description in PA. */
> Can you give an example how this is used? What about sources, maybe
> we'll one day have module-plugin-source too?
The sources we currently have are quite different and not easy
to consolidate. So I decided (for the moment) to leave it at that.
>
> Is this the same thing that is returned as the filter description in
> the parameter-get-description message? If so, I think "description"
> would be a better name for this field than "desc_head".
No, this has nothing to do with the parameter description. This
is just a bit of text that is used in the description property of
sink and sink input. For the virtual sink it would just be
"Virtual Sink" or "LADSPA Sink" for the ladspa sink. There is
surely a better name for it ...
>
>> const char *filter_type; /* Name for the type of filter, used as suffix for
>> * the sink name if the name is derived from the
>> * master sink. */
> I believe using a prefix is better than suffix. A tunnel sink has
> prefix "tunnel.", so if there's a sink named "tunnel.foo.eq", it's
> ambiguous whether that's a tunnel sink connected to remote "foo.eq"
> sink or a local eq sink using "tunnel.foo" as master. "eq.tunnel.foo"
> doesn't have this problem.
The current virtual sinks all use a suffix. Are you OK with changing
that for all virtual sinks?
>
>> unsigned input_channels; /* Number of audio input channels, 0 if variable */
>> unsigned output_channels; /* Number of audio output channels, 0 if variable */
> How are these used? You wanted to avoid deinterleaving, so I guess mono
> filters are expected to support variable channels. What kind of filters
> are going to set fixed channels? Is a filter expected to support all
> possible channel counts if it supports more than one channel count?
Either the filter provides a fixed number of channels (for example
2 output and 6 input channels for a virtual surround filter) or the
number of channels can be set freely (based on master sink and/or
provided channel maps). There may be filter specific conditions like
number of in/out channels being equal, but this must be checked
by the filter at initialization time.
>
>> size_t max_chunk_size; /* Maximum chunk size in bytes that the filter will
>> * accept, 0 for default */
> Shouldn't this be in frames rather than in bytes, since the other
> fields use frames? And what's the difference between "chunk" and
> "block"? If there's none, then let's call it a "block" always.
>
> What's the default chunk size? Does 0 mean that the chunk size is
> allowed to change between process_chunk() calls?
>
> Is this even necessary? LADSPA doesn't define a maximum block size.
This is supposed to be a memory limit like 32kB, regardless of
the number of channels, that's why it is not in frames. I am
not sure if it is necessary, it was something that was set by
one of the existing filters. Default (and maximum) is
pa_frame_align(pa_mempool_block_size_max(s->core->mempool), &s->sample_spec)
>
>> size_t fixed_block_size; /* Block size in frames for fixed block size filters,
>> * 0 if block size is variable */
> I guess this is added here, because module-echo-cancel uses a fixed
> block size. LADSPA plugins don't seem to care about the block size.
>
> With module-echo-cancel the block size is configurable, but this API
> doesn't allow configuring the block size. My suggestion would be to
> replace this field with a block size initialization parameter. If the
> plugin doesn't have that initialization parameter, then it's assumed to
> accept variable block sizes. (More on initialization parameters later.)
This is for the case that the filter requires a specific block size.
Then it will specify it in this value. If the filter does not provide
a block size, it is assumed, that it can handle variable block sizes.
What additional API would be needed?
>
>> int max_rewind; /* Maximum number of frames that the sink can rewind.
>> * 0 means use value from master sink, -1 disables
>> * rewinding. Unless the filter implements rewinding
>> * the value should be set to -1. */
> "0 means use value from master sink" - wouldn't it be better to define
> 0 simply as "unlimited"? That is, the plugin doesn't impose any
> limitation on the maximum rewind amount; the hardware of course defines
> some maximum.
I meant unlimited.
>
>> /* Functions defined by the filter */
>>
>> /* Initializes a new filter instance and returns a handle to it. Passes the number of
>> * input and output channels, the corresponding channel positions and the sample rate
>> * to the filter. kill_filter can be called by the filter if it detects a condition
>> * that makes it necessary to remove the filter. kill_filter must be called with
>> * the module pointer as argument. */
>> void *(*create_filter)(unsigned input_channels, int *input_map,
>> unsigned output_channels, int *output_map,
>> Kill_Filter_Function kill_filter, void *module, unsigned sample_rate);
> There doesn't seem to be any concept of initialization parameters,
> except the ones that are explicitly listed here in the create_filter()
> arguments. I thought the main motivation for this exercise was to
> facilitate the new eq algorithm that supports different amounts of
> frequency bands. How can the eq bands be configured? Are you still
> suggesting that the number of bands should be a regular parameter?
Yes, this can easily be coded in the meta data. Naturally, a parameter
which changes the total number of parameters cannot be set alone,
but only with parameter-set-all.
>
> My suggestion would be to have an initialization parameter API to do
> the following:
>
> - query the initialization parameters that the plugin supports (e.g.
> block size, sample rate, eq bands)
> - query the default values for the initialization parameters
> - set the initialization parameters
Each filter should come with a reasonable set of default parameters
in place, because, contrary to LADSPA, it is the filters responsibility to
handle and provide parameter structures. These default values can
be queried using the meta data and changed as needed. All data
needed by PA for the initialization should be in the filter_plugin structure
that the filter provides, and the things that the filter needs to know
should be passed in the create() call. Therefore I cannot see the point
of introducing a special API. Can you provide a use case that can not
be handled by the current approach?
>
> The metadata for the initialization parameters should probably be the
> same as for runtime parameters: identifier, description, type, default
> value, range. Some initialization parameters would have their
> identifiers and semantics defined by the specification (at least block
> size and sample rate).
See above.
>
> I'm not sure if it makes sense to make the channel configuration part
> of the initialization parameters. If the initialization parameter API
> should only cover parameters that the user can change, then the sample
> rate shouldn't be an initialization parameter either.
Well, the filter needs to know the sample rate, so it must be passed
somewhere. Sample rate and channel information are critical for
the function of the filter, so I think they must be passed at creation
time.
>
> I think the void pointers should be replaced with typedefs, so that
> it's clear what's what. Void pointers are used for several things in
> this API.
OK.
>
> The "module" parameter could be renamed to kill_cb_data to not expose
> unnecessary implementation details, and to make its purpose clearer.
OK, but again I do not see the need to stick to PA naming conventions.
>
>> /* Deletes filter instance. */
>> void (*delete_filter)(void *filter_handle);
>>
>> /* Activates filter. Returns 0 on success and a negative errror code on failure.
>> * May be NULL. */
>> int (*activate_filter)(void *filter_handle);
>>
>> /* Deactivates filter. May be NULL. */
>> void (*deactivate_filter)(void *filter_handle);
>>
>> /* 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);
>>
>> /* Callback to rewind the filter when pulseaudio requests it. May be NULL.
>> * Amount indicates the number of bytes to roll back. 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 it is not possible to do so, the best option is to disable rewinding
>> * completely and limit the latency. */
>> void (*rewind_filter)(size_t amount, void *filter_handle);
>>
>> /* If not NULL, this function is called whenever the active port of a sink or
>> * the sink itself changes. It is used to communicate the currently active port
>> * and sink to the filter. */
>> void (*output_changed)(const char *sink_name, const char *port_name, void *filter_handle);
> What is the filter supposed to do with this information? I recall from
> the earlier discussion that the filter could disable itself, if the
> sink name and the port name look somehow bad, but there doesn't seem to
> be any mechanism for telling the filter which sinks and ports are bad.
> I don't think this logic belongs in the filter anyway, the policy code
> for enabling/disabling filters automatically should be implemented
> separately.
See Alexanders response to that topic. I think, that the sink information
is only there to detect if the filter has moved, while the port information
may be something that a filter can use. It might for example distinguish
between playing on speakers or headphones. The sink/port information
might also be something that a GUI wants to query.
>
>> /* If set, this function is called in thread context when an update of the
> For clarity, "thread context" -> "the IO thread context".
>
> The call context should be documented for all functions.
>
>> * 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);
> I'd rename this to "parameters_free" because the function handles
> parameter sets, not single parameters. Even better would be
> "free_parameters", because that's better English. I guess you chose to
> put "free" last, because most of the time freeing functions have "free"
> at the end of the function name, but that's because therey're part of a
> namespace (like "pa_idxset_free" is a part of namespace "pa_idxset_".)
>
> Maybe we should call them "filter parameters" instead of just
> "parameters" (you did this with "update_filter_parameters" too, rather
> than calling it just "update_parameters"). If we add the initialization
> parameter concept, then there will be two kinds of 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 host code makes no assumption on the contents
>> * of the structure but only passes references. The host 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. */
> I think parameter_get() should return a plain string.
>
> I don't like that parameter_get_all() is required to wrap the parameter
> values in the message API format, but that's probably the pragmatic
> choice since returning an array would probably only cause extra
> overhead when the array needs to be allocated, populated, and finally
> converted to the message API format anyway.
I don't like it either and you are right, this has been a pragmatic
choice. I decided to wrap the response to parameter-get in curly
braces for consistency, so that all the get messages return a
string in message format (and the caller can simply pass it on
to the messaging functions) while all set messages receive plain
strings (and the filter needs not parse the messaging format).
>
> One possibility would be to handle the parameter-get-all message in the
> host, which would call parameter_get() in a loop. The filter would have
> to somehow provide the control names in order for this to be possible.
> If we choose that path, then we could consider implementing the
> parameter-get-description message handler in the host too. The filter
> would provide the necessary metadata in some appropriate C structures.
> I don't expect this to reduce complexity, though, it's just a way to
> avoid any message API details in the filter API.
I tried to simplify it for the filter side so that it does not have to
parse message strings. I think we should provide some utility
functions for constructing message strings, similar to what PA
has. I do not like calling parameter-set (or parameter-get) in a loop,
this would mean that the parameters change one by one which
might be audible, especially if you think about changing parameters
that influence the size of the parameter set.
If we want to avoid that the filter has to construct strings, I would
rather change the interface to passing arrays, though that means
allocating and de-allocating a lot of strings.
>
> I think "filter" would be a better prefix for the message names than
> "parameter". "parameter-get-description" in particular doesn't seem
> very good to me, because the message returns some information that is
> not related to parameters.
>
> Since filters have the desc_head field (which may get renamed to
> "description"), "filter-get-description" sounds like it only returns
> that field. Since it actually returns the full metadata, it should be
> named "filter-get-metadata".
Agreed.
>
> I'd also rename the get/set functions:
>
> get_(filter_)parameter
> set_(filter_)parameter
> get_all_(filter_)parameters
> set_all_(filter_)parameters
Agreed.
>
>> /* 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 on 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, 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 *filter_handle);
>>
>> /* 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 *filter_handle);
>>
>> /* 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().
>> * If all_params is NULL, the function should return a default set of parameters. */
>> void *(*parameter_set_all)(const char **all_params, int param_count, void *filter_handle);
>>
>> /* Returns a parameter description string as described above. */
>> char *(*parameter_get_description)(void *filter_handle);
>>
>> /* Handler for additional messages. */
>> int (*filter_message_handler)(const char *message, char *message_parameters, char **response, void *filter_handle);
> Do you have some ideas for how this would get used?
This would be used to get some information from the filter or to change
things that cannot be changed via parameters. I can't really say what it
would be used for, but it comes at no additional cost, so I thought I should
include it. At least it is nice for testing purposes.
>
>> } filter_plugin;
> It might be a good idea to add an API version field to the struct, and
> maybe another field for indicating the minimum API version that the
> plugin requires from the host (ideally all plugins would support all
> past API versions, though).
>
> The API version would be incremented when new fields are added to the
> struct or something else is changed in the spec.
The structure is provided by the filter, not by PA, so it could
only be a field that indicates which API version is required.
>
> I would leave all the rewind stuff out, and if someone really wants to
> have rewinding support in their plugin, that can be added in v2.
Meanwhile I have it implemented correctly which was really not
easy to achieve, so I am very reluctant to leave this part out.
It is already part of !88, so there is no additional cost to add it
here. The standard approach will be to disable rewinding anyway.
>
>> /* Datatype corresponding to the get_Filter_Info() function. */
> The get_Filter_Info() function is not explained anywhere.
>
>> typedef const struct filter_plugin *(*Filter_Info_Function)(unsigned long Index);
Sorry for the missing explanation. This function is similar to the
LADSPA_Descriptor_Function() and is the entry point into the plugin
file. The concept is the same as for LADSPA, that is in one file there
can be multiple filters and the get_Filter_Info() functions retrieves
the filter_plugin structures for the various filters based on an index.
If an invalid index is specified, the function returns NULL.
>
> In an earlier mail you wrote:
>
>> It is not fixed yet what parameter_get_description() will exactly return, so the list
>> above has to be viewed as an example.
> To get closer a final decision, I have this comment: I already
> complained that min/max may not always make sense; I suggest solving
> this by wrapping the min/max information in a structure whose contents
> are type specific. That allows us to extend the filter metadata
> structure with non-type-specific stuff later if necessary, and new
> type-specific stuff can be added too without breaking old code.
>
Yes, something like {{name}{type}{default value}{Array of type specific
information}}
sounds like a good approach.
For reference I attached my first plugin, so that you can see what
a filter implementation would look like.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: amplifier.c
Type: text/x-csrc
Size: 12867 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20190513/d2205160/attachment-0001.c>
More information about the pulseaudio-discuss
mailing list