29.01.2015 03:14, David Henningsson wrote:
> The chrome OS audio server has some already existing code, which
> has been made available under a BSD-style license, which should be
> safe to import by us.
>
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
> LICENSE | 3 +
> src/pulsecore/filter/LICENSE.WEBKIT | 27 +++
> src/pulsecore/filter/biquad.c | 368 ++++++++++++++++++++++++++++++++++++
> src/pulsecore/filter/biquad.h | 57 ++++++
> src/pulsecore/filter/crossover.c | 188 ++++++++++++++++++
> src/pulsecore/filter/crossover.h | 70 +++++++
> 6 files changed, 713 insertions(+)
> create mode 100644 src/pulsecore/filter/LICENSE.WEBKIT
> create mode 100644 src/pulsecore/filter/biquad.c
> create mode 100644 src/pulsecore/filter/biquad.h
> create mode 100644 src/pulsecore/filter/crossover.c
> create mode 100644 src/pulsecore/filter/crossover.h
>
> diff --git a/LICENSE b/LICENSE
> index 226c4ce..6932317 100644
> --- a/LICENSE
> +++ b/LICENSE
> @@ -29,6 +29,9 @@ considered too small and stable to be considered as an external library) use the
> more permissive MIT license. This include the device reservation DBus protocol
> and realtime kit implementations.
>
> +A more permissive BSD-style license is used for LFE filters, see
> +src/pulsecore/filter/LICENSE.WEBKIT for details.
> +
> Additionally, a more permissive Sun license is used for code that performs
> u-law, A-law and linear PCM conversions.
>
> diff --git a/src/pulsecore/filter/LICENSE.WEBKIT b/src/pulsecore/filter/LICENSE.WEBKIT
> new file mode 100644
> index 0000000..2f69d9f
> --- /dev/null
> +++ b/src/pulsecore/filter/LICENSE.WEBKIT
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + * its contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> diff --git a/src/pulsecore/filter/biquad.c b/src/pulsecore/filter/biquad.c
> new file mode 100644
> index 0000000..b28256d
> --- /dev/null
> +++ b/src/pulsecore/filter/biquad.c
> @@ -0,0 +1,368 @@
> +/* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE file.
> + */
> +
> +/* Copyright (C) 2010 Google Inc. All rights reserved.
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.WEBKIT file.
> + */
> +
> +#include <math.h>
> +#include "biquad.h"
> +
> +#ifndef max
> +#define max(a, b) ({ __typeof__(a) _a = (a); \
> + __typeof__(b) _b = (b); \
> + _a > _b ? _a : _b; })
> +#endif
> +
> +#ifndef min
> +#define min(a, b) ({ __typeof__(a) _a = (a); \
> + __typeof__(b) _b = (b); \
> + _a < _b ? _a : _b; })
> +#endif
> +
> +#ifndef M_PI
> +#define M_PI 3.14159265358979323846
> +#endif
> +
> +static void set_coefficient(struct biquad *bq, double b0, double b1, double b2,
> + double a0, double a1, double a2)
> +{
> + double a0_inv = 1 / a0;
> + bq->b0 = b0 * a0_inv;
> + bq->b1 = b1 * a0_inv;
> + bq->b2 = b2 * a0_inv;
> + bq->a1 = a1 * a0_inv;
> + bq->a2 = a2 * a0_inv;
> +}
> +
> +static void biquad_lowpass(struct biquad *bq, double cutoff, double resonance)
> +{
> + /* Limit cutoff to 0 to 1. */
> + cutoff = max(0.0, min(cutoff, 1.0));
> +
> + if (cutoff == 1) {
> + /* When cutoff is 1, the z-transform is 1. */
> + set_coefficient(bq, 1, 0, 0, 1, 0, 0);
> + } else if (cutoff > 0) {
> + /* Compute biquad coefficients for lowpass filter */
> + resonance = max(0.0, resonance); /* can't go negative */
> + double g = pow(10.0, 0.05 * resonance);
> + double d = sqrt((4 - sqrt(16 - 16 / (g * g))) / 2);
> +
> + double theta = M_PI * cutoff;
> + double sn = 0.5 * d * sin(theta);
> + double beta = 0.5 * (1 - sn) / (1 + sn);
> + double gamma = (0.5 + beta) * cos(theta);
> + double alpha = 0.25 * (0.5 + beta - gamma);
> +
> + double b0 = 2 * alpha;
> + double b1 = 2 * 2 * alpha;
> + double b2 = 2 * alpha;
> + double a1 = 2 * -gamma;
> + double a2 = 2 * beta;
> +
> + set_coefficient(bq, b0, b1, b2, 1, a1, a2);
> + } else {
> + /* When cutoff is zero, nothing gets through the filter, so set
> + * coefficients up correctly.
> + */
> + set_coefficient(bq, 0, 0, 0, 1, 0, 0);
> + }
> +}
This definitely comes from some book, so a reference would be nice.
Also, even though the function is static, it would be nice to describe
the meaning (including units) of each parameter. And why do we have
"double resonance" with some decibel-conversion applied to it, instead
of the more-common Q-factor? Does this function come from the "matched
Z-transform", "bilinear transform" or "impulse invariance" method of
digital filter design? [just to check the understanding]
(well, by comparing with a known set of filter coefficients for certain
parameters, I can tell that a bilinear transform and frequency-warping
is apparently used here, and that "resonance == 0", as passed from
crossover.c, indeed results in a Butterworth filter)
IOW, too much "scientific code" without proper comments, references, or
simple testcases.
> +
> +static void biquad_highpass(struct biquad *bq, double cutoff, double resonance)
(trimming the quotation, because the above is enough to illustrate the
point)
> +static void biquad_bandpass(struct biquad *bq, double frequency, double Q)
...
> +static void biquad_peaking(struct biquad *bq, double frequency, double Q,
> + double db_gain)
> +void biquad_set(struct biquad *bq, enum biquad_type type, double freq, double Q,
> + double gain)
Of the above, you use only lowpass and highpass biquad filters. I.e. too
much dead code. Please delete, because we are not implementing Web Audio
API, and because the math cannot change and thus does not have to be
maintained.
Besides, the function uses a very non-standard definition of Q. In this
function, to get a Butterworth filter, Q == 0 is passed, while,
according to the traditional definition, Q == 0.707 should result in a
Butterworth filter.
Currently, we only need to be able to design 2nd order highpass and
lowpass Butterworth filters with a given cutoff frequency, which is
supposed to be very low as compared to the sample rate, so that the
"bilinear transform vs other alternatives" distinction does not matter.
Later, for the "virtual headphone" effect that is also based on IIR
filters, I would need to set the biquad coefficients directly (e.g.,
load from a file).
If adding proper comments to all the math is a too-hard requirement,
then, in v2, I'd suggest to (temporarily?) squash patches 1 and 2 for
easier review, and to remove all unused functions and parameters.
> diff --git a/src/pulsecore/filter/biquad.h b/src/pulsecore/filter/biquad.h
Not reviewing, as all criticisms are already expressed above.
> +/* The biquad filter parameters. The transfer function H(z) is (b0 + b1 * z^(-1)
> + * + b2 * z^(-2)) / (1 + a1 * z^(-1) + a2 * z^(-2)). The previous two inputs
> + * are stored in x1 and x2, and the previous two outputs are stored in y1 and
> + * y2.
> + *
> + * We use double during the coefficients calculation for better accurary, but
> + * float is used during the actual filtering for faster computation.
> + */
Yes, this is the kind of comments that I am looking for.
> +struct biquad {
> + float b0, b1, b2;
> + float a1, a2;
> + float x1, x2;
> + float y1, y2;
> +};
You don't seem to use this directly, but copy the coefficients and the
history into struct lr4. Maybe it makes sense to remove x1, x2, y1, y2
from this structure, and use a "struct biquad coeffs;" inside struct lr4?
> +static void lr4_set(struct lr4 *lr4, enum biquad_type type, float freq)
> +{
> + struct biquad q;
> + biquad_set(&q, type, freq, 0, 0);
I have verified that passing 0, 0 as the last two parameters indeed
yields a Butterworth filter of 2nd order.
> +/* Split input data using two LR4 filters, put the result into the input array
> + * and another array.
> + *
> + * data0 --+-- lp --> data0
> + * |
> + * \-- hp --> data1
Why? Later you seem to fight with the in-place processing in PATCH 5/6,
"Make sure we get a copy - we want to keep our original unchanged".
> + */
> +static void lr4_split(struct lr4 *lp, struct lr4 *hp, int count, float *data0,
> + float *data1)
> +{
The implementation of this strange function is correct.
> +/* Split input data using two LR4 filters and sum them back to the original
> + * data array.
> + *
> + * data --+-- lp --+--> data
> + * | |
> + * \-- hp --/
> + */
> +static void lr4_merge(struct lr4 *lp, struct lr4 *hp, int count, float *data)
> +{
Also correct.
But later (in PATCH 2/6) you write your own functions that contain the
same math. Again, this is an argument for squashing the first two
patches (at least for review purposes).
> diff --git a/src/pulsecore/filter/crossover.h b/src/pulsecore/filter/crossover.h
> new file mode 100644
> index 0000000..99a601c
> --- /dev/null
> +++ b/src/pulsecore/filter/crossover.h
> @@ -0,0 +1,70 @@
> +/* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE file.
> + */
> +
> +#ifndef CROSSOVER_H_
> +#define CROSSOVER_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* An LR4 filter is two biquads with the same parameters connected in series:
> + *
> + * x -- [BIQUAD] -- y -- [BIQUAD] -- z
> + *
> + * Both biquad filter has the same parameter b[012] and a[12],
> + * The variable [xyz][12] keep the history values.
> + */
> +struct lr4 {
> + float b0, b1, b2;
> + float a1, a2;
> + float x1, x2;
> + float y1, y2;
> + float z1, z2;
> +};
> +
> +/* Three bands crossover filter:
> + *
> + * INPUT --+-- lp0 --+-- lp1 --+---> LOW (0)
> + * | | |
> + * | \-- hp1 --/
> + * |
> + * \-- hp0 --+-- lp2 ------> MID (1)
> + * |
> + * \-- hp2 ------> HIGH (2)
> + *
> + * [f0] [f1]
> + *
> + * Each lp or hp is an LR4 filter, which consists of two second-order
> + * lowpass or highpass butterworth filters.
> + */
Do we actually need this three-band filter on any known hardware?
--
Alexander E. Patrakov
