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
