[pulseaudio-discuss] [PATCH 1/2] sconv: Change/fix conversion to/from float32

Tanu Kaskinen tanuk at iki.fi
Sat Jan 12 12:58:41 PST 2013


On Sat, 2013-01-05 at 20:25 +0100, Peter Meerwald wrote:
> From: Peter Meerwald <p.meerwald at bct-electronic.com>
> 
> use (1<<15) instead of 0x7fff as a factor when converting from s16 to float32
> use (1<<31) instead of 0x7fffffff as a factor when converting from s32 to float32
> 
> the change is motivated by the following desireable properties:
> * s16_from_f32(f32_from_s16(x)) == x for all possible s16 values
> * x / (1.0f << 15) == x * (1.0f / (1 << 15)) for all x in s16
> 
> above changes enable easier optimization while guaranteeing bit-exact results
> 
> further, other audio sample conversion code (libavresample) does it the same way
> 
> Signed-off-by: Peter Meerwald <p.meerwald at bct-electronic.com>
> ---
>  src/pulsecore/sconv-s16le.c |   84 +++++++++++++++++++------------------------
>  src/pulsecore/sconv_neon.c  |   17 ++++-----
>  src/pulsecore/sconv_sse.c   |   66 +++++++++++++++++-----------------
>  3 files changed, 75 insertions(+), 92 deletions(-)
> 
> diff --git a/src/pulsecore/sconv-s16le.c b/src/pulsecore/sconv-s16le.c
> index 138e418..e0d7975 100644
> --- a/src/pulsecore/sconv-s16le.c
> +++ b/src/pulsecore/sconv-s16le.c
> @@ -85,11 +85,11 @@ void pa_sconv_s16le_to_float32ne(unsigned n, const int16_t *a, float *b) {
>  #if SWAP_WORDS == 1
>      for (; n > 0; n--) {
>          int16_t s = *(a++);
> -        *(b++) = ((float) INT16_FROM(s))/(float) 0x7FFF;
> +        *(b++) = INT16_FROM(s) * (1.0f / (1 << 15));
>      }
>  #else
>      for (; n > 0; n--)
> -        *(b++) = ((float) (*(a++)))/(float) 0x7FFF;
> +        *(b++) = *(a++) * (1.0f / (1 << 15));
>  #endif
>  }
>  
> @@ -100,11 +100,11 @@ void pa_sconv_s32le_to_float32ne(unsigned n, const int32_t *a, float *b) {
>  #if SWAP_WORDS == 1
>      for (; n > 0; n--) {
>          int32_t s = *(a++);
> -        *(b++) = (float) (((double) INT32_FROM(s))/0x7FFFFFFF);
> +        *(b++) = INT32_FROM(s) * (1.0f / (1U << 31));
>      }
>  #else
>      for (; n > 0; n--)
> -        *(b++) = (float) (((double) (*(a++)))/0x7FFFFFFF);
> +        *(b++) = *(a++) * (1.0f / (1U << 31));
>  #endif
>  }
>  
> @@ -115,18 +115,16 @@ void pa_sconv_s16le_from_float32ne(unsigned n, const float *a, int16_t *b) {
>  #if SWAP_WORDS == 1
>      for (; n > 0; n--) {
>          int16_t s;
> -        float v = *(a++);
> +        float v = *(a++) * (1 << 15);
>  
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.f);
> -        s = (int16_t) lrintf(v * 0x7FFF);
> +        s = (int16_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x8000, 0x7FFF);

If you call lrintf() before clamping the input, you get undefined
results in case the input is very large. I guess this would work:

    s = (int16_t) lrintf(PA_CLAMP_UNLIKELY(v, (float) -0x8000, (float) 0x7FFF));

The same comment applies to all "from float" conversions.

>          *(b++) = INT16_TO(s);
>      }
>  #else
>      for (; n > 0; n--) {
> -        float v = *(a++);
> +        float v = *(a++) * (1 << 15);
>  
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.f);
> -        *(b++) = (int16_t) lrintf(v * 0x7FFF);
> +        *(b++) = (int16_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x8000, 0x7FFF);
>      }
>  #endif
>  }
> @@ -138,18 +136,16 @@ void pa_sconv_s32le_from_float32ne(unsigned n, const float *a, int32_t *b) {
>  #if SWAP_WORDS == 1
>      for (; n > 0; n--) {
>          int32_t s;
> -        float v = *(a++);
> +        float v = *(a++) * (1U << 31);
>  
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> +        s = (int32_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x80000000LL, 0x7FFFFFFFLL);

You probably meant to use llrintf() rather than lrintf(), but if you do
the clamping before the rounding, I guess lrintf() will be enough here
and in the other "from float" functions.

>          *(b++) = INT32_TO(s);
>      }
>  #else
>      for (; n > 0; n--) {
> -        float v = *(a++);
> +        float v = *(a++) * (1U << 31);
>  
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        *(b++) = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> +        *(b++) = (int32_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x80000000LL, 0x7FFFFFFFLL);
>      }
>  #endif
>  }
> @@ -160,7 +156,7 @@ void pa_sconv_s16le_to_float32re(unsigned n, const int16_t *a, float *b) {
>  
>      for (; n > 0; n--) {
>          int16_t s = *(a++);
> -        float k = ((float) INT16_FROM(s))/0x7FFF;
> +        float k = INT16_FROM(s) * (1.0f / (1 << 15));
>          k = PA_FLOAT32_SWAP(k);
>          *(b++) = k;
>      }
> @@ -172,7 +168,7 @@ void pa_sconv_s32le_to_float32re(unsigned n, const int32_t *a, float *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s = *(a++);
> -        float k = (float) (((double) INT32_FROM(s))/0x7FFFFFFF);
> +        float k = INT32_FROM(s) * (1.0f / (1U << 31));
>          k = PA_FLOAT32_SWAP(k);
>          *(b++) = k;
>      }
> @@ -185,9 +181,8 @@ void pa_sconv_s16le_from_float32re(unsigned n, const float *a, int16_t *b) {
>      for (; n > 0; n--) {
>          int16_t s;
>          float v = *(a++);
> -        v = PA_FLOAT32_SWAP(v);
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int16_t) lrintf(v * 0x7FFF);
> +        v = PA_FLOAT32_SWAP(v) * (1 << 15);
> +        s = (int16_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x8000, 0x7FFF);
>          *(b++) = INT16_TO(s);
>      }
>  }
> @@ -199,9 +194,8 @@ void pa_sconv_s32le_from_float32re(unsigned n, const float *a, int32_t *b) {
>      for (; n > 0; n--) {
>          int32_t s;
>          float v = *(a++);
> -        v = PA_FLOAT32_SWAP(v);
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int32_t) lrint((double) v * 0x7FFFFFFF);
> +        v = PA_FLOAT32_SWAP(v) * (1U << 31);
> +        s = (int32_t) PA_CLAMP_UNLIKELY(llrintf(v), -0x80000000LL, 0x7FFFFFFFLL);
>          *(b++) = INT32_TO(s);
>      }
>  }
> @@ -304,9 +298,9 @@ void pa_sconv_s24le_to_float32ne(unsigned n, const uint8_t *a, float *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s = READ24(a) << 8;
> -        *b = ((float) s) / 0x7FFFFFFF;
> +        *b = s * (1.0f / (1U << 31));
>          a += 3;
> -        b ++;
> +        b++;
>      }
>  }
>  
> @@ -316,12 +310,11 @@ void pa_sconv_s24le_from_float32ne(unsigned n, const float *a, uint8_t *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s;
> -        float v = *a;
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> +        float v = *a * (1U << 31);
> +        s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
>          WRITE24(b, ((uint32_t) s) >> 8);
>          a++;
> -        b+=3;
> +        b += 3;
>      }
>  }
>  
> @@ -331,10 +324,10 @@ void pa_sconv_s24le_to_float32re(unsigned n, const uint8_t *a, float *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s = READ24(a) << 8;
> -        float k = ((float) s) / 0x7FFFFFFF;
> +        float k = s * (1.0f / (1U << 31));
>          *b = PA_FLOAT32_SWAP(k);
>          a += 3;
> -        b ++;
> +        b++;
>      }
>  }
>  
> @@ -345,9 +338,8 @@ void pa_sconv_s24le_from_float32re(unsigned n, const float *a, uint8_t *b) {
>      for (; n > 0; n--) {
>          int32_t s;
>          float v = *a;
> -        v = PA_FLOAT32_SWAP(v);
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> +        v = PA_FLOAT32_SWAP(v) * (1U << 31);
> +        s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
>          WRITE24(b, ((uint32_t) s) >> 8);
>          a++;
>          b+=3;
> @@ -406,9 +398,9 @@ void pa_sconv_s24_32le_to_float32ne(unsigned n, const uint32_t *a, float *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s = (int32_t) (UINT32_FROM(*a) << 8);
> -        *b = (float) s / (float) 0x7FFFFFFF;
> -        a ++;
> -        b ++;
> +        *b = s * (1.0f / (1U << 31));
> +        a++;
> +        b++;
>      }
>  }
>  
> @@ -418,10 +410,10 @@ void pa_sconv_s24_32le_to_float32re(unsigned n, const uint32_t *a, float *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s = (int32_t) (UINT32_FROM(*a) << 8);
> -        float k = (float) s / (float) 0x7FFFFFFF;
> +        float k = s * (1.0f / (1U << 31));
>          *b = PA_FLOAT32_SWAP(k);
> -        a ++;
> -        b ++;
> +        a++;
> +        b++;
>      }
>  }
>  
> @@ -431,9 +423,8 @@ void pa_sconv_s24_32le_from_float32ne(unsigned n, const float *a, uint32_t *b) {
>  
>      for (; n > 0; n--) {
>          int32_t s;
> -        float v = *a;
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> +        float v = *a * (1U << 31);
> +        s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
>          *b = UINT32_TO(((uint32_t) s) >> 8);
>          a++;
>          b++;
> @@ -447,9 +438,8 @@ void pa_sconv_s24_32le_from_float32re(unsigned n, const float *a, uint32_t *b) {
>      for (; n > 0; n--) {
>          int32_t s;
>          float v = *a;
> -        v = PA_FLOAT32_SWAP(v);
> -        v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> -        s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> +        v = PA_FLOAT32_SWAP(v) * (1U << 31);
> +        s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
>          *b = UINT32_TO(((uint32_t) s) >> 8);
>          a++;
>          b++;
> diff --git a/src/pulsecore/sconv_neon.c b/src/pulsecore/sconv_neon.c
> index 6fd966d..111b56f 100644
> --- a/src/pulsecore/sconv_neon.c
> +++ b/src/pulsecore/sconv_neon.c
> @@ -36,16 +36,11 @@ static void pa_sconv_s16le_from_f32ne_neon(unsigned n, const float *src, int16_t
>          "movs       %[n], %[n], lsr #2      \n\t"
>          "beq        2f                      \n\t"
>  
> -        "vdup.f32   q2, %[plusone]          \n\t"
> -        "vneg.f32   q3, q2                  \n\t"
> -        "vdup.f32   q4, %[scale]            \n\t"
> -        "vdup.u32   q5, %[mask]             \n\t"
> +        "vdup.f32   q1, %[scale]            \n\t"
>  
>          "1:                                 \n\t"
>          "vld1.32    {q0}, [%[src]]!         \n\t"
> -        "vmin.f32   q0, q0, q2              \n\t" /* clamp */
> -        "vmax.f32   q0, q0, q3              \n\t"
> -        "vmul.f32   q0, q0, q4              \n\t" /* scale */
> +        "vmul.f32   q0, q0, q1              \n\t" /* scale */
>          "vcvt.s32.f32 q0, q0, #16           \n\t" /* narrow */

You removed clamping - what happens if there's need for clamping? (I'm
not very good at reading assembly.)

>          "vrshrn.s32  d0, q0, #16            \n\t"
>          "subs       %[n], %[n], #1          \n\t"
> @@ -55,13 +50,13 @@ static void pa_sconv_s16le_from_f32ne_neon(unsigned n, const float *src, int16_t
>          "2:                                 \n\t"
>  
>          : [dst] "+r" (dst), [src] "+r" (src), [n] "+r" (n) /* output operands (or input operands that get modified) */
> -        : [plusone] "r" (1.0f), [scale] "r" (32767.0f), [mask] "r" (0x80000000) /* input operands */
> -        : "memory", "cc", "q0", "q1", "q2", "q3", "q4", "q5", "q6" /* clobber list */
> +        : [scale] "r" (32768.0f) /* input operands */
> +        : "memory", "cc", "q0", "q1" /* clobber list */
>      );
>  
>      /* leftovers */
>      while (i--) {
> -        *dst++ = (int16_t) lrintf(PA_CLAMP_UNLIKELY(*src, -1.0f, 1.0f) * 0x7FFF);
> +        *dst++ = (int16_t) PA_CLAMP_UNLIKELY(lrintf(*src * (1 << 15)), -0x8000, 0x7FFF);
>          src++;
>      }
>  }
> @@ -69,7 +64,7 @@ static void pa_sconv_s16le_from_f32ne_neon(unsigned n, const float *src, int16_t
>  static void pa_sconv_s16le_to_f32ne_neon(unsigned n, const int16_t *src, float *dst) {
>      unsigned i = n & 3;
>  
> -    const float invscale = 1.0f / 0x7FFF;
> +    const float invscale = 1.0f / (1 << 15);
>  
>      __asm__ __volatile__ (
>          "movs        %[n], %[n], lsr #2     \n\t"
> diff --git a/src/pulsecore/sconv_sse.c b/src/pulsecore/sconv_sse.c
> index 3b56a9c..072d329 100644
> --- a/src/pulsecore/sconv_sse.c
> +++ b/src/pulsecore/sconv_sse.c
> @@ -35,17 +35,13 @@
>  
>  #if !defined(__APPLE__) && defined (__i386__) || defined (__amd64__)
>  
> -static const PA_DECLARE_ALIGNED (16, float, one[4]) = { 1.0, 1.0, 1.0, 1.0 };
> -static const PA_DECLARE_ALIGNED (16, float, mone[4]) = { -1.0, -1.0, -1.0, -1.0 };
> -static const PA_DECLARE_ALIGNED (16, float, scale[4]) = { 0x7fff, 0x7fff, 0x7fff, 0x7fff };
> +static const PA_DECLARE_ALIGNED (16, float, scale[4]) = { 0x8000, 0x8000, 0x8000, 0x8000 };
>  
>  static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b) {
>      pa_reg_x86 temp, i;
>  
>      __asm__ __volatile__ (
>          " movaps %5, %%xmm5             \n\t"
> -        " movaps %6, %%xmm6             \n\t"
> -        " movaps %7, %%xmm7             \n\t"
>          " xor %0, %0                    \n\t"
>  
>          " mov %4, %1                    \n\t"
> @@ -56,12 +52,8 @@ static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b
>          "1:                             \n\t"
>          " movups (%q2, %0, 2), %%xmm0   \n\t" /* read 8 floats */
>          " movups 16(%q2, %0, 2), %%xmm2 \n\t"
> -        " minps  %%xmm5, %%xmm0         \n\t" /* clamp to 1.0 */
> -        " minps  %%xmm5, %%xmm2         \n\t"
> -        " maxps  %%xmm6, %%xmm0         \n\t" /* clamp to -1.0 */
> -        " maxps  %%xmm6, %%xmm2         \n\t"
> -        " mulps  %%xmm7, %%xmm0         \n\t" /* *= 0x7fff */
> -        " mulps  %%xmm7, %%xmm2         \n\t"
> +        " mulps  %%xmm5, %%xmm0         \n\t" /* *= 0x8000 */
> +        " mulps  %%xmm5, %%xmm2         \n\t"
>  
>          " cvtps2pi %%xmm0, %%mm0        \n\t" /* low part to int */
>          " cvtps2pi %%xmm2, %%mm2        \n\t"
> @@ -73,7 +65,7 @@ static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b
>          " packssdw %%mm1, %%mm0         \n\t" /* pack parts */
>          " packssdw %%mm3, %%mm2         \n\t"
>          " movq     %%mm0, (%q3, %0)     \n\t"
> -        " movq    %%mm2, 8(%q3, %0)     \n\t"
> +        " movq     %%mm2, 8(%q3, %0)    \n\t"
>  
>          " add $16, %0                   \n\t"
>          " dec %1                        \n\t"
> @@ -82,24 +74,30 @@ static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b
>          "2:                             \n\t"
>          " mov %4, %1                    \n\t" /* prepare for leftovers */
>          " and $7, %1                    \n\t"
> -        " je 4f                         \n\t"
> +        " je 5f                         \n\t"
>  
>          "3:                             \n\t"
>          " movss (%q2, %0, 2), %%xmm0    \n\t"
> -        " minss  %%xmm5, %%xmm0         \n\t"
> -        " maxss  %%xmm6, %%xmm0         \n\t"
> -        " mulss  %%xmm7, %%xmm0         \n\t"
> +        " mulss  %%xmm5, %%xmm0         \n\t"
> +        " cvtss2si %%xmm0, %4           \n\t"
> +        " add $0x8000, %4               \n\t" /* check for saturation */
> +        " and $~0xffff, %4              \n\t"
>          " cvtss2si %%xmm0, %4           \n\t"
> -        " movw  %w4, (%q3, %0)          \n\t"
> +        " je 4f                         \n\t"
> +        " sar $31, %4                   \n\t"
> +        " xor $0x7fff, %4               \n\t"
> +
> +        "4:                             \n\t"
> +        " movw  %w4, (%q3, %0)          \n\t" /* store leftover */
>          " add $2, %0                    \n\t"
>          " dec %1                        \n\t"
>          " jne 3b                        \n\t"
>  
> -        "4:                             \n\t"
> +        "5:                             \n\t"
>          " emms                          \n\t"
>  
>          : "=&r" (i), "=&r" (temp)
> -        : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*one), "m" (*mone), "m" (*scale)
> +        : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*scale)
>          : "cc", "memory"
>      );
>  }
> @@ -109,8 +107,6 @@ static void pa_sconv_s16le_from_f32ne_sse2(unsigned n, const float *a, int16_t *
>  
>      __asm__ __volatile__ (
>          " movaps %5, %%xmm5             \n\t"
> -        " movaps %6, %%xmm6             \n\t"
> -        " movaps %7, %%xmm7             \n\t"
>          " xor %0, %0                    \n\t"
>  
>          " mov %4, %1                    \n\t"
> @@ -121,12 +117,8 @@ static void pa_sconv_s16le_from_f32ne_sse2(unsigned n, const float *a, int16_t *
>          "1:                             \n\t"
>          " movups (%q2, %0, 2), %%xmm0   \n\t" /* read 8 floats */
>          " movups 16(%q2, %0, 2), %%xmm2 \n\t"
> -        " minps  %%xmm5, %%xmm0         \n\t" /* clamp to 1.0 */
> -        " minps  %%xmm5, %%xmm2         \n\t"
> -        " maxps  %%xmm6, %%xmm0         \n\t" /* clamp to -1.0 */
> -        " maxps  %%xmm6, %%xmm2         \n\t"
> -        " mulps  %%xmm7, %%xmm0         \n\t" /* *= 0x7fff */
> -        " mulps  %%xmm7, %%xmm2         \n\t"
> +        " mulps  %%xmm5, %%xmm0         \n\t" /* *= 0x8000 */
> +        " mulps  %%xmm5, %%xmm2         \n\t"
>  
>          " cvtps2dq %%xmm0, %%xmm0       \n\t"
>          " cvtps2dq %%xmm2, %%xmm2       \n\t"
> @@ -141,23 +133,29 @@ static void pa_sconv_s16le_from_f32ne_sse2(unsigned n, const float *a, int16_t *
>          "2:                             \n\t"
>          " mov %4, %1                    \n\t" /* prepare for leftovers */
>          " and $7, %1                    \n\t"
> -        " je 4f                         \n\t"
> +        " je 5f                         \n\t"
>  
>          "3:                             \n\t"
>          " movss (%q2, %0, 2), %%xmm0    \n\t"
> -        " minss  %%xmm5, %%xmm0         \n\t"
> -        " maxss  %%xmm6, %%xmm0         \n\t"
> -        " mulss  %%xmm7, %%xmm0         \n\t"
> +        " mulss  %%xmm5, %%xmm0         \n\t"
> +        " cvtss2si %%xmm0, %4           \n\t"
> +        " add $0x8000, %4               \n\t"
> +        " and $~0xffff, %4              \n\t" /* check for saturation */
>          " cvtss2si %%xmm0, %4           \n\t"
> -        " movw  %w4, (%q3, %0)          \n\t"
> +        " je 4f                         \n\t"
> +        " sar $31, %4                   \n\t"
> +        " xor $0x7fff, %4               \n\t"
> +
> +        "4:                             \n\t"
> +        " movw  %w4, (%q3, %0)          \n\t" /* store leftover */
>          " add $2, %0                    \n\t"
>          " dec %1                        \n\t"
>          " jne 3b                        \n\t"
>  
> -        "4:                             \n\t"
> +        "5:                             \n\t"
>  
>          : "=&r" (i), "=&r" (temp)
> -        : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*one), "m" (*mone), "m" (*scale)
> +        : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*scale)
>          : "cc", "memory"
>      );
>  }

The x86 code remains unreviewed. I was able to guess to some extent
what's happening in the arm code, but can't do that for the x86 code.
Maybe Arun will check this, or maybe I should learn this stuff properly.

-- 
Tanu



More information about the pulseaudio-discuss mailing list