Bug#799176: #799176 - libspeexdsp1: Severe clipping of audio when libspeex resampler is used by ALSA or pulse audio

[Erik Montnemery]

The information in the bug report was a bit lacking, I should have
mentioned that the fixed point implementation of the resampler is used on
armhf architecture where I see the issue.
As a side note, why is the fixed point implementation compiled for armhf?

A patch with the necessary changes to libspeex/resampler.c from upstream to
solve the issue can be found at the end of this message.
Please note the changes only affect the fixed point implementation.

The remaining upstream changes to libsspeex/resampler.c seem to be:
 - General cleanup, such as declaring constant as const
 - Better error handling when allocating memory
 - There are some changes in update_filter, but it is not obvious to me if
it is also related to the error handling or actual bug fixes

Patch:
diff -rup rc1.2_debian_changes/speex-1.2~rc1.2/libspeex/arch.h
backport_rc3_minimal/speex-1.2~rc1.2/libspeex/arch.h
--- rc1.2_debian_changes/speex-1.2~rc1.2/libspeex/arch.h 2014-08-26
12:35:22.000000000 +0200
+++ backport_rc3_minimal/speex-1.2~rc1.2/libspeex/arch.h 2015-09-16
18:45:42.226989704 +0200
@@ -171,6 +171,7 @@ typedef float spx_word32_t;
 #define VSHR32(a,shift) (a)
 #define SATURATE16(x,a) (x)
 #define SATURATE32(x,a) (x)
+#define SATURATE32PSHR(x,shift,a) (x)

 #define PSHR(a,shift)       (a)
 #define SHR(a,shift)       (a)
diff -rup rc1.2_debian_changes/speex-1.2~rc1.2/libspeex/fixed_generic.h
backport_rc3_minimal/speex-1.2~rc1.2/libspeex/fixed_generic.h
--- rc1.2_debian_changes/speex-1.2~rc1.2/libspeex/fixed_generic.h 2014-08-26
12:35:22.000000000 +0200
+++ backport_rc3_minimal/speex-1.2~rc1.2/libspeex/fixed_generic.h 2015-09-16
18:45:40.670989642 +0200
@@ -52,6 +52,10 @@
 #define SATURATE16(x,a) (((x)>(a) ? (a) : (x)<-(a) ? -(a) : (x)))
 #define SATURATE32(x,a) (((x)>(a) ? (a) : (x)<-(a) ? -(a) : (x)))

+#define SATURATE32PSHR(x,shift,a) (((x)>=(SHL32(a,shift))) ? (a) : \
+                                   (x)<=-(SHL32(a,shift)) ? -(a) : \
+                                   (PSHR32(x, shift)))
+
 #define SHR(a,shift) ((a) >> (shift))
 #define SHL(a,shift) ((spx_word32_t)(a) << (shift))
 #define PSHR(a,shift) (SHR((a)+((EXTEND32(1)<<((shift))>>1)),shift))
diff -rup rc1.2_debian_changes/speex-1.2~rc1.2/libspeex/resample.c
backport_rc3_minimal/speex-1.2~rc1.2/libspeex/resample.c
--- rc1.2_debian_changes/speex-1.2~rc1.2/libspeex/resample.c 2015-09-15
20:21:30.000000000 +0200
+++ backport_rc3_minimal/speex-1.2~rc1.2/libspeex/resample.c 2015-09-16
18:45:43.878989770 +0200
@@ -337,7 +337,6 @@ static int resampler_basic_direct_single
    const int frac_advance = st->frac_advance;
    const spx_uint32_t den_rate = st->den_rate;
    spx_word32_t sum;
-   int j;

    while (!(last_sample >= (spx_int32_t)*in_len || out_sample >=
(spx_int32_t)*out_len))
    {
@@ -345,20 +344,28 @@ static int resampler_basic_direct_single
       const spx_word16_t *iptr = & in[last_sample];

 #ifndef OVERRIDE_INNER_PRODUCT_SINGLE
-      float accum[4] = {0,0,0,0};
-
+      int j;
+      sum = 0;
+      for(j=0;j<N;j++) sum += MULT16_16(sinc[j], iptr[j]);
+
+/*    This code is slower on most DSPs which have only 2 accumulators.
+      Plus this this forces truncation to 32 bits and you lose the HW
guard bits.
+      I think we can trust the compiler and let it vectorize and/or unroll
itself.
+      spx_word32_t accum[4] = {0,0,0,0};
       for(j=0;j<N;j+=4) {
-        accum[0] += sinc[j]*iptr[j];
-        accum[1] += sinc[j+1]*iptr[j+1];
-        accum[2] += sinc[j+2]*iptr[j+2];
-        accum[3] += sinc[j+3]*iptr[j+3];
+        accum[0] += MULT16_16(sinct[j], iptr[j]);
+        accum[1] += MULT16_16(sinct[j+1], iptr[j+1]);
+        accum[2] += MULT16_16(sinct[j+2], iptr[j+2]);
+        accum[3] += MULT16_16(sinct[j+3], iptr[j+3]);
       }
       sum = accum[0] + accum[1] + accum[2] + accum[3];
+*/
+      sum = SATURATE32PSHR(sum, 15, 32767);
 #else
       sum = inner_product_single(sinc, iptr, N);
 #endif

-      out[out_stride * out_sample++] = PSHR32(sum, 15);
+      out[out_stride * out_sample++] = sum;
       last_sample += int_advance;
       samp_frac_num += frac_advance;
       if (samp_frac_num >= den_rate)
@@ -435,7 +442,6 @@ static int resampler_basic_interpolate_s
    const int int_advance = st->int_advance;
    const int frac_advance = st->frac_advance;
    const spx_uint32_t den_rate = st->den_rate;
-   int j;
    spx_word32_t sum;

    while (!(last_sample >= (spx_int32_t)*in_len || out_sample >=
(spx_int32_t)*out_len))
@@ -452,6 +458,7 @@ static int resampler_basic_interpolate_s


 #ifndef OVERRIDE_INTERPOLATE_PRODUCT_SINGLE
+      int j;
       spx_word32_t accum[4] = {0,0,0,0};

       for(j=0;j<N;j++) {
@@ -463,13 +470,14 @@ static int resampler_basic_interpolate_s
       }

       cubic_coef(frac, interp);
-      sum = MULT16_32_Q15(interp[0],accum[0]) +
MULT16_32_Q15(interp[1],accum[1]) + MULT16_32_Q15(interp[2],accum[2]) +
MULT16_32_Q15(interp[3],accum[3]);
+      sum = MULT16_32_Q15(interp[0],SHR32(accum[0], 1)) +
MULT16_32_Q15(interp[1],SHR32(accum[1], 1)) +
MULT16_32_Q15(interp[2],SHR32(accum[2], 1)) +
MULT16_32_Q15(interp[3],SHR32(accum[3], 1));
+      sum = SATURATE32PSHR(sum, 15, 32767);
 #else
       cubic_coef(frac, interp);
       sum = interpolate_product_single(iptr, st->sinc_table +
st->oversample + 4 - offset - 2, N, st->oversample, interp);
 #endif

-      out[out_stride * out_sample++] = PSHR32(sum,15);
+      out[out_stride * out_sample++] = sum;
       last_sample += int_advance;
       samp_frac_num += frac_advance;
       if (samp_frac_num >= den_rate)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pkg-voip-maintainers/attachments/20150916/ab6da774/attachment-0001.html>