On Sat, 2019-05-04 at 22:41 +0200, Georg Chini wrote: > /*** > This file is part of PulseAudio. > > PulseAudio is free software; you can redistribute it and/or modify > it under the terms of the GNU Lesser General Public License as published > by the Free Software Foundation; either version 2.1 of the License, > or (at your option) any later version. > > PulseAudio is distributed in the hope that it will be useful, but > WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > General Public License for more details. > > You should have received a copy of the GNU Lesser General Public License > along with PulseAudio; if not, see <http://www.gnu.org/licenses/>;. > ***/ > > /* Channel positions copied from pulseaudio channelmap.h */ > enum channel_position { > CHANNEL_POSITION_INVALID = -1, > CHANNEL_POSITION_MONO = 0, > > CHANNEL_POSITION_FRONT_LEFT, /**< Apple, Dolby call this 'Left' */ > CHANNEL_POSITION_FRONT_RIGHT, /**< Apple, Dolby call this 'Right' */ > CHANNEL_POSITION_FRONT_CENTER, /**< Apple, Dolby call this 'Center' */ > > CHANNEL_POSITION_LEFT = CHANNEL_POSITION_FRONT_LEFT, > CHANNEL_POSITION_RIGHT = CHANNEL_POSITION_FRONT_RIGHT, > CHANNEL_POSITION_CENTER = CHANNEL_POSITION_FRONT_CENTER, > > CHANNEL_POSITION_REAR_CENTER, /**< Microsoft calls this 'Back Center', Apple calls this 'Center Surround', Dolby calls this 'Surround Rear Center' */ > CHANNEL_POSITION_REAR_LEFT, /**< Microsoft calls this 'Back Left', Apple calls this 'Left Surround' (!), Dolby calls this 'Surround Rear Left' */ > CHANNEL_POSITION_REAR_RIGHT, /**< Microsoft calls this 'Back Right', Apple calls this 'Right Surround' (!), Dolby calls this 'Surround Rear Right' */ > > CHANNEL_POSITION_LFE, /**< Microsoft calls this 'Low Frequency', Apple calls this 'LFEScreen' */ > CHANNEL_POSITION_SUBWOOFER = CHANNEL_POSITION_LFE, > > CHANNEL_POSITION_FRONT_LEFT_OF_CENTER, /**< Apple, Dolby call this 'Left Center' */ > CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER, /**< Apple, Dolby call this 'Right Center */ > > CHANNEL_POSITION_SIDE_LEFT, /**< Apple calls this 'Left Surround Direct', Dolby calls this 'Surround Left' (!) */ > CHANNEL_POSITION_SIDE_RIGHT, /**< Apple calls this 'Right Surround Direct', Dolby calls this 'Surround Right' (!) */ > > CHANNEL_POSITION_AUX0, > CHANNEL_POSITION_AUX1, > CHANNEL_POSITION_AUX2, > CHANNEL_POSITION_AUX3, > CHANNEL_POSITION_AUX4, > CHANNEL_POSITION_AUX5, > CHANNEL_POSITION_AUX6, > CHANNEL_POSITION_AUX7, > CHANNEL_POSITION_AUX8, > CHANNEL_POSITION_AUX9, > CHANNEL_POSITION_AUX10, > CHANNEL_POSITION_AUX11, > CHANNEL_POSITION_AUX12, > CHANNEL_POSITION_AUX13, > CHANNEL_POSITION_AUX14, > CHANNEL_POSITION_AUX15, > CHANNEL_POSITION_AUX16, > CHANNEL_POSITION_AUX17, > CHANNEL_POSITION_AUX18, > CHANNEL_POSITION_AUX19, > CHANNEL_POSITION_AUX20, > CHANNEL_POSITION_AUX21, > CHANNEL_POSITION_AUX22, > CHANNEL_POSITION_AUX23, > CHANNEL_POSITION_AUX24, > CHANNEL_POSITION_AUX25, > CHANNEL_POSITION_AUX26, > CHANNEL_POSITION_AUX27, > CHANNEL_POSITION_AUX28, > CHANNEL_POSITION_AUX29, > CHANNEL_POSITION_AUX30, > CHANNEL_POSITION_AUX31, > > CHANNEL_POSITION_TOP_CENTER, /**< Apple calls this 'Top Center Surround' */ > > CHANNEL_POSITION_TOP_FRONT_LEFT, /**< Apple calls this 'Vertical Height Left' */ > CHANNEL_POSITION_TOP_FRONT_RIGHT, /**< Apple calls this 'Vertical Height Right' */ > CHANNEL_POSITION_TOP_FRONT_CENTER, /**< Apple calls this 'Vertical Height Center' */ > > CHANNEL_POSITION_TOP_REAR_LEFT, /**< Microsoft and Apple call this 'Top Back Left' */ > CHANNEL_POSITION_TOP_REAR_RIGHT, /**< Microsoft and Apple call this 'Top Back Right' */ > CHANNEL_POSITION_TOP_REAR_CENTER, /**< Microsoft and Apple call this 'Top Back Center' */ > > CHANNEL_POSITION_MAX > }; > > /* Error codes copied from pulseaudio def.h */ > enum { > OK = 0, /**< No error */ > ERR_ACCESS, /**< Access failure */ > ERR_COMMAND, /**< Unknown command */ > ERR_INVALID, /**< Invalid argument */ > ERR_EXIST, /**< Entity exists */ > ERR_NOENTITY, /**< No such entity */ > ERR_CONNECTIONREFUSED, /**< Connection refused */ > ERR_PROTOCOL, /**< Protocol error */ > ERR_TIMEOUT, /**< Timeout */ > ERR_AUTHKEY, /**< No authentication key */ > ERR_INTERNAL, /**< Internal error */ > ERR_CONNECTIONTERMINATED, /**< Connection terminated */ > ERR_KILLED, /**< Entity killed */ > ERR_INVALIDSERVER, /**< Invalid server */ > ERR_MODINITFAILED, /**< Module initialization failed */ > ERR_BADSTATE, /**< Bad state */ > ERR_NODATA, /**< No data */ > ERR_VERSION, /**< Incompatible protocol version */ > ERR_TOOLARGE, /**< Data too large */ > ERR_NOTSUPPORTED, /**< Operation not supported \since 0.9.5 */ > ERR_UNKNOWN, /**< The error code was unknown to the client */ > ERR_NOEXTENSION, /**< Extension does not exist. \since 0.9.12 */ > ERR_OBSOLETE, /**< Obsolete functionality. \since 0.9.15 */ > ERR_NOTIMPLEMENTED, /**< Missing implementation. \since 0.9.15 */ > ERR_FORKED, /**< The caller forked without calling execve() and tried to reuse the context. \since 0.9.15 */ > ERR_IO, /**< An IO error happened. \since 0.9.16 */ > ERR_BUSY, /**< Device or resource busy. \since 0.9.17 */ > ERR_MAX /**< Not really an error but the first invalid error code */ > }; I don't see the need for copying the channel position and error enums. We can #include the relevant headers. > > /* 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? I think we should use the normal PA naming conventions, so this would be named pa_filter_plugin_kill_cb_t. > /* 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. > 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? 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". > 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. > 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? > 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. > 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.) > 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. > > /* 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? 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 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). 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. 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. The "module" parameter could be renamed to kill_cb_data to not expose unnecessary implementation details, and to make its purpose clearer. > /* 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. > > /* 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. 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 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". I'd also rename the get/set functions: get_(filter_)parameter set_(filter_)parameter get_all_(filter_)parameters set_all_(filter_)parameters > > /* 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? > } 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. 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. > > /* 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); 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. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
