[FFmpeg-devel] [PATCH] IFF demuxer and 8SVX decoder

[Michael Niedermayer]

On Wed, Mar 26, 2008 at 08:05:54PM +0000, Jai Menon wrote:
> Hi,
> 
> The new patch fixes stuff mentioned by Reimar, Diego and Benoit.
> Thanks for the comments.
> 
[...]

> +static int16_t fibonacci[16]   = { -34, -21, -13,  -8, -5, -3, -2, -1, 0, 1, 2, 3, 5,  8, 13, 21 };
> +static int16_t exponential[16] = {-128, -64, -32, -16, -8, -4, -2, -1, 0, 1, 2, 4, 8, 16, 32, 64 };

should be const static int8_t




> +
> +/**
> + *
> + * decode a frame
> + *
> + */
> +static int eightsvx_decode_frame(AVCodecContext *avctx, void *data, int *data_size, 
> +                                 const uint8_t *buf, int buf_size)
> +{
> +    EightSvxContext *esc = avctx->priv_data;
> +    const int8_t *in_data = buf;
> +    int16_t *out_data = data;

> +    uint32_t n, k;

id use unsigned int, but thats rather irrelevant


> +    uint8_t d;
> +
> +    if(!buf_size)
> +        return 0;
> +

> +    if(esc->first_frame) {
> +        esc->fib_acc = in_data[1];
> +        n = buf_size - (esc->first_frame << 1);

thats just buf_size - 2


> +        in_data+=2;
> +        esc->first_frame = 0;
> +    }
> +    else n = buf_size;


> +
> +    for(k=n;k>0;k--) {

a test of in_data against th end of the array would need 1 variable
and 1 -- less


> +        d = *in_data++;

as d is not use anywhere else it could also be declared here like:
uint8_t d = *in_data++;


> +        esc->fib_acc += esc->table[d & 0x0f];
> +        av_clip_uint8(esc->fib_acc);
> +        *out_data++ = esc->fib_acc << 8;
> +        esc->fib_acc += esc->table[d >> 4];
> +        av_clip_uint8(esc->fib_acc);
> +        *out_data++ = esc->fib_acc << 8;

you are not checking if the out_data buffer is large enough


[...]
> +/**
> + *
> + * initialize 8svx decoder
> + *
> + */
> +static av_cold int eightsvx_decode_init(AVCodecContext *avctx)
> +{
> +    EightSvxContext *esc = avctx->priv_data;
> +    esc->first_frame = 1;
> +
> +    switch(avctx->codec->id) {
> +        case CODEC_ID_8SVX_FIB:
> +          esc->table = fibonacci;
> +          break;
> +        case CODEC_ID_8SVX_EXP:
> +          esc->table = exponential;
> +          break;
> +        default:
> +          return -1;
> +    }

Iam tempted to suggest
if(CODEC_ID_8SVX_FIB)   esc->table = fibonacci;
else                    esc->table = exponential;

but this is really minor preferance of low relevance


> +    return 0;
> +}
> +
> +

> +AVCodec eightsvx_fib_decoder = {
> +  .name = "8svx fibonacci decoder",
> +  .type = CODEC_TYPE_AUDIO,
> +  .id   = CODEC_ID_8SVX_FIB,
> +  .priv_data_size = sizeof (EightSvxContext),
> +  .init   = eightsvx_decode_init,
> +  .decode = eightsvx_decode_frame,
> +};
> +
> +AVCodec eightsvx_exp_decoder = {
> +  .name = "8svx exponential decoder",
> +  .type = CODEC_TYPE_AUDIO,
> +  .id   = CODEC_ID_8SVX_EXP,
> +  .priv_data_size = sizeof (EightSvxContext),
> +  .init   = eightsvx_decode_init,
> +  .decode = eightsvx_decode_frame,
> +};

these could be vertically aligned


[...]
> +#define PACKET_SIZE 1024
> +
> +/** 8svx  vhdr */
> +typedef struct {

> +    uint32_t  OneShotHigh;
> +    uint32_t  RepeatHigh;
> +    uint32_t  SamplesCycle;

unused


> +    uint16_t  SamplesPerSec;


> +    uint8_t   Octaves;

unused


> +    uint8_t   Compression;


> +    uint32_t  Volume;

unused


> +} SVX8_Vhdr;
> +
> +/** 8svx compression */
> +enum {COMP_NONE, COMP_FIB, COMP_EXP};

give the enum a name and use it for "Compression"


> +
> +/** 8svx envelope structure definition (used for ATAK and RLSE chunks) */
> +struct {
> +    uint16_t duration;    /**< segment duration in milliseconds, > 0 */
> +    uint32_t dest;        /**< destination volume factor */
> +} SVX8_Env;
> +
> +
> +typedef struct {
> +    SVX8_Vhdr vhdr;

> +    uint32_t channels;

could be a local var


> +    uint32_t body_offset;

unused


> +    uint32_t body_size;
> +    uint32_t sent_bytes;

> +    int8_t   gotVhdr;

could be a local var


> +    uint8_t  audio_stream_index;

always 0


[...]
> +    const uint8_t *d;
> +
> +    d = p->buf;

declaration and initialiuation can be merged


> +    if (AV_RL32(d) == ID_FORM) {
> +        return AVPROBE_SCORE_MAX;
> +    }

superfous {}


[...]
> +    st->codec->codec_id = CODEC_ID_PCM_S8;
> +    if(iff->vhdr.Compression == COMP_FIB)
> +        st->codec->codec_id = CODEC_ID_8SVX_FIB;
> +    if(iff->vhdr.Compression == COMP_EXP)
> +        st->codec->codec_id = CODEC_ID_8SVX_EXP;
> +    st->codec->codec_tag = ID_8SVX;

seeing this now, i think iff->vhdr.Compression should be the codec_tag

also this could be s switch() but again this is very minor, use what you
like best


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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080326/d3ad8ad0/attachment.pgp>