[MPlayer-dev-eng] [PATCH] [TEST FUNC] Multi-channel reorder function

[Ulion]

Hello, Corey,

First of all, thank you for review and test my reorder function.

2007/11/17, Corey Hickey <bugfood-ml at fatooh.org>:
> Ulion wrote:
> > This patch is just to show the idea, could be buggy and ugly, maybe
> > disliked by somebodies, it's ok, I just want to fix the aac wrong
> > channel order bug, wellcome any idea to resolve the problem. Shall we
> > end this bug in the near future by either way?
>
> I hardly care who's patch gets applied, as long as one of them does...
>
> Here's a small review, to the extent I am able.
>
> > Index: libmpcodecs/ae_faac.c
> > ===================================================================
> > --- libmpcodecs/ae_faac.c       (revision 25008)
> > +++ libmpcodecs/ae_faac.c       (working copy)
> > @@ -98,6 +98,15 @@
>
> Missing #include "reorder_copy.h"

Yes, thank you. That probably because I didn't compile faac in my
mencoder so I missed it.

>
> > Index: libmpcodecs/ae_pcm.c
> > ===================================================================
> > --- libmpcodecs/ae_pcm.c      (revision 25008)
> > +++ libmpcodecs/ae_pcm.c      (working copy)
> > ...
> >       max_size = FFMIN(nsamples, max_size);
> > +     if (encoder->params.channels == 6 || encoder->params.channels == 5) {
> > +             max_size -= max_size % (encoder->params.channels * 2);
> > +             if (encoder->params.channels == 6)
> > +                     reorder_channel_copy(src, AF_CHANNEL_LAYOUT_MPLAYER_5CH_DEFAULT,
> > +                                                              dest, AF_CHANNEL_LAYOUT_WAVEEX_5CH_DEFAULT,
> > +                                                              max_size / 2, 2);
>
> Shouldn't these be AF_CHANNEL_LAYOUT_MPLAYER_6CH_DEFAULT and
> AF_CHANNEL_LAYOUT_WAVEEX_6CH_DEFAULT?

Yes, it should.

>
> > Index: reorder_copy.c
> > ===================================================================
> > --- reorder_copy.c    (revision 0)
> > +++ reorder_copy.c    (revision 0)
> > @@ -0,0 +1,872 @@
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <inttypes.h>
> > +#include <string.h>
> > +#include "libvo/fastmemcpy.h"
> > +
> > +#include "reorder_copy.h"
> > +
> > +#define REORDER_COPY_5(DEST,SRC,SAMPLES,S0,S1,S2,S3,S4) \
>
> It seems like all these reorder macros would be nicer as (possibly
> inline) functions. Do you have a particular reason for having them as
> macros?

Sorry because I didn't familar with inline functions, so I wrote this
macro codes. I don't know whether they (macro and inline) will have
same performance. Also, I didn't really mean to make this patch tobe
commited, just show the idea here and push the bug fix sooner. Indeed
in the irc, the discuss result seems choose a lightweight hack to fix
the reorder bug since all current patches are hackes.

>
> > +        printf("[reorder_copy] Unsupported number of bytes/sample: %d, please "\
> > +               "report this error on the MPlayer mailing list.\n",SAMPLESIZE);\
>
> This and other printf() calls ought to use mp_msg() instead.

Yes, it should.

>
>
> I knew this looked familiar...
> http://developer.apple.com/documentation/MusicAudio/Reference/CACoreAudioReference/CoreAudioTypes/CompositePage.html
>
> I don't know the license of the above document. Is it OK to include a
> derivative work in mplayer?

Yes, that's the reference I used. I don't think this will be commited
in, so just use it to show the idea.

>
>
> You're missing a reorder for ao_pcm.c.
>

That use a fwrite function, unless I change the input buffer by swap
functions, or write a reorder fwrite, neither is prefered by me, so I
didn't patch that file.


>
> I got mplayer to compile with this patch (after adding the missing
> #include), but it doesn't seem to be properly reordering the channels on
> playback of a 6-channel WAVE file.
>
> http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/Samples/Microsoft/6_Channel_ID.wav

That sounds bad, after a quick look, I didn't found the reason make it
fail. So what's the detail wrong channel status after this patch, or
before this patch?


>
> 6-channel AAC seems ok.
>
> -Corey
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>


-- 
Ulion