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

Michael Niedermayer michaelni
Wed Jun 20 12:41:18 CEST 2007


Hi

On Tue, Jun 19, 2007 at 11:50:06PM +0200, Benjamin Zores wrote:
> Hi there,
> 
> As recently stated on Kostya's blog, a light GPL (plain C) decoder has
> been written by Dave Chapman for Rockbox project.
> I've started porting it to FFMpeg to have a native demuxer/decoder for
> this audio format.
> 
> Good news is that demuxer seems _almost_ okay by now.
> Not so bad news is that decoder seems to work a bit: it decodes a few
> samples (too slow however) but then crashes before the end of the
> first frame, and here's where I need a few help.
> 
> Well, first thing that bugs me is that MonkeyAudio uses really HUUUUGE 
> frames.
> Though, I redefined the following in avcodec.h:
> 
> 
> -#define AVCODEC_MAX_AUDIO_FRAME_SIZE 192000 // 1 second of 48khz 32bit 
> audio
> +#define AVCODEC_MAX_AUDIO_FRAME_SIZE 384000 // 1 second of 96khz 32bit 
> audio
> 
> In order to at least have enough space in buffer to decode all samples
> >from one frame. Is this the best way to do (I really doubt so) or what
> else can be done ?

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


> 
> By the way, FFMpeg only supports 16bits audio samples right ? So if
> anything else in found by demuxer (8/24 bits, I can just drop the
> file, right ?)

no, the demuxer cant just drop things because our decode does not
support it, it breaks stream copy ad players using their own decoders


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

ok quick review below ...



> 
> For the record, in its current form, the decoder's implementation
> nearly is a copy/paste from original libdemac.
> That's why it uses a new RangeCoder implementation, makes uses of some
> already existing defines/macros and so on ... it'll be changed later
> on, but first step is to be able to decode with original libdemac
> code.
> 
> For those who may wanna test, you can find a MA sample here:
> http://geexbox.org/~ben/test.ape
> 
> Thx to whoever can help me out ;-)
[...]

> +  /* We store all the filter delays in a single buffer */
> +  int16_t* historybuffer;  /* ORDER*2 + HISTORY_SIZE entries */
> +  

trailing whitespace


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


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


[...]
> +  /* from demuxer */
> +  int16_t fileversion;
> +  uint16_t compression_level;
> +  uint16_t flags;

duplicate of APECodecInfo


[...]
> +#define FP_HALF(y)  (1 << (y - 1))   /* 0.5 in fixed-point format. */

used once -> unneeded obfuscation


> +#define FP_TO_INT(x,y) ((x + FP_HALF(y)) >> y);  /* round(x) */
> +

> +#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)


> +
> +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++;
> +  }
> +}

indention in ffmpeg should be 4 spaces

and this whole function is just
for (i = 0; i < order; i++)
    *v1++ += *v2++;

if you do optimize it, do it properly, that is by using 32bit ints or
mmx



> +
> +static inline void vector_sub(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++;
> +  }
> +}

duplicate of the above with - instead of +


> +
> +static inline int32_t scalarproduct(int16_t* v1, int16_t* v2, int order)
> +{
> +  int res = 0;
> +  int or = 1;
> +  
> +  if (order > 16)
> +    or = (order >> 4);
> +  
> +  while (or--)
> +  {
> +    int i;
> +
> +    for (i = 0; i < 16; i++)
> +      res += *v1++ * *v2++;
> +  }
> +
> +  return res;
> +}

same nonsense bloatup here


> +
> +/* 32-bit vector math functions */
> +
> +#define scalarproduct4_rev32(x,y) ((x[0]  * y[0]) + (x[1] * y[-1]) + \
> +                                   (x[2] * y[-2]) + (x[3] * y[-3]))
> +
> +#define scalarproduct5_rev32(x,y) ((x[0]  * y[0]) + (x[1] * y[-1]) + \
> +                                   (x[2] * y[-2]) + (x[3] * y[-3]) + \
> +                                   (x[4] * y[-4]))
> +
> +#define vector_sub4_rev32(x, y) { x[0] -= y[0]; \
> +                                  x[1] -= y[-1]; \
> +                                  x[2] -= y[-2]; \
> +                                  x[3] -= y[-3]; }
> +
> +#define vector_sub5_rev32(x, y) { x[0] -= y[0]; \
> +                                  x[1] -= y[-1]; \
> +                                  x[2] -= y[-2]; \
> +                                  x[3] -= y[-3]; \
> +                                  x[4] -= y[-4]; }
> +
> +#define vector_add4_rev32(x, y) { x[0] += y[0]; \
> +                                  x[1] += y[-1]; \
> +                                  x[2] += y[-2]; \
> +                                  x[3] += y[-3]; }
> +
> +#define vector_add5_rev32(x, y) { x[0] += y[0]; \
> +                                  x[1] += y[-1]; \
> +                                  x[2] += y[-2]; \
> +                                  x[3] += y[-3]; \
> +                                  x[4] += y[-4]; }

this can be factorized or even better be written as a normal function

[..]
> +static inline void skip_byte(APEContext *ctx)
> +{
> +  if (ctx->bytebufferoffset)
> +    ctx->bytebufferoffset--;
> +  else
> +  {
> +    ctx->bytebufferoffset = 3;
> +    ctx->bytebuffer += 4;
> +  }
> +}
> +
> +static inline int read_byte(APEContext *ctx)
> +{
> +  int ch = ctx->bytebuffer[ctx->bytebufferoffset];
> +    
> +  skip_byte (ctx);
> +
> +  return ch;
> +}

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


[...]
> +/* counts_diff_3970[i] = counts_3970[i+1] - counts_3970[i] */
> +static const int counts_diff_3970[64] =
> +{
> +    14824,13400,11124,8507,6139,4177,2755,1756,
> +    1104,677,415,248,150,89,54,31,
> +    19,11,7,4,2,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1
> +};
[...]
> +/* counts_diff_3980[i] = counts_3980[i+1] - counts_3980[i] */
> +
> +static const int counts_diff_3980[64] =
> +{
> +    19578,16582,12257,7906,4576,2366,1170,536,
> +    261,119,65,31,19,10,6,3,
> +    3,2,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1,
> +    1,1,1,1,1,1,1,1
> +};

uint16_t


[...]
> +static inline int range_get_symbol_3980(APEContext *ctx)
> +{
> +    int symbol, cf;
> +
> +    cf = range_decode_culshift(ctx,16);
> +
> +    /* figure out the symbol inefficiently; a binary search would be much better */
> +    for (symbol = 0; counts_3980[symbol+1] <= cf; symbol++);
> +
> +    range_decode_update(ctx,counts_diff_3980[symbol],counts_3980[symbol]);
> +
> +    return symbol;
> +}
> +
> +static inline int range_get_symbol_3970(APEContext *ctx)
> +{
> +    int symbol, cf;
> +
> +    cf = range_decode_culshift(ctx,16);
> +
> +    /* figure out the symbol inefficiently; a binary search would be much better */
> +    for (symbol = 0; counts_3970[symbol+1] <= cf; symbol++);
> +
> +    range_decode_update(ctx,counts_diff_3970[symbol],counts_3970[symbol]);
> +
> +    return symbol;
> +}

duplicated


> +
> +static inline void update_rice(struct rice_t* rice, int x)
> +{
> +  rice->ksum += ((x + 1) / 2) - ((rice->ksum + 16) >> 5);
> +
> +  if (rice->k == 0) {
> +    rice->k = 1;
> +  } else if (rice->ksum < ((uint32_t)1 << (rice->k + 4))) {
> +    rice->k--;
> +  } else if (rice->ksum >= ((uint32_t)1 << (rice->k + 5))) {
> +    rice->k++;
> +  }
> +}

useless cast


[...]
> +  x = base + (overflow * pivot);
> +  update_rice(rice, x);
> +
> +  /* Convert to signed */
> +  if (x & 1)
> +    return (x >> 1) + 1;
> +  else
> +    return -(x >> 1);
> +}
[...]
> +    x += (overflow << tmpk);
> +
> +    update_rice(rice, x);
> +
> +    /* Convert to signed */
> +    if (x & 1)
> +        return (x >> 1) + 1;
> +    else
> +        return -(x >> 1);
> +}

duplicate


[...]
> +  *bytesconsumed = (intptr_t) ctx->bytebuffer - (intptr_t) inbuffer;

IIRC intptr_t is not mandatory in the c spec


[...]
> +  if (ctx->frameflags & APE_FRAMECODE_STEREO_SILENCE) {
> +    /* We are pure silence, just memset the output buffer. */
> +    memset(decoded0, 0, blockstodecode * sizeof(int32_t));
> +    memset(decoded1, 0, blockstodecode * sizeof(int32_t));
> +  } else {
> +    if (ctx->fileversion > 3970) {
> +      while (blockstodecode--) {
> +        *(decoded0++) = entropy_decode3980 (ctx,&(ctx->riceY));
> +        if (decoded1 != NULL)

the != NULL is unneeded
and decoded[0] / [1] seems cleaner to me


[...]
> +static const int32_t initial_coeffs[4] = {
> +  360, 317, -109, 98
> +};

why int32_t ?


[...]
> +/* 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


[...]
> +                  
> +void init_filter_16_11 (APEContext *ctx, int16_t* buf)
> +{
> +  init_filter (ctx, buf, 16);
> +}
> +
> +void init_filter_64_11 (APEContext *ctx, int16_t* buf)
> +{
> +  init_filter (ctx, buf, 64);
> +}
> +
> +void init_filter_32_10 (APEContext *ctx, int16_t* buf)
> +{
> +  init_filter (ctx, buf, 32);
> +}
> +
> +void init_filter_256_13 (APEContext *ctx, int16_t* buf)
> +{
> +  init_filter (ctx, buf, 256);
> +}
> +
> +void init_filter_1280_15 (APEContext *ctx, int16_t* buf)
> +{
> +  init_filter (ctx, buf, 1280);
> +}

what are these fuctions good for, why isnt init_filter called directly?
and they have to be static at least


[...]
> +int apply_filter_16_11 (APEContext *ctx, int fileversion, int32_t* decoded0, int32_t* decoded1, int count)
> +{
> +  return apply_filter (ctx, fileversion, decoded0, decoded1, count, 16, 11);
> +}
> +
> +int apply_filter_64_11(APEContext *ctx, int fileversion, int32_t* decoded0, int32_t* decoded1, int count)
> +{
> +  return apply_filter (ctx, fileversion, decoded0, decoded1, count, 64, 11);
> +}
> +
> +int apply_filter_32_10(APEContext *ctx, int fileversion, int32_t* decoded0, int32_t* decoded1, int count)
> +{
> +  return apply_filter (ctx, fileversion, decoded0, decoded1, count, 32, 10);
> +}
> +
> +int apply_filter_256_13(APEContext *ctx, int fileversion, int32_t* decoded0, int32_t* decoded1, int count)
> +{
> +  return apply_filter (ctx, fileversion, decoded0, decoded1, count, 256, 13);
> +}
> +
> +int apply_filter_1280_15(APEContext *ctx, int fileversion, int32_t* decoded0, int32_t* decoded1, int count)
> +{
> +  return apply_filter (ctx, fileversion, decoded0, decoded1, count, 1280, 15);
> +}

more unneeded wraper functions


[...]
> +#ifndef MIN
> +#define MIN(a,b) ((a) < (b) ? (a) : (b))
> +#endif

FFMIN


[...]
> Index: libavformat/ape.c
> ===================================================================
> --- libavformat/ape.c	(revision 0)
> +++ libavformat/ape.c	(revision 0)
[...]
> +#include <stdio.h>

no


[...]
> +/* struct predictor_t */
> +/* { */
> +/*     /\* Adaption co-efficients *\/ */
> +/*     int32_t coeffsA[4]; */
> +/*     int32_t coeffsB[5]; */
> +
> +/*     /\* Filter histories *\/ */
> +/*     int32_t historybuffer[HISTORY_SIZE + PREDICTOR_ORDER * 4]; */
> +/*     int32_t* delayA; */
> +/*     int32_t* delayB; */
> +/*     int32_t* adaptcoeffsA; */
> +/*     int32_t* adaptcoeffsB; */
> +
> +/*     int32_t lastA; */
> +
> +/*     int32_t filterA; */
> +/*     int32_t filterB; */
> +/* }; */

this does not belong in here


[...]
> +static int ape_probe(AVProbeData *p)
> +{

> +  printf ("%s:%d\n", __FUNCTION__, __LINE__);

printf does not belong in codecs, demuxers, ...
breaks output to stdout amongth others


> +
> +  /* check file header */
> +  if (p->buf_size <= 32)
> +    return 0;

unneeded


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

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

Democracy is the form of government in which you can choose your dictator
-------------- 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/20070620/d47370dd/attachment.pgp>



More information about the ffmpeg-devel mailing list