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

[Tanu Kaskinen]

On Wed, 2013-04-03 at 16:29 -0700, Justin Chudgar wrote:
> Gentlemen: 
> 
> I believe that I have gotten rewind working, and I have moved the biquad 
> filtering specific code to its own location in order to be able to share it with 
> the forthcoming "module-xover-sink" that is intended to correctly implement 
> remixing with a Linkwitz-Riley 4th order crossover. I have started to cleanup 
> the code to meet the style guide, as well. 
> 
> Would you kindly take a look at the code in:
> 
> https://github.com/justinzane/pulseaudio/tree/master/src/modules
> 
> - module-lfe-lp.c
> - biquad/*
> - Makefile.am
> 
> and let me know what more I need to do to get finished with module-lfe-lp and 
> move onto module-xover-sink (oh, and heftig, feel free to name it whatever you 
> think is proper, you idea = your naming rights :) ).

Are you sure you want to license your code under GPLv3 instead of
LGPLv2.1? If so, your code needs to be conditionally compiled, and I'd
say even disabled by default. Instead of adding an --enable-lfe-lp
option or similar, though, I think we should have an --enable-gpl3
option which would enable this module, and any other GPLv3 code if we
ever get more of such code (I guess that would happen pretty soon, if
you plan to work on the crossover module too).

I'd prefer the file locations be set up in one of the following ways:

1) Move module-lfe-lp.c, module-xover-sink.c and biquad-filter.[ch] all
in the same directory (e.g. src/modules/remixing).

2) Move biquad-filter.[ch] to src/pulsecore and build them as part of
libpulsecore.

Since the biquad filter implementation is potentially useful also for
other things than remixing, I think I prefer option 2, but either is
fine for me.

I'd like to have a "-sink" suffix in the module name, because that's the
convention for modules that create a sink.

The _("foo") syntax makes the string translatable. I don't think you
need to make your name translatable in PA_MODULE_AUTHOR. The same goes
for the bogus PACKAGE_VERSION.

The "force_flat_volume" option should probably not be there. It's in
module-virtual-sink, because I once had use for such an option when
testing the core volume handling. I should add comments to
module-virtual-sink saying that derived modules don't need that
option...

The names defined in biquad-filter.h should have "pa_" prefix. The
functions should use the same prefix as the structs (i.e. "pa_biquad_",
you can drop "filter" from the function names).

The "extern" specifiers in biquad-filter.h are unnecessary.

You seem to refer to the input samples with w0, w1 and w2. Why not x0,
x1 and x2 as in the EQ cookbook?

Pointer dereferencing style: use bqdt->y0 instead of (*bqdt).y0. (Also,
to me "data" would seem more intuitive than "bqdt", and similarly
"factors" vs. "bqfs".)

Now that the biquad stuff is in separate files, the userdata struct
should not use "struct" for the biquad fields.

We don't usually put parentheses around the value in return statements.
Is there some rationale for doing that?

The final version shouldn't have the "JZ:" prefix in log messages. Also,
adding the file and line number to the log messages is redundant,
because those are printed anyway if you run pulseaudio with the
--log-meta option.

I'm not sure what to think about the __attribute__((optimize(3))) usage.
Have you done some benchmarking that shows that the speedup is
significant compared to the normal -O2? If yes, I guess we can keep
them.

Instead of using characters 'l', 'h' or 'a' to select the filter type,
there should be an enum that defines constants for these.

In sink_input_pop_cb() (and in similar cases elsewhere), it would be
better to use a switch statement for handling the different cases of the
filter type instead of a series of "else ifs".

     lp = filter_biquad(& (u->s1lpdt[chan_idx]), * (u->lpfs), cur_sample);

Please don't put a space after the unary & and * operators (I wouldn't
use the parentheses either, but they don't bother me so much). Also,
similar to what I said earlier, I'd prefer "data" instead of "dt" and
"factors" instead of "fs".

Finally, it would be great if you could wrap all the biquad state in a
filter object so that modules wouldn't have to manage all that by
themselves. The object would have a simple process() function that takes
just an input and an output buffer and the requested number of frames to
process. Similarly, there would be a simple rewind() function for
resetting the filter to an earlier state.

I got to the end of sink_input_pop_cb() in my review, I'll continue
tomorrow.

-- 
Tanu