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

[Michael Niedermayer]

On Fri, Mar 28, 2008 at 09:21:36PM +0000, Jai Menon wrote:
> On Thursday 27 March 2008 20:00:48 Michael Niedermayer wrote:
> > 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?

>  Thats because the encoding scheme requires adding an 8 bit signed value (from 
> the table) to fib_acc. So if we change the table size to int16_t , we could 
> do away with the shifts but the value can't be directly added to fib_acc 
> without clipping. 

Could you give me an example with numbers where a single value ending
up in out_data differs?


> the av_clip macro iirc uses comparisons. you could actually 
> try making the change and running it on the compressed sample Dennis posted 
> earlier on the list. Instead of the sound sample, you will get a highly 
> distorted waveform which sounds nothing like the original sample.
> 
> > + ? ?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
> where? is it buf_size-- ?

yes buf_size-- is a unneeded subtraction of 1


[...]
> +static int iff_probe(AVProbeData *p)
> +{
> +    const uint8_t *d = p->buf;
> +

> +    if ((AV_RL32(d) == ID_FORM) && (AV_RL32(d+8) == ID_8SVX))

the () are superflous


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

> +static int iff_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVStream *st;
> +    int8_t gotVhdr = 0;
> +    uint32_t chunk_id, data_size;
> +    uint16_t SamplesPerSec;
> +    svx8_compression_t Compression;
> +    uint8_t channels = 1;
> +    int padding, ret, done = 0;
> +
> +    url_fskip(pb, 8);
> +

> +    chunk_id = get_le32(pb);
> +    if(chunk_id != ID_8SVX)
> +        return AVERROR_INVALIDDATA;

this is redundant


> +
> +    while(!done && !url_feof(pb)) {
> +        chunk_id = get_le32(pb);
> +        data_size = get_be32(pb);
> +        padding = data_size & 1;
> +
> +        switch(chunk_id) {
> +        case ID_VHDR:
> +            url_fskip(pb, 12);
> +            SamplesPerSec = get_be16(pb);
> +            url_fskip(pb, 1);
> +            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:
> +            channels = (get_be32(pb) < 6) ? 1 : 2;
> +            break;
> +
> +        default:
> +            url_fseek(pb, data_size + padding, SEEK_CUR);
> +            break;
> +        }
> +    }
> +
> +    if(!gotVhdr)
> +       return AVERROR_INVALIDDATA;
> +

> +    st = av_new_stream(s, 0);

I would move that before the while() or in the ID_VHDR case and directly read
values into the context where possible.


[...]
> +/**
> + * 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 = buf_size;
> +
> +    if((*data_size >> 2) < buf_size)
> +        return -1;

good!

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

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20080328/55fde200/attachment.pgp>