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

[Georg Chini]

On 30.04.19 19:23, Alexander E. Patrakov wrote:
> вт, 30 апр. 2019 г. в 16:31, Georg Chini <georg at chini.tk>:
>>       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 parameter set.