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

[Benjamin Zores]

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

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

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

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

> > +#define SATURATE(x) (int16_t)(((x) == (int16_t)(x)) ? (x) : ((x) >> 31) ^ 0x7FFF);
>
> av_clip() or if the above is faster add a av_clip_int16 (in a seperate patch)

added an av_clip_int16() function instead.

>
> > +
> > +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 :-(

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

Ben

-- 
"My life, and by extension everyone else's is meaningless."
Bender, Futurama