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

[Benoit Fouet]

Jai Menon wrote:
> Hi,
>
> The new patch fixes stuff mentioned by Reimar, Diego and Benoit.
> Thanks for the comments.
>
>   
> Index: 8svx.c
> ===================================================================
> --- 8svx.c	(revision 0)
> +++ 8svx.c	(revision 0)
> @@ -0,0 +1,127 @@
> +/*
> + * 8SVX Audio Decoder
> + * Copyright (C) 2008 Jaikrishnan Menon
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file 8svx.c
> + * 8svx audio decoder
> + * @author Jaikrishnan Menon
> + * supports: fibonacci delta encoding
> + *         : exponential encoding
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include "avcodec.h"
> +
> +/**
> + * decoder context
> + */
> +typedef struct EightSvxContext {
> +        int16_t fib_acc;
> +        int first_frame;
> +        int16_t *table;
> +} EightSvxContext;
> +
> +
> +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 };
> +
>   

as mentionned by Reimar, this fits to 8 bits too

> +/**
> + *
> + * decode a frame
> + *
> + */
>   

as mentionned by Diego, this fits in three lines instead of 5

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

you don't need in_data, you can use buf

> +    int16_t *out_data = data;
> +    uint32_t n, k;
> +    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);
>   

you know esc->first_frame is 1 here
and you don't need n, you can use buf_size

> +        in_data+=2;
> +        esc->first_frame = 0;
> +    }
> +    else n = buf_size;
> +
> +    for(k=n;k>0;k--) {
> +        d = *in_data++;
> +        esc->fib_acc += esc->table[d & 0x0f];
> +        av_clip_uint8(esc->fib_acc);
>   

esc->fib_acc = av_clip_uint8(esc->fib_acc);
you are not clipping here

> +        *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;
> +    }
> +    *data_size = n << 2;
> +
> +    return buf_size;
> +}
> +
> +
>   
> Index: iff.c
> ===================================================================
> --- iff.c	(revision 0)
> +++ iff.c	(revision 0)
>
> [...]
>
> +static int iff_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVStream *st;
> +
> +    uint32_t chunk_id ,data_size;
> +    int padding, ret, done = 0;
> +
> +    iff->channels = 1; /**< Set default to mono */
> +    /** Skip FORM and FORM chunk size */
> +    url_fskip(pb, 8);
> +
> +    chunk_id = get_le32(pb);
> +    if(chunk_id != ID_8SVX)
> +        return AVERROR_INVALIDDATA;
> +
> +    /** start reading chunks */
> +    while(!done)
> +    {
> +        chunk_id = get_le32(pb);
> +        /** read data size */
> +        data_size = get_be32(pb);
> +        padding = data_size & 1;
> +
> +        switch(chunk_id)
> +        {
> +            case ID_VHDR:
>   

case shouldn't be indented

> +                iff->vhdr.OneShotHigh = get_be32(pb);
> +                iff->vhdr.RepeatHigh = get_be32(pb);
> +                iff->vhdr.SamplesCycle = get_be32(pb);
> +                iff->vhdr.SamplesPerSec = get_be16(pb);
> +                iff->vhdr.Octaves = get_byte(pb);
> +                iff->vhdr.Compression = get_byte(pb);
> +                iff->vhdr.Volume = get_be32(pb);
> +                iff->gotVhdr = 1;
>   

as it is only used here, no need to store it in the context i guess

-- 
Benoit Fouet
Purple Labs S.A.
www.purplelabs.com