[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 :) ).

(Continuing review. I noticed that I complained yesterday about several
things that Alexander had already mentioned. Sorry, Alexander, for
ignoring your message.)

In sink_input_process_rewind_cb():

    if (u->s1histbuf->idx > rewind_samples) {
        //we're cool, no wrap required
        u->s1histbuf->idx -= rewind_samples;
    }

The comparison operator should be ">=" instead of ">".

Only the stage 1 history buffer idx is moved backwards, the stage 2
history buffer idx is kept unchanged.

    // add the 2 frame backlog for the biquad
    rewind_frames += 2;

The increment should be 3 instead of 2. The last frame in the history
contains the most recent (x0, y0) pairs, and the history idx points to
the place where the next frame will be start. Let's imagine that we want
to rewind 0 frames and restore the filter state from the history buffer
(a silly thing to do, but this is just an illustrative example). We need
to restore the last three frames from the history buffer, i.e. read
buffer[idx - 3 * channels + i], buffer[idx - 2 * channels + i] and
buffer[idx - 1 * channels + i] for each channel i. Buffer[idx] is
invalid data, we don't want to access that. So, as you can see, with
rewind size 0 we need to go back 3 frames. With rewind size 1 we would
go back 4 frames etc.

The history buffer indexing in sink_input_process_rewind_cb() looks
wrong:

    frame_size = u->sample_spec.channels * sizeof(biquad_data_element);
    for (i = 0; i < u->sample_spec.channels; i++) {
        switch (u->filter_map[i]) {
            case 'l': {
                // stage 1
                u->s1lpdt[i].w2 = u->s1histbuf->buffer[i].w0;
                u->s1lpdt[i].w1 = u->s1histbuf->buffer[i + frame_size].w0;
                u->s1lpdt[i].w0 = u->s1histbuf->buffer[i + 2 * frame_size].w0;

u->s1histbuf->buffer is an array of biquad_data_elements, not a byte
array, so you don't need to care about the frame size in bytes. Instead
of frame_size, you should use the number of channels as the multiplier.
Also, s1histbuf->idx is not taken into account, so the data is always
read from the beginning of the history buffer. So, the last line, for
example, should look like this:

    u->s1lpdt[i].w0 = u->s1histbuf->buffer[history_idx + 2 * channels + i].w0;

...and that's still broken, because it doesn't take into account that
history_idx may point to the last or second last frame of the buffer,
causing invalid memory to be accessed due to the addition of "2 *
channels".

    // move the buffer forward 2 frames so we can write right
    u->s1histbuf->idx += frame_size;
    u->s2histbuf->idx += frame_size;

The increment amount should be "3 * channels" instead of "frame_size".

I said this already in an earlier mail: the "(5) PUT YOUR CODE HERE TO
REWIND YOUR FILTER" comment seems to be in the wrong place, because the
"amount" variable in process_rewind_cb() is the amount that
u->memblockq's write index is moved backwards, but the filter actually
operates where u->memblockq's read index points to. process_rewind_cb()
may apparently in some situations move the read index more than the
write index, so you should not use "amount" as the amount to rewind the
biquad history buffer, because that may be too little. Instead, use
"rewind_bytes", because that's the amount that u->memblockq's read index
is moved, and move the biquad rewinding code outside the "if
(u->sink->thread_info.rewind_nbytes > 0)" block.

The references to bug 53709 aren't correct, because your module doesn't
contain any buffering that would increase latency. The whole bug might
be invalid, because I created it based on incorrect assumptions, but it
might apply to some modules.

The "if (max_rewind == u->sink->thread_info.max_rewind)" check in
update_max_rewind_cb() is not necessary. It's not an unexpected
situation if the function is called even if there ends up being no
difference to the old value. It can happen when creating or moving the
sink input.

The "if ( (max_rewind / u->sz_frm) < MIN_MAX_REWIND_FRAMES)" check is
unnecessary too. There's a comment saying that the check exists to avoid
excessive reallocing, but that can be avoided simply by reallocing only
when the history buffer is too small. There's no good reason to ever
shrink the history buffer. Sure, it can save some memory in some
situations, but I don't think it's worth the effort.

The field names in biquad_data_element could be just x and y instead of
x0 and y0, since there's only only one of each, and they don't have any
particular connection to biquad_data's x0 and y0 (the history data
applies equally much to x1, x2, y1 and x2 too).

    growth = (max_rewind / u->sz_smp) - u->s1histbuf->length;

should be

    growth = (max_rewind / u->sz_smp) + 3 * channels - u->s1histbuf->length;

because max_rewind doesn't contain the 3 extra frames that you need to
keep in the history.

The history buffers are completely wiped in update_max_rewind_cb(). The
history data must not be lost when resizing the buffer. Only the new
empty space in the buffer should be zeroed. Note that before zeroing
anything, you need to move the data after idx to the end of the new
buffer, and then zero the area between idx and the beginning of the
moved part.

You could add a comment above each function in module-lfe-lp.c telling
from which thread they are called.

Hmm, actually, contrary to what I have written in an earlier mail, I
think you don't need to (and shouldn't, at least using the current code,
because it has some bugs) allocate the history buffers in
sink_input_attach_cb(). It seems that sink_input_update_max_rewind_cb()
will always be called after the attach() callback (see
PA_SINK_MESSAGE_ADD_INPUT and PA_SINK_MESSAGE_FINISH_MOVE handling in
pa_sink_process_msg() in sink.c), so it's sufficient to do the buffer
allocation in sink_input_update_max_rewind_cb().

The long comment in pa__init() about the history buffer initialization
should be cleaned up, even though it's nice to be quoted :) It contains
some incorrect information (see the previous paragraph).

So update_max_rewind_cb() will always be called after attach_cb(). The
same is true for update_max_request_cb(). This means that the
pa_sink_set_max_request_within_thread() and
pa_sink_set_max_rewind_within_thread() calls in attach_cb() are
redundant, because they will be called anyway again from the
corresponding update callbacks. Those redundant calls could be removed
from module-virtual-sink.c too (added to my todo list).

sink_input_may_move_to_cb() is entirely redundant, because the loop
check that it does is nowadays implemented in the core code. (I have a
todo item for removing similarly redundant may_move_to() implementations
from other modules too.)

You use pa_proplist_gets(u->sink->proplist, "device.vsink.name") in
sink_input_moving_cb(), but you don't set that property anywhere.
module-virtual-sink seems to set "device.vsink.name" to the name of the
virtual sink, so you could do the same, but I don't really see the
point. You can just use u->sink->name directly.

In pa__init():

    u->lpfreq = atof(pa_modargs_get_value(ma, "lpfreq", "100.0"));

atof() doesn't provide proper error handling. Use
pa_modargs_get_value_double() instead.

The pa_modargs_get_sample_spec_and_channel_map() call is redundant,
because your module doesn't accept any arguments that could modify the
sample spec or channel map. Perhaps it should accept some, though?

The pa_memblockq_new() call uses tabs to align the comments, but at
least in my browser code the comments aren't result is not a neatly
aligned in the github code view. Please never use use tabs for anything
(except makefiles) when working on PulseAudio.

Use pa_xnew0() instead of malloc() and calloc() and pa_xrealloc()
instead of realloc(). And pa_xfree() instead of free().

The "fail" label in pa__init() should be on its own line and should have
no indentation.

MIN_CUTOFF_FREQ and MAX_CUTOFF_FREQ seem to be used only for sanity
checking the module arguments, and the biquad filters could also work
with values outside that range, so I think it's better to move those
constants inside module-lfe-lp.c. If the constants were kept in
biquad-filter.h, they should be prefixed with "PA_BIQUAD_".

MEMBLOCKQ_MAXLENGTH doesn't belong in biquad-filter.h either, nor the PI
constant. I don't think the PI constant is needed at all, just use M_PI
everywhere. you have #ifndef M_PI, is there some platform that doesn't
have M_PI defined in math.h?

More about the copyright notice: if you are going to choose GPLv3 as the
license, the copyright notice at the top of the files shouldn't make
statements about PulseAudio, because PulseAudio is covered by a
different license in general. You should limit the notice so that it
only talks about your own code. Also, the doxygen tags at the top of the
files are redundant and IMHO ugly.

I said yesterday that the "extern" specifiers are useless in
biquad-filter.h. For completeness, I'll point out here that they are
equally useless in biquad-filter.c.

filter_biquad() should drop the "struct" specifier from the bqdt and
bqfs arguments.

In my opinion it would be more logical to do the x1, x2, y1 and y2
assignments before the x0 and y0 assignments in filter_biquad(). That
way x2, x1 and x0 would always mean the last three input samples. With
the current code, x0 and x1 both refer to the last input sample and x2
refers to the second last sample (as pointed out by Alexander too). This
breaks loading state from the history, at least with the changes that I
suggested (going back 2 instead of 3 extra frames when rewinding might
actually successfully compensate for this "oddity" in the current
filter_biquad() implementation).

    Q = sqrt(1.0) / 2.0;

This formula is not from the EQ cookbook, so please add a comment where
it comes from. Also, without any comments, this looks like an overly
complicated way to say "Q = 0.5" :)

...And that's all. This became quite a long mail... the good news is, if
you fix everything that has been pointed out (also by Alexander), there
shouldn't be much left to do before the code can be accepted upstream.

-- 
Tanu