[PATCH] resampler: Support speex resampler compiled with FIXED_POINT

[patrakov@xxxxxxxxx (Alexander E. Patrakov)]

03.04.2014 04:27, Peter Meerwald wrote:
> When Speex is compiled with FIXED_POINT defined, it scales float input
> to +/-32768 instead of +/-1. In order to make floating point resampler
> work with speex compiled with FIXED_POINT, we need to rescale the input
> to speex.

Right, the problem does exist.

> Unfortunately, there is no easy way to tell how speex has been built,
> so we probe at runtime.

Right, the headers and the set of exported symbols in the speexdsp 
library are the same in the two possible cases.

>
> A patch to fix speex was rejected saying that speex API is stable.
>
> Further discussion is here
> http://lists.openembedded.org/pipermail/openembedded-core/2014-January/087886.html

I don't see any pointer to the speex list, could you please add one to a 
comment below?

> +/* speexdsp has a nasty issue: when built with FIXED_POINT, the float resampler
> + * expects samples in [-32768,32768] instead of [-1,1]; there is no easy way
> + * to figure out how speexdsp has been compiled. */
> +
> +static unsigned (*speex_resampler_float)(pa_resampler *r, const pa_memchunk *input, unsigned in_n_frames, pa_memchunk *output, unsigned *out_n_frames) = speex_resample_float;
> +
> +static void speex_probe_fixed_point() {
> +    PA_ONCE_BEGIN {
> +        float f_out = -1.0f, f_in = 1.0f;
> +        spx_uint32_t in_len = 1, out_len = 1;
> +
> +        SpeexResamplerState *s = speex_resampler_init(1, 1, 1,
> +            SPEEX_RESAMPLER_QUALITY_MIN, NULL);
> +        if (!s)
> +            goto done;
> +
> +        /* feed one sample */
> +        if (speex_resampler_process_float(s, 0, &f_in, &in_len,
> +            &f_out, &out_len) != RESAMPLER_ERR_SUCCESS)
> +            goto done;
> +
> +        /* expecting sample has been processed, one sample output */
> +        if (in_len != 1 && out_len != 1)
> +            goto done;
> +
> +        /* FIXED_POINT implementation will output 0.0 */
> +        if (fabsf(f_out) < 0.00001f)
> +            speex_resampler_float = speex_resample_float_fixed;
> +
> +done:
> +        if (s)
> +            speex_resampler_destroy(s);
> +    } PA_ONCE_END;
> +}

I'd say that this function either should not be void or should abort on 
speex_resampler_init() failure. Right now, you just hide this failure.

My overall opinion on this patch is mixed. If the right solution is to 
work around this speex bug and if my notes above are taken into account, 
then the patch is fine. However, there are other choices that need to be 
considered before applying this patch:

  * Poking speex guys again - their stable API is unusable.
  * A compile-time check (with a way to provide an answer if 
cross-compiling) that just fails on speex compiled with 
--enable-fixed-point.
  * Documentation of the requirement that speex must not be compiled 
with --enable-fixed-point, with a clear focus on the fact that it is a 
bug in speex.
  * Putting a copy of the speex resampler into PulseAudio source tree 
and fixing it locally, with appropriate warnings to the distributors. 
Possible problem: conflict with the external speex echo canceller.

I defer the choice between the original patch and these alternatives to 
someone else.

-- 
Alexander E. Patrakov