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

[Alexander E. Patrakov]

вт, 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?

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

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

>
>      /* 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?

-- 
Alexander E. Patrakov