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

[Michael Niedermayer]

On Thu, Mar 27, 2008 at 01:17:52AM +0000, Jai Menon wrote:
> On Wednesday 26 March 2008 16:35:10 Michael Niedermayer wrote:
> > On Wed, Mar 26, 2008 at 08:05:54PM +0000, Jai Menon wrote:
> > > +        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
> How exactly do i do that? i looked through a majority of decoders and none of 
> them seem to perform such a check.....but i may be missing something

see the data_size argument


> 
> > >
> > > +    uint32_t channels;
> >
> > could be a local var
> 
> I need this in the context because i use it to compute the audio frame count 
> in the read packet method

AVFormatContext.streams[0].codec.channels
Also what applies to channels also applies to other variables.


> 
> 
> Patch with other changes applied is attached.

[...]

> +/**
> + * 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;
> +    int16_t *out_data = data;
> +    int consumed;
> +    uint8_t d;
> +

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

unneeded


> +
> +    if(esc->first_frame) {

AVCodecContext.frame_number could be used here instead.


> +        esc->fib_acc = buf[1];
> +        buf_size -= 2;
> +        buf += 2;
> +        esc->first_frame = 0;
> +    }
> +
> +    consumed = buf_size;

> +    for(;buf_size>0;buf_size--) {
> +        d = *buf++;

uint8_t d = *buf++;


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

you can do this with one subtraction and 2 shifts less



> +    *data_size = consumed << 2;
> +
> +    return consumed;
> +}
> +
> +

> +/**
> + *
> + * initialize 8svx decoder
> + *
> + */

Inconsistant empty lines.


[...]
> +/** 8svx channel specifications */
> +#define LEFT    2
> +#define RIGHT   4
> +#define STEREO  6

These comments still are associated incorrectly


> +
> +#define PACKET_SIZE 1024
> +

> +/** 8svx compression */
> +typedef enum {COMP_NONE, COMP_FIB, COMP_EXP} svx8_compression_t;
> +
> +/** 8svx  vhdr */
> +typedef struct {
> +    uint16_t             SamplesPerSec;
> +    svx8_compression_t   Compression;
> +} SVX8_Vhdr;

These comments are useless as they dont say anything thats not already
in the name of the variable.


[...]
> +    /** 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:
> +            url_fskip(pb, 12);
> +            iff->vhdr.SamplesPerSec = get_be16(pb);
> +            url_fskip(pb, 1);
> +            iff->vhdr.Compression = get_byte(pb);
> +            url_fskip(pb, 4);
> +            gotVhdr = 1;
> +            break;
> +
> +        case ID_BODY:
> +            iff->body_size = data_size;
> +            done = 1;
> +            break;
> +
> +        case ID_CHAN:
> +            iff->channels = (get_be32(pb) < 6) ? 1 : 2;
> +            break;
> +
> +        default:
> +            url_fseek(pb, data_size + padding, SEEK_CUR);
> +            break;
> +        }
> +    }

this can end in an infnite loop


[...]
> +AVInputFormat iff_demuxer = {
> +    "IFF",
> +    "IFF format",
> +    sizeof(IffDemuxContext),
> +    iff_probe,
> +    iff_read_header,
> +    iff_read_packet,

> +    NULL,
> +};

The NULL is unneeded.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/d23f21a8/attachment.pgp>