[FFmpeg-devel] [PATCH/DRAFT] MonkeyAudio Demuxer/Decoder

[Michael Niedermayer]

Hi

On Mon, Jun 25, 2007 at 09:48:43AM +0200, Benjamin Zores wrote:
> On 6/20/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> > Hi
> 
> Thank you Michael for your kind review.
> I didn't expected that much as the demuxer/decoder is still in
> development process (i.e. not fully working). As I've said, big parts
> of the code where copy/pasted from libdemac and not meant to stay the
> way they are.
> 
> > the audio decoder should set AVCodecContext.frame_size the application
> > should allocate enough space according to it (i doubt ffmpeg.c does ...)
> > then the decoder should check that the allocated size is in fact enough
> > this is an important safty check
> 
> When does it need to be done ?
> At codec init time or at each frame being decoded ?

well i would set it as soon as its known


[...]
> > > Then, I'll be glad if someone could review this patch (quickly, no
> > > need to be exhaustive) to help me find out what can cause the decoder
> > > to crash after a few samples.
> 
> I've applied some of the points you've reviewed but i'm still stuck.
> All the frames are well demuxed and the first one is well decoded one
> but all others are just muted (I'll send an updated patch soon enough)
> for undetermined reason.
> I'll need some more help there to see if i'm not doing something
> bad/unexpected with packet demuxing/decoding.

have you tried to transcode to .wav and play that? have you tried to
stream copy the thing into avi or another container and play that?


> 
> > > +struct rangecoder_t
> > > +{
> > > +  uint32_t low;        /* low end of interval */
> > > +  uint32_t range;      /* length of interval */
> > > +  uint32_t help;       /* bytes_to_follow resp. intermediate value */
> > > +  unsigned int buffer; /* buffer for input/output */
> > > +};
> >
> > this belongs into a seperate file and patch
> 
> This is the rangecoder implementation from libdemac and I'd like to
> avoid using it in final patch (ffmpeg already has one so i guess it'd
> be useless).
> Would you be able to help me switch from this implementation to ffmpeg's one ?
> I'm not very confident with this part of code.

its kinda unlikely that 2 independantly developed range coders would be
compatible ...


> 
> > > +typedef struct {
> > > +  int16_t fileversion;
> > > +  uint16_t compression;
> > > +  uint16_t flags;
> > > +} APECodecInfo;
> > > +
> > > +typedef struct APEContext {
> > > +  AVCodecContext *avctx;
> > > +  int channels;
> >
> > duplicate of the corresponding variable in AVCodecContext
> 
> where ?

AVCodecContext.channels


[...]
> >
> > > +
> > > +static inline void vector_add (int16_t* v1, int16_t* v2, int order)
> > > +{
> > > +  int or = 1;
> > > +  if (order > 32)
> > > +    or = (order >> 5);
> > > +
> > > +  while (or--)
> > > +  {
> > > +    int i;
> > > +
> > > +    for (i = 0; i < 16; i++)
> > > +      *v1++ += *v2++;
> > > +
> > > +    if (order > 16)
> > > +      for (i = 0; i < 16; i++)
> > > +        *v1++ += *v2++;
> > > +  }
> > > +}
> 
> > and this whole function is just
> > for (i = 0; i < order; i++)
> >     *v1++ += *v2++;
> 
> I've tested it and it's not :-(

then find out why ...


> 
> > if you do optimize it, do it properly, that is by using 32bit ints or
> > mmx
> 
> This is unknown to me unfortunately.

> So right now (until everything works at least), it'll stay this way.

thats fine, iam not saying you should do any of the changes now iam just
saying they should be done before this is commited



> 
> > its ctx->bytebuffer[ctx->bytebufferoffset ^ 3];
> > instead of this mess and besides why not use the bitreader as this seems
> > just used in init code so speed doesnt matter
> 
> Hum, read_byte(), skip_byte() are used by decoding process to (by the
> rangecoder), not just at init stage.

maybe you should bswap the buffer then? or use proper big/little endian
32bit reading? 


[...]
> > [...]
> > > +  data = av_malloc (sizeof (APECodecInfo));
> > > +  data->fileversion = ape->fileversion;
> > > +  data->compression = ape->compressiontype;
> > > +  data->flags = ape->formatflags;
> > > +  st->codec->extradata = (void *) data;
> >
> > this is not ok, extradata must have a unique byte based description
> > storing structs in it will break things
> 
> How to nicely pass data between demuxer and decoder then ?

by deciding on a byte based extradata, which ideally should be just
what is stored in the file with no reshufflings
keep in mind that stream copy will take all packets and the extradata
and store it in a different container which then on another system
(big endian maybe) would be demuxed, and as its a different container
your demuxer wouldnt be involved ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070625/1cadf2d9/attachment.pgp>