On 30.04.19 19:23, Alexander E. Patrakov wrote:
вт, 30 апр. 2019 г. в 16:31, Georg Chini <georg@xxxxxxxx>: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. _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
