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

[Jason Tackaberry]

Thanks for the review, Reimar.

On Wed, 2009-11-04 at 09:57 +0100, Reimar Döffinger wrote:
> 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?

[...]

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

[...]

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

reorder_ch.c is filled with code just like that.  The very questionable
code in my patch merely conforms to existing precedence set in that
file.

Even the last suggestion of yours quoted above could apply to several
spots in that file.  My first assumption was that the original author
had done some benchmarking and split out the conditions for performance
reasons.  Even if that's true, it a) might not be relevant anymore, or
b) might not yield suitable gains to warrant the sacrifice in
readability.

I assume it's best to first add the functionality in a manner that's
consistent with the surrounding approach and style, and come along
after-the-fact and clean things up, which I'm prepared to do.


> 
> > @@ -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?

It seems surprising, but yes.  It's an optimization which works on two
adjacent channels at a time, and so the function is passed n_channels/2
(so for 6 channel content, that's 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.

As I said above, I'm happy to clean up this code afterward.  Unless you
think I should go ahead and add the new code with the saner style, and
come along afterwards to clean up the rest of it?

For purposes of adding new functionality, I'd frankly prefer to keep
things similar to the existing code and not try to get too clever,
especially because the existing approach is quite well baked.


> > +	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.

Well, the lack of ordering was one of your niggles on my last submission
of this patch. :)

I'd originally had it checking 6, 5, then 8, which was actually
intentionally ordered in what I thought would be most likely to least
likely to occur.


> Hm, is that plug: thing still necessary with current ALSA?

Hopefully Clemens' reply was satisfactory, because I don't know.  I
force device=hdmi so can't easily test that path.


> Anyway, that is for some later patch, as well as if not changing the
> if/else to ?:

I had the same thought when adding that stanza, but again it was a
matter of maintaining consistency with surrounding code.

I can submit another patch later to change all of the cases to use
the ?: operator.

So let me know if you're happy with the patch as it is, and then
cleanups afterward of reorder_ch.c and ao_alsa.c, or whether or you want
the new code to be restyled.

Thanks,
Jason.