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

[Tanu Kaskinen]

On Sat, 2013-04-06 at 00:01 +0600, Alexander E. Patrakov wrote:
> 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.

It's true that biquad_data doesn't need to store 3 samples, 2 is enough.
If the redundant fields from biquad_data are removed, then 2 "extra"
frames is sufficient for the history buffer too.

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

That could be done, yes.

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

Refactoring shouldn't cause any need to rewrite the math from scratch.
It's desirable to be able to verify that the code implements the math
that it's supposed to, though. To achieve that, I think it's enough to
link to the EQ cookbook, like the code already does, and mention
(preferably with a reference) that second-order Butterworth filters
always have Q=1/sqrt(2).

-- 
Tanu