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

[Michael Niedermayer]

On Thu, Mar 27, 2008 at 11:48:10PM +0000, Jai Menon wrote:
> On Thursday 27 March 2008 15:23:54 Michael Niedermayer wrote:
> > On Thu, Mar 27, 2008 at 08:44:54PM +0000, Jai Menon wrote:
> > > On Wednesday 26 March 2008 21:12:26 Michael Niedermayer wrote:
> > > > 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
> > >
> > > I still don't know how i can eliminate the two shifts?
> >
> > change the table ...
> 
> I could change it to int16_t, and remove the 2 shifts.....but then i would 
> need to clip twice before adding the table value to the accumulator......in 
> which case imho we should stick to 2 shifts. 

Why would you need to clip?


> 
> Other changes  were made and all local tests were successfull. patch 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;
> +

> +    if(*data_size/2 < buf_size)
> +      return -1;

this is not correct


> +
> +    if(avctx->frame_number == 0) {
> +        esc->fib_acc = buf[1];
> +        buf_size -= 2;
> +        buf += 2;
> +    }
> +
> +    consumed = buf_size;

> +    for(;buf_size>0;buf_size--) {
> +        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;
> +    }

this still does a unneeded subtraction besides the shifts


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

> +    return consumed;

this is not always correct


[...]
> +typedef struct {

> +    uint16_t             SamplesPerSec;
> +    svx8_compression_t   Compression;

These are as well redundant


[...]
> +static int iff_probe(AVProbeData *p)
> +{
> +    const uint8_t *d = p->buf;
> +
> +    if (AV_RL32(d) == ID_FORM)
> +        return AVPROBE_SCORE_MAX;
> +    return 0;
> +}

this is insufficient, it also applies to wc3 files
see libavformat/wc3movie.c


[...]
> +static int iff_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;

> +    int ret = 0;

the initialization is redundant


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

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/20080327/7760de41/attachment.pgp>