[pulseaudio-discuss] [PATCH] Remove module-equalizer-sink

[Alexander E. Patrakov]

17.03.2014 18:18, Jason Newton wrote:
> Hi Again Alexander,
> Further, I didn't appreciate that you
> didn't CC me on the initial email, despite my contact details being
> present in the sourcecode

I did CC you (you can check that by downloading official mbox archives), 
and there was no bounce. Maybe it ended up in the SPAM folder?

Replying only to the points where I have something to reply.

> For using the windowing, the main reason I brought it in was because
> from what I read, it is required for time-variant filters.  Now I know
> the filters are quasi-invariant but when someone is fiddling with the UI
> the filter is time-variant.  Should I ignore that?

Well, that argument is valid, and that also fully explains why you used 
overlap-add (overlap-save doesn't have the intrinsic crossfading that 
creates a gradual transition between the old and the new equalizer 
settings). We should not ignore that, but the automatic crossfade 
inherent in OLA with a non-rectangular window is not the only way to 
deal with it.

What is also valid is that the equalizer settings don't change all the 
time (so it is somewhat wasteful to always pay the price for the 
time-varying filters), and I would expect any change to trigger a rewind 
anyway in order to apply immediately. So a perfect solution, IMHO, would 
be to do a short crossfade manually upon any rewind.

> As far as I know it is a problem if you're saying to still sample at the
> next power of 2 for say 44.1khz but allow the latency to go smaller.
> The length of the window choice may not even change the length of the
> next power of 2 FFT but more of these per second as a result of less
> latency will be felt as it's the FFTs at high enough samplerates that
> are - I wanted to avoid hitting 10% CPU usage on the i7 920 I mostly
> tested on and at 4k or so of latency (samples) and 96khz audio, it was
> definitely up there.  But if its a tunable parameter (in ms, a
> reasonable default for most can be found and individuals can then choose
> what amount they think is allowable; it's also been almost 5 years.  Or
> were you talking about also using smaller FFTs and losing resolution -
> if so, even if that's common practice... I don't like that and am
> unfamiliar with it but if you have some literature to point to, I will
> take a look.

You have almost correctly reworded what I wanted to say. Yes, window 
size controls one part of the latency, half of the impulse response size 
adds to that latency (because a linear-phase filter impulse response 
often has a maximum around its center), and FFT size indirectly controls 
the available frequency resolution. And yes, lowering the window size 
without lowering the FFT size means increased CPU usage.

It is important to avoid confusing the FFTs that would appear in the 
proper implementation. One IFFT is used to convert the desired frequency 
response to the time domain before shifting/truncation/windowing. This 
IFFT indeed controls the available frequency resolution, and 
truncation/windowing of the impulse response limits it further. The 
other FFT/IFFT pair is just a technical detail of how a convolution is 
calculated in OLA or OLS. Its size should be just enough to contain the 
windowed signal and the properly-windowed filter impulse response (with 
windows being different!), and so it depends on the desired frequency 
resolution only through the length of the impulse response.

As for the decreased frequency resolution, the rule is as follows. FFT 
of size N gives you N/2 + 1 independent points evenly distributed in 
frequency domain, the first one being 0 Hz and the last one being one 
half of the sampling rate. So, with FFT of size 8 (to keep the example 
small), and 48 kHz sampling rate, you will get the following points: 0 
Hz, 6 kHz, 12 kHz, 18 kHz, 24 kHz.

> Re output_q: the filter output has to be chunked into segments which
> have size constraints to continue through the PA pipeline.  It is
> dequeued in accordance with the sink_input_pop_cb which only takes one
> memchunk at a time.  I once tried to do this all in one big chunk
> originally but pa core complained about that due to the memchunk size
> limitations.

Looks like a consequence of too-big window size.

Also I think that the do{ ... } while (u->samples_gathered < 
target_samples) loop in the pop callback is unneeded. Upper layers deal 
with less-than-needed number of provided samples just fine and will 
reask for them.

      Do you think it should be discarded from the
> calculation still?  Also, input_q and algorithmic latency seems a bit
> muddled.  Perhaps the right thing to do is to round up how much input_q
> is by algorithmic latency.

The algorithmic latency should be added on top, and this does not 
correspond to any rounding.

> I think the real problem here is DRY over the modules that are based or
> are similar to module-virtual-sink.  Many of those callbacks aren't
> really 'overridden' in a given implementation.

Spot on. But, if you factor out the common details (as you suggest 
below), all that's left for the plugin author to implement is 
essentially a LADSPA-like API plus rewinds and the means to report 
latency. However, nobody except the ALSA sink implements rewinds 
correctly anyway - so I sometimes think that implementing them at all 
may in some cases be not worth the trouble if the overall latency is 
limited.

> The cost of
> implementation and maintenance of one of these sinks shouldn't be that
> excessive as most of the changes would then be made in fewer places
> given a better way of inheriting from that common base.

> I think ladspa
> also forces a weird external (plugin) dependency that could be
> troublesome for some people and it also sounds linux specific where as
> pulse aspires to be cross platform.  Further/in general I feel better
> integration between algorithm and PA is possible only without the use of
> a prepackaged library set like ladspa's.

LADSPA is not a library, it is only a header file needed at the plugin 
build time, and it exists under win32, too.

> And the DSP code shouldn't not
> be that hard to write - even if my implementation was/is incorrect, the
> core region of it is about 50 lines.  Would you bring in  blas to add 2
> vectors if that was all you were going to do?

Comparison with BLAS is unwarranted here. BLAS is a library. LADSPA is 
just a header defining the interface for plugins.

As for OLA vs OLS - I think that, after you mentioned a time-varying 
filter, it is no longer a clear-cut question.

-- 
Alexander E. Patrakov