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
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
