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

[Reimar Döffinger]

On Wed, Nov 04, 2009 at 02:56:16PM -0500, Jason Tackaberry wrote:
> [...]
> 
> > 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.

I didn't look at the surrounding code I admit.

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

Well, I guess it's possible there are performance considerations,
benchmarking and/or looking at the generated code can't hurt.

> > > @@ -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).

Well, make it's clearer with the surrounding code, but the check is for
chnum == 4.
I think you very much should add a comment there explaining what is
going on there.

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

Whichever you prefer.

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

Yes I can imagine that, it's just that the reorder change had me
wondering why you replaced the == 6 check with == 5 till I realized
you had just swapped it.

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

Yes, it's ok by me, I'd appreciate it if someone had a look at those
strange things.
And if you find out any good reasons why the code is as it is please add
comments explaining it! And no need to send patches for that, just
apply it, I really want to encourage people writing comments for
non-obvious hacks and if reviewing on -cvslog helps I don't mind that at
least for comments.