[pulseaudio-discuss] [PATCH 2/6] lfe-filter: Enable LFE filter in the resampler

[Alexander E. Patrakov]

10.02.2015 02:27, David Henningsson wrote:
>> Filters can overshoot. Please clamp to the range of short.
>
> Is this really true here - as Butterworth and LR4 filters never boost
> any frequency, only attenuate, can this filter still overshoot?

Well, I think yes. See here for an example how an ideal brickwall filter 
(that also only removes frequencies) overshoots (by about 9%) when 
applied to a square wave:

https://en.wikipedia.org/wiki/Gibbs_phenomenon

If you want to test the worst case for sure, please save the impulse 
response of the whole filter (obviously, you will have to cut it off at 
some point in time), and replace all positive samples with +32760, and 
all negative ones with -32760. Then time reverse the result, add some 
zeros at the end just in case, and run the result through the filter. I 
can do it for you after my official work (i.e. later today).

>
>>
>>> +    }
>>> +
>>> +    lr4->x1 = lx1;
>>> +    lr4->x2 = lx2;
>>> +    lr4->y1 = ly1;
>>> +    lr4->y2 = ly2;
>>> +    lr4->z1 = lz1;
>>> +    lr4->z2 = lz2;
>>> +}
>>> +
>>> +
>>>   /* Split input data using two LR4 filters, put the result into the
>>> input array
>>>    * and another array.
>>>    *
>>> diff --git a/src/pulsecore/filter/crossover.h
>>> b/src/pulsecore/filter/crossover.h
>>> index 99a601c..aa1140f 100644
>>> --- a/src/pulsecore/filter/crossover.h
>>> +++ b/src/pulsecore/filter/crossover.h
>>> @@ -25,6 +25,12 @@ struct lr4 {
>>>       float z1, z2;
>>>   };
>>>
>>> +void lr4_set(struct lr4 *lr4, enum biquad_type type, float freq);
>>> +
>>> +void lr4_process(struct lr4 *lr4, int samples, int channels, float
>>> *data0);
>>> +void lr4_process_s16(struct lr4 *lr4, int samples, int channels,
>>> short *data0);
>>> +
>>> +
>>>   /* Three bands crossover filter:
>>>    *
>>>    * INPUT --+-- lp0 --+-- lp1 --+---> LOW (0)
>>> diff --git a/src/pulsecore/filter/lfe-filter.c
>>> b/src/pulsecore/filter/lfe-filter.c
>>> new file mode 100644
>>> index 0000000..1597eca
>>> --- /dev/null
>>> +++ b/src/pulsecore/filter/lfe-filter.c
>>> @@ -0,0 +1,97 @@
>>> +/***
>>> +  This file is part of PulseAudio.
>>> +
>>> +  Copyright 2014 David Henningsson, Canonical Ltd.
>>> +
>>> +  PulseAudio is free software; you can redistribute it and/or modify
>>> +  it under the terms of the GNU Lesser General Public License as
>>> published
>>> +  by the Free Software Foundation; either version 2.1 of the License,
>>> +  or (at your option) any later version.
>>> +
>>> +  PulseAudio is distributed in the hope that it will be useful, but
>>> +  WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> +  General Public License for more details.
>>> +
>>> +  You should have received a copy of the GNU Lesser General Public
>>> License
>>> +  along with PulseAudio; if not, write to the Free Software
>>> +  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
>>> +  USA.
>>> +***/
>>> +
>>> +#ifdef HAVE_CONFIG_H
>>> +#include <config.h>
>>> +#endif
>>> +
>>> +#include "lfe-filter.h"
>>> +#include <pulse/xmalloc.h>
>>> +#include <pulsecore/filter/biquad.h>
>>> +#include <pulsecore/filter/crossover.h>
>>> +
>>> +// An LR4 filter, implemented as a chain of two LR2 filters.
>>
>> As a chain of two Butterworth filters. LR2 is not Butterworth.
>> Butterworth filter has Q == 0.707, LR2 has Q == 0.5.
>
> Ack
>
>>
>>> +
>>> +struct pa_lfe_filter {
>>> +    float crossover;
>>> +    pa_channel_map cm;
>>> +    pa_sample_spec ss;
>>> +    bool active;
>>> +    struct lr4 lr4[PA_CHANNELS_MAX];
>>
>> The allocation (and the fact that there is one filter per output
>> channel) is non-obvious here and should be commented better. For each
>> non-LFE channel, a trivial implementation would have one struct lr4 for
>> highpass filter and one struct lr4 for lowpass filter. Here you reduced
>> the number of the needed filters by pre-averaging non-LFE input channels
>> and running them through a single filter.
>
> The existing remap algorithm - or policy, if you prefer - is to average
> all channels and put it on the LFE channel.
>
> It seems to me that if you wanted another remap algorithm that should be
> a comment in setup_remap(), not here. I wouldn't say that this filter
> "expects" anything specific, it just removes frequencies according to
> the crossover, and it's the job of setup_remap() to set things up
> correctly.
>
> I could even add a channel bitmask to make it more generic and allow
> other channels to be high- or lowpass than the current fixed "LFE is
> lowpass everything else is highpass", but I thought it was unnecessary
> complexity at this point.

Not only it is unnecessary complexity, but it also doesn't cover the 
second existing use case. Suppose a 4.0 system where front speakers can 
reproduce low frequencies, and rear speakers cannot. In this case,

LF from front channels should be sent to front speakers
HF from front channels should be sent to front speakers
LF from rear channels should be sent to front speakers
HF from rear channels should be sent to rear speakers

Here the "average, then apply the filter" strategy doesn't work. That's 
why I expected, in general, two separate filters per channel. Maybe that 
remark will help you place the comment better :)

>
>>> +};
>>> +
>>> +pa_lfe_filter_t * pa_lfe_filter_new(const pa_sample_spec* ss, const
>>> pa_channel_map* cm, float crossover_freq) {
>>> +
>>> +    pa_lfe_filter_t *f = pa_xnew0(struct pa_lfe_filter, 1);
>>> +    f->crossover = crossover_freq;
>>> +    f->cm = *cm;
>>> +    f->ss = *ss;
>>> +    pa_lfe_filter_update_rate(f, ss->rate);
>>> +    return f;
>>> +}
>>> +
>>> +void pa_lfe_filter_free(pa_lfe_filter_t *f) {
>>> +    pa_xfree(f);
>>> +}
>>> +
>>> +void pa_lfe_filter_reset(pa_lfe_filter_t *f) {
>>> +    pa_lfe_filter_update_rate(f, f->ss.rate);
>>> +}
>>> +
>>> +pa_memchunk * pa_lfe_filter_process(pa_lfe_filter_t *f, pa_memchunk
>>> *buf) {
>>
>> Maybe a comment is needed that this function expects an average of all
>> non-LFE channels in the would-be-LFE channel? Not sure how this would
>> mix with the alternative LFE channel generation policies.
>
> See above.
>

-- 
Alexander E. Patrakov