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

[Georg Chini]

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 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?
> Whatever extensions we need to support the plugins that we want to
> support. I'm not familiar with the LV2 plugin ecosystem, and which
> extensions are widely used and relevant to PulseAudio (the GUI
> extensions may be widely used, but can't be supported).
>
>>> 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.
> Ok, if we assume an extension for interleaved audio ports, then I
> believe the Option extension would be suitable for configuring the
> number of channels.
That was what I had been thinking of. Or of creating an extension
that provides functions that set and get the number of channels
and are only available if the host supports the extension. A host
not supporting the extension would simply see  "normal" mono
channels.
>
>>>>>>>>> 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.
>>>>>
>>>> Control ports are used for both - initial configuration and changes
>>>> during run time. And yes, the intention is to change parameters
>>>> while the filter is running. As explained in the follow-up mail to
>>>> this one, changes at run-time must be done from the IO thread
>>>> while the initial configuration can be done from the main thread.
>>>> If a parameter changes during run-time, the filter must be reset.
>>>> So you have to distinguish between the two cases of preparing
>>>> the filter for running the first time and changing parameters
>>>> while it is running.
>>> 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.

>
>>> 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.

So it looks like we have to live with the filters we already provide
and further development seems pointless.