[pulseaudio-discuss] [SUMMARY PATCH] lfe-filter: The first three patches (import, adjust, cleanup)

[Alexander E. Patrakov]

17.02.2015 17:29, David Henningsson wrote:
> This is the combined diff of the first three patches for easier review.

> +static void biquad_lowpass(struct biquad *bq, double cutoff, double resonance)
> +{
> +	/* Limit cutoff to 0 to 1. */
> +	cutoff = PA_MIN(cutoff, 1.0);
> +	cutoff = PA_MAX(0.0, cutoff);
> +
> +	if (cutoff >= 1.0) {
> +		/* 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 */
> +		double r = PA_MAX(0.0, resonance); /* can't go negative */
> +		double g = pow(10.0, 0.05 * r);

This pow() converts something from decibels to regular units. The code 
below passes Q here. However, the usual electric-engineering definition 
of Q does not involve decibels. So this is something very strange.

But, as Q (in this weird definition) is always 0, I'd suggest that the 
resonance and Q parameters are removed altogether. Thus, the 
biquad_lowpass() function will only be able to design Butterworth 
filters, but that's exactly what we need.

Same for biquad_highpass().

And we don't need BQ_NONE, either. On the good side, I found no other 
obviously-dead code.

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

> +/* An LR4 filter, implemented as a chain of two Butterworth filters.
> +
> +   Currently the channel map is fixed so that a highpass filter is applied to all
> +   channels except for the LFE channel, where a lowpass filter is applied.
> +   This works well for e g stereo to 2.1/5.1/7.1 scenarios, where the remap engine
> +   has calculated the LFE channel to be the average of all source channels.
> +*/

Yes, this comment addresses my earlier suggestion well. Thanks!

> +pa_memchunk * pa_lfe_filter_process(pa_lfe_filter_t *f, pa_memchunk *buf) {
> +    int samples = buf->length / pa_sample_size_of_format(f->ss.format);
> +
> +    if (!f->active)
> +        return buf;
> +    if (f->ss.format == PA_SAMPLE_FLOAT32NE) {
> +        int i;
> +        float *data = pa_memblock_acquire_chunk(buf);
> +        for (i = 0; i < f->cm.channels; i++)
> +            lr4_process_float32(&f->lr4[i], samples, f->cm.channels, &data[i], &data[i]);

I tried to review this parameter-passing, and it looks correct to me. 
But the very fact that I paused to review this particular line made me 
think that it might be a good idea to add a pa_assert(samples % channels 
== 0) into lr4_process_float32().

> +        pa_memblock_release(buf->memblock);
> +    }
> +    else if (f->ss.format == PA_SAMPLE_S16NE) {
> +        int i;
> +        short *data = pa_memblock_acquire_chunk(buf);
> +        for (i = 0; i < f->cm.channels; i++)
> +            lr4_process_s16(&f->lr4[i], samples, f->cm.channels, &data[i], &data[i]);
> +        pa_memblock_release(buf->memblock);
> +    }
> +    else pa_assert_not_reached();
> +    return buf;
> +}

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

Didn't we recently apply a patch that removes the FSF address?

This review is incomplete, I intend to do one or more additional passes 
later, or after resubmissions.

-- 
Alexander E. Patrakov