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

[Kostya]

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 ?
> The frame size is variable (well all have the same size except for the
> latest one which obviously is smallest).

decoder returns decoded data size, just set frame_size in demuxer.
 
> > no, the demuxer cant just drop things because our decode does not
> > support it, it breaks stream copy ad players using their own decoders
> 
> Ok, noted.
> 
> > > 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.

What about trying to dump decoded data straight into some file instead of
normal passing it to decoder. Or not passing it at all - just decode it,
verify CRC and print out if CRC is correct (demac does this). If it is
wrong then match decoded coefficients with the ones produced by demac
otherwise it's decoder problem and it may be fixed 

[...] 
> > > +
> > > +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 :-(

it does this fixed number of times:
order <= 16: 16 times;
order <= 32: 32 times;
order > 32: (order >> 5) * 32 times or order & ~31 times. 
 
> > 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.
> 
> > 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.
> 
> [...]
> > duplicate
> 
> I've factorized a lot of things following your advices.
> 
> > > +/* Return 0 if x is zero, -1 if x is positive, 1 if x is negative */
> > > +#define SIGN(x) (x) ? (((x) > 0) ? -1 : 1) : 0
> >
> > FFSIGN
> 
> In fact, no ;-)
> Sign is inverted and it can be zero.
> 
> > what are these fuctions good for, why isnt init_filter called directly?
> > and they have to be static at least
> 
> You are perfectly right.
> 
> 
> > [...]
> > > +  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 ?

store it explicitly (AV_WB16,bytestream_put) and IIRC MAC header is almost the same, just its
position differs.
 
> Ben