[pulseaudio-discuss] [PATCH 04/10] remap: Add stereo to mono special case remapping

[Tanu Kaskinen]

On Fri, 2013-03-29 at 16:56 +0100, Peter Meerwald wrote:
> From: Peter Meerwald <p.meerwald at bct-electronic.com>
> 
> add special-case C code for stereo-to-mone remapping
> 
> v2:
> * use consistent array addressing
> 
> Signed-off-by: Peter Meerwald <p.meerwald at bct-electronic.com>
> ---
>  src/pulsecore/remap.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pulsecore/remap.c b/src/pulsecore/remap.c
> index 5d0cd62..9300ab0 100644
> --- a/src/pulsecore/remap.c
> +++ b/src/pulsecore/remap.c
> @@ -86,6 +86,53 @@ static void remap_mono_to_stereo_c(pa_remap_t *m, void *dst, const void *src, un
>      }
>  }
>  
> +static void remap_stereo_to_mono_c(pa_remap_t *m, void *dst, const void *src, unsigned n) {
> +    unsigned i;
> +
> +    switch (*m->format) {
> +        case PA_SAMPLE_FLOAT32NE:
> +        {
> +            float *d = (float *) dst, *s = (float *) src;
> +
> +            for (i = n >> 2; i > 0; i--) {
> +                d[0] = (s[0] + s[1])*0.5f;
> +                d[1] = (s[2] + s[3])*0.5f;
> +                d[2] = (s[4] + s[5])*0.5f;
> +                d[3] = (s[6] + s[7])*0.5f;
> +                s += 8;
> +                d += 4;
> +            }
> +            for (i = n & 3; i; i--) {
> +                d[0] = (s[0] + s[1])*0.5f;
> +                s += 2;
> +                d += 1;
> +            }
> +            break;
> +        }
> +        case PA_SAMPLE_S16NE:
> +        {
> +            int16_t *d = (int16_t *) dst, *s = (int16_t *) src;
> +
> +            for (i = n >> 2; i > 0; i--) {
> +                d[0] += (s[0] + s[1])/2;
> +                d[1] += (s[2] + s[3])/2;
> +                d[2] += (s[4] + s[5])/2;
> +                d[3] += (s[6] + s[7])/2;

You probably meant '=', not '+='?

Also, s[0] + s[1] can overflow, so the inputs should be divided
individually before summing.

> +                s += 8;
> +                d += 4;
> +            }
> +            for (i = n & 3; i; i--) {
> +                d[0] += (s[0] + s[1])/2;
> +                s += 2;
> +                d += 1;
> +            }
> +            break;
> +        }
> +        default:
> +            pa_assert_not_reached();
> +    }
> +}
> +
>  static void remap_channels_matrix_c(pa_remap_t *m, void *dst, const void *src, unsigned n) {
>      unsigned oc, ic, i;
>      unsigned n_ic, n_oc;
> @@ -172,13 +219,16 @@ static void init_remap_c(pa_remap_t *m) {
>              m->map_table_i[0][0] == PA_VOLUME_NORM && m->map_table_i[1][0] == PA_VOLUME_NORM) {
>          m->do_remap = (pa_do_remap_func_t) remap_mono_to_stereo_c;
>          pa_log_info("Using mono to stereo remapping");
> +    } else if (n_ic == 2 && n_oc == 1 &&
> +            m->map_table_i[0][0] == PA_VOLUME_HALF && m->map_table_i[0][1] == PA_VOLUME_HALF) {

While this happens to work, I don't think it's correct to use
PA_VOLUME_HALF here. PA_VOLUME_NORM is a pa_volume_t value, and
pa_volume_t is not meant to be used as a multiplication factor type. I'd
expect PA_VOLUME_HALF to be a pa_volume_t constant too, not a
multiplication factor constant.

I should have complained about the use of PA_VOLUME_NORM in commit
578d2ce5, but I didn't realize at that time that map_table_i stored
multiplication factors, not pa_volume_t values.

I think plain numbers would be OK here, although adding new constants
for 16:16 fixed-point values might work too.

-- 
Tanu