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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2015-02-01 16:15, Alexander E. Patrakov wrote:
> 29.01.2015 03:14, David Henningsson wrote:
>> When enable-lfe-remixing is set, an LFE channel is present in the
>> resampler's destination channel map but not in the source channel map,
>> we insert a low-pass filter instead of just averaging the channels.
>> Other channels will get a high-pass filter.
>
> That's a policy decision. Essentially, your code assumes that the LFE
> channel, if present, is correct for the target speakers. Other possible
> policies would be to add it to center and then always regenerate it, or
> to add the regenerated content to the original LFE content.
>
> But, for the time being, let's proceed with this policy.
>
>>
>> In this patch, the crossover frequency is hardcoded to 120Hz (to be fixed
>> in later patches).
>>
>> Note that in current state the LFE filter is
>>   - not very optimised
>>   - not rewind friendly (rewinding can cause audible artifacts)
>>
>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>> ---
>>   src/Makefile.am                   |  3 ++
>>   src/pulsecore/filter/crossover.c  | 79 ++++++++++++++++++++++++++++++-
>>   src/pulsecore/filter/crossover.h  |  6 +++
>>   src/pulsecore/filter/lfe-filter.c | 97
>> +++++++++++++++++++++++++++++++++++++++
>>   src/pulsecore/filter/lfe-filter.h | 38 +++++++++++++++
>>   src/pulsecore/resampler.c         | 34 ++++++++++++--
>>   src/pulsecore/resampler.h         |  3 ++
>>   7 files changed, 255 insertions(+), 5 deletions(-)
>>   create mode 100644 src/pulsecore/filter/lfe-filter.c
>>   create mode 100644 src/pulsecore/filter/lfe-filter.h
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index e37a22a..d200271 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -911,6 +911,9 @@ lib_LTLIBRARIES += libpulsecore- at PA_MAJORMINOR@.la
>>
>>   # Pure core stuff
>>   libpulsecore_ at PA_MAJORMINOR@_la_SOURCES = \
>> +        pulsecore/filter/lfe-filter.c pulsecore/filter/lfe-filter.h \
>> +        pulsecore/filter/biquad.c pulsecore/filter/biquad.h \
>> +        pulsecore/filter/crossover.c pulsecore/filter/crossover.h \
>>           pulsecore/asyncmsgq.c pulsecore/asyncmsgq.h \
>>           pulsecore/asyncq.c pulsecore/asyncq.h \
>>           pulsecore/auth-cookie.c pulsecore/auth-cookie.h \
>> diff --git a/src/pulsecore/filter/crossover.c
>> b/src/pulsecore/filter/crossover.c
>> index 11a8c6e..c902099 100644
>> --- a/src/pulsecore/filter/crossover.c
>> +++ b/src/pulsecore/filter/crossover.c
>> @@ -3,10 +3,10 @@
>>    * found in the LICENSE file.
>>    */
>>
>> -#include "crossover.h"
>>   #include "biquad.h"
>> +#include "crossover.h"
>>
>> -static void lr4_set(struct lr4 *lr4, enum biquad_type type, float freq)
>> +void lr4_set(struct lr4 *lr4, enum biquad_type type, float freq)
>>   {
>>       struct biquad q;
>>       biquad_set(&q, type, freq, 0, 0);
>> @@ -23,6 +23,81 @@ static void lr4_set(struct lr4 *lr4, enum
>> biquad_type type, float freq)
>>       lr4->z2 = 0;
>>   }
>>
>> +void lr4_process(struct lr4 *lr4, int samples, int channels, float
>> *data0)
>> +{
>> +    float lx1 = lr4->x1;
>> +    float lx2 = lr4->x2;
>> +    float ly1 = lr4->y1;
>> +    float ly2 = lr4->y2;
>> +    float lz1 = lr4->z1;
>> +    float lz2 = lr4->z2;
>> +    float lb0 = lr4->b0;
>> +    float lb1 = lr4->b1;
>> +    float lb2 = lr4->b2;
>> +    float la1 = lr4->a1;
>> +    float la2 = lr4->a2;
>> +
>> +    int i;
>> +    for (i = 0; i < samples; i += channels) {
>> +        float x, y, z;
>> +        x = data0[i];
>> +        y = lb0*x + lb1*lx1 + lb2*lx2 - la1*ly1 - la2*ly2;
>> +        z = lb0*y + lb1*ly1 + lb2*ly2 - la1*lz1 - la2*lz2;
>> +        lx2 = lx1;
>> +        lx1 = x;
>> +        ly2 = ly1;
>> +        ly1 = y;
>> +        lz2 = lz1;
>> +        lz1 = z;
>> +        data0[i] = z;
>
> In-place modification. Why?
>
>> +    }
>> +
>> +    lr4->x1 = lx1;
>> +    lr4->x2 = lx2;
>> +    lr4->y1 = ly1;
>> +    lr4->y2 = ly2;
>> +    lr4->z1 = lz1;
>> +    lr4->z2 = lz2;
>> +}
>> +
>> +void lr4_process_s16(struct lr4 *lr4, int samples, int channels,
>> short *data0)
>> +{
>> +    float lx1 = lr4->x1;
>> +    float lx2 = lr4->x2;
>> +    float ly1 = lr4->y1;
>> +    float ly2 = lr4->y2;
>> +    float lz1 = lr4->z1;
>> +    float lz2 = lr4->z2;
>> +    float lb0 = lr4->b0;
>> +    float lb1 = lr4->b1;
>> +    float lb2 = lr4->b2;
>> +    float la1 = lr4->a1;
>> +    float la2 = lr4->a2;
>> +
>> +    int i;
>> +    for (i = 0; i < samples; i += channels) {
>> +        float x, y, z;
>> +        x = data0[i];
>> +        y = lb0*x + lb1*lx1 + lb2*lx2 - la1*ly1 - la2*ly2;
>> +        z = lb0*y + lb1*ly1 + lb2*ly2 - la1*lz1 - la2*lz2;
>> +        lx2 = lx1;
>> +        lx1 = x;
>> +        ly2 = ly1;
>> +        ly1 = y;
>> +        lz2 = lz1;
>> +        lz1 = z;
>> +        data0[i] = z;
>
> 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?

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

>> +};
>> +
>> +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.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux