[pulseaudio-discuss] Updated module-lfe-lp, requesting review/recommendations

[Alexander E. Patrakov]

2013/4/4 Justin Chudgar <justin at justinzane.com>:
> https://github.com/justinzane/pulseaudio/tree/master/src/modules
>
> - module-lfe-lp.c
> - biquad/*
> - Makefile.am

Thanks for separating the biquad filter out. This makes it easy to
test it with various test signals without building pulseaudio.

This e-mail contains only things found by looking at your code. When I
test it, there will be another e-mail.

1. biquad/biquad-filter.h contains a lot of unneeded includes at the
top. Please clean them up.

2. In the comment above filter_calc_factors, the "stage" parameter is
incorrectly referred to as "start". However, actually it is unused,
and please be assured that the Q factor is the same for both stages of
the LR4 crossover and for your use case. The use case when it is not
the same is, say, when you are trying to build a 4th order Butterworth
filter from two biquads - but this is not our use case, we both need a
cascade of two 2nd order Butterworth filters, not a 4th order
Butterworth filter.

3. Above filter_biquad(), I'd add what these xs and ys mean. Yes, you
do follow the established standard naming convention when dealing with
IIR filters, but not all pulseaudio hackers are DSP specialists. I.e.
please add something like:

"Here x0, x1 and x2 are the most recent input sample, the previous one
and the second-previous one. y0, y1 and y2 are the output samples
corresponding to x0, x1 and x2."

4. Your x in the comments and w in the code seem to be the same thing.
If this is so, rename w to x in the code or x to w in the comment.

5. I still have to test the implementation of filter_biquad(). My
previous comment about the fact that at the end of the function (and
thus in the history buffer) y1 == y0 is still valid. If the
implementation is still correct (and just after the y0 = ... line it
seems that the variables do have their intended meaning), please don't
keep the redundant copies. If it is incorrect (and I will test this
later), well, fix it.

6. Not really a criticism, but - why did you make filter_biquad and
filter_store_history separate functions?

7. (*bqfs).a0 is more commonly written as bqfs->a0

8. The comment about s2lpdt incorrectly mentions stage 1.

9. I'd rather not pass structures such as struct biquad_factors by
value into functions such as filter_biquad. This causes
whole-structure copying and wastes time.

10 (probably a duplicate of (6)). In general, I think that any direct
reference to w0 or y0 in module-lfe-lp.c is a sign of bad API design
in biquad.h. Please refactor, so that you don't have to write this:

                    hp = filter_biquad(& (u->s1hpdt[chan_idx]), *
(u->hpfs), cur_sample);
                    bqdtel.w0 = u->s1hpdt[chan_idx].w0;  /* <--- this */
                    bqdtel.y0 = u->s1hpdt[chan_idx].y0;  /* <--- and this */
                    filter_store_history(u->s1histbuf, &bqdtel);

In fact I think that you don't need u->s1hpdt and its friends at all,
you probably can just store all the w and y data in the history buffer
directly.

11. About the "add the 2 frame backlog for the biquad" comment - not
sure. Please manually inspect your code for the case when pulseaudio
asks your filter to rewind one sample and then writes the same sample
as it did.

12. I don't understand why filter sinks such as yours need the
"use_volume_sharing" and "force_flat_volume" parameters.

-- 
Alexander E. Patrakov