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
