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
