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

[Alexander E. Patrakov]

2013/4/5 Tanu Kaskinen <tanuk at iki.fi>:
> (Continuing review. I noticed that I complained yesterday about several
> things that Alexander had already mentioned. Sorry, Alexander, for
> ignoring your message.)

Duplication of valid suggestions is not a problem. Missing the
suggestion is an issue, and, unfortunately, due to the number of the
suggested fixes, you did just that (see below about the Q value).

>     // add the 2 frame backlog for the biquad
>     rewind_frames += 2;
>
> The increment should be 3 instead of 2.

Given the duplication of y1 and y0 and your assumption that we need to
keep three previous samples, I'd question that. But I am sleepy now
and I can be wrong. In fact, to avoid any mistakes in the further
review, I'd suggest removing the duplication first (i.e. storing only
two samples - this is definitely possible because the filter already
produces the expected output) and only then fixing the rewind isues.

> 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.

See above.


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

I'd even suggest getting rid of the separation of the two different
ways to keep the history. y1 is the previous output sample of the
biquad filter and thus it is already available in the big history
buffer.

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

Again, if one removes duplication, there are probably only two extra frames.

> 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.

However, only two of them are useful, even after your suggested fix:
x2 would be simply "assigned but never used". Well, there is also my
comment about the two history buffers (2- or 3-sample buffer and big
buffer) that may be unnecessary.

>     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 it is actually wrong, the correct value for a Butterworth filter,
as I already pointed out in
http://lists.freedesktop.org/archives/pulseaudio-discuss/2013-April/016840.html,
is sqrt(2.0)/2, i.e. M_SQRT_1_2 (that's what I was referring to at the
top of this e-mail). See e.g.
http://opencourseware.kfupm.edu.sa/colleges/ces/ee/ee303/files%5C5-Projects_Sample_Project3.pdf

> ...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.

Actually, there is still an important task for me and Tanu:
communicate (on IRC or via e-mail). The goal is to make sure that Tanu
understands all the math in this module, can meaningfully verify the
operation of this module and can, in theory, rewrite it from scratch
correctly (just for the case if all modules would have to be
refactored due to some sweeping global change in the future). In other
words, to make sure there is no magic in the code and there is a core
developer who can explain it to a rubber duck.

--
Alexander E. Patrakov