[MPlayer-dev-eng] [PATCH] 8 channel support

[Reimar Döffinger]

On Tue, Nov 03, 2009 at 08:40:41PM -0500, Jason Tackaberry wrote:
> +        REORDER_COPY_8(dest_8,src_8,samples,s0,s1,s2,s3,s4,s5,s6,s7);

Maybe add a few spaces?

> +        for (i = 0; i < samples; i += 24) {
> +            dest_8[i]    = src_8[i+s0*3];
> +            dest_8[i+1]  = src_8[i+s0*3+1];
> +            dest_8[i+2]  = src_8[i+s0*3+2];
> +            dest_8[i+3]  = src_8[i+s1*3];
> +            dest_8[i+4]  = src_8[i+s1*3+1];
> +            dest_8[i+5]  = src_8[i+s1*3+2];
> +            dest_8[i+6]  = src_8[i+s2*3];
> +            dest_8[i+7]  = src_8[i+s2*3+1];
> +            dest_8[i+8]  = src_8[i+s2*3+2];
> +            dest_8[i+9]  = src_8[i+s3*3];
> +            dest_8[i+10] = src_8[i+s3*3+1];
> +            dest_8[i+11] = src_8[i+s3*3+2];
> +            dest_8[i+12] = src_8[i+s4*3];
> +            dest_8[i+13] = src_8[i+s4*3+1];
> +            dest_8[i+14] = src_8[i+s4*3+2];
> +            dest_8[i+15] = src_8[i+s5*3];
> +            dest_8[i+16] = src_8[i+s5*3+1];
> +            dest_8[i+17] = src_8[i+s5*3+2];
> +            dest_8[i+18] = src_8[i+s6*3];
> +            dest_8[i+19] = src_8[i+s6*3+1];
> +            dest_8[i+20] = src_8[i+s6*3+2];
> +            dest_8[i+21] = src_8[i+s7*3];
> +            dest_8[i+22] = src_8[i+s7*3+1];
> +            dest_8[i+23] = src_8[i+s7*3+2];

I'd suggest using *dest_8++ and aligning the ]; at the end, too,
for minimally better readability.

> @@ -327,6 +425,9 @@
>          if (chnum==6) {
>              REORDER_SELF_SWAP_2(src_8,tmp,samples,6,s0,s1);
>          }
> +        else if (chnum==8) {
> +            REORDER_SELF_SWAP_2(src_8,tmp,samples,8,s0,s1);
> +        }

if (chnum == 6 || chnum == 8)
    REORDER_SELF_SWAP_2(src_8, tmp, sample, chnum, s0, s1);

> @@ -342,6 +443,9 @@
>          else if (chnum==3) {
>              REORDER_SELF_SWAP_2(src_16,tmp,samples,3,s0,s1);
>          }
> +        else if (chnum==4) {
> +            REORDER_SELF_SWAP_2(src_16,tmp,samples,3,s0,s1);
> +        }
>          else {
>              REORDER_SELF_SWAP_2(src_16,tmp,samples,5,s0,s1);
>          }

Is that really supposed to be 3?
Anyway I suggest to do as suggested above for all others.
And I have to say there seems to be rather a lot of duplicated code,
reducing that would be welcome, too.

> Index: mplayer/libmpcodecs/ae_pcm.c
> ===================================================================
> --- mplayer/libmpcodecs/ae_pcm.c	(revision 29822)
> +++ mplayer/libmpcodecs/ae_pcm.c	(working copy)
> @@ -39,7 +39,8 @@
>  static int encode_pcm(audio_encoder_t *encoder, uint8_t *dest, void *src, int nsamples, int max_size)
>  {
>  	max_size = FFMIN(nsamples, max_size);
> -	if (encoder->params.channels == 6 || encoder->params.channels == 5) {
> +	if (encoder->params.channels == 5 || encoder->params.channels == 6 ||
> +		    encoder->params.channels == 8) {

What's the point of swapping the order?
Ah, to keep it ordered. I guess I personally would very slightly prefer
it to be done in two steps (also for ao_pcm), but do as you like.

> Index: mplayer/libao2/ao_alsa.c
> ===================================================================
> --- mplayer/libao2/ao_alsa.c	(revision 29822)
> +++ mplayer/libao2/ao_alsa.c	(working copy)
> @@ -457,6 +457,13 @@
>  	    device.str = "surround51";
>  	  mp_msg(MSGT_AO,MSGL_V,"alsa-init: device set to surround51\n");
>  	  break;
> +	case 8:
> +	  if (alsa_format == SND_PCM_FORMAT_FLOAT_LE)
> +	    device.str = "plug:surround71";
> +	  else
> +	    device.str = "surround71";
> +	  mp_msg(MSGT_AO,MSGL_V,"alsa-init: device set to surround71\n");
> +	  break;

Hm, is that plug: thing still necessary with current ALSA?
Anyway, that is for some later patch, as well as if not changing the
if/else to ?: