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

[Michael Niedermayer]

On Sat, Mar 29, 2008 at 11:13:18AM +0000, Jai Menon wrote:
> On Friday 28 March 2008 18:29:35 Michael Niedermayer wrote:
> > 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?
> 
> I'll phrase this in a different way. After the decoding, the fib_acc variable 
> stores the signed pcm data. The shifts are required because the decoder's 
> output is supposed to be 16 bit. If i convert the table to use 16 bit data 
> instead and also make fib_acc an int16_t, i'll need to extract just the 
> lsbyte since the msbyte is not part of the sample information.
> Otherwise, i could use a couple of casts to ensure that the msbyte remains 
> 0x00, but that wouldn't be too clean.

Currently the code does practically:

while(not end){
    acc += constant_table[*x++];
    *out++= acc<<8;
}

The code can be changed so it does not need the "<<8" while keeping the output
the same 16bit data.


> 
> > > 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
> I do this check in a different way now.....is this what you intended?

yes, its one operation less (at the C level at least).


[...]
> +#include <stdio.h>
> +#include <stdlib.h>

are all these includes needed?


[...]
> +/**
> + * 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;

> +    uint8_t *buf_end = buf + buf_size;

const uint8_t *


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

slightly more readable IMHO: (really minor ...)
if (   AV_RL32(d  ) == ID_FORM
    && AV_RL32(d+8) == ID_8SVX)


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

redundant check sample_rate instead


> +    uint32_t chunk_id, data_size;

> +    svx8_compression_t Compression;

redundant, see codec_tag


> +    int padding, ret, done = 0;
> +
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +      return AVERROR(ENOMEM);
> +
> +    st->codec->channels = 1;
> +    url_fskip(pb, 8);
> +

> +    chunk_id = get_le32(pb);

unused


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

> +    if(iff->sent_bytes <= iff->body_size) {
> +        ret = av_get_packet(pb, pkt, PACKET_SIZE);
> +        iff->sent_bytes += PACKET_SIZE;
> +    }
> +    else
> +        return AVERROR(EIO);

if(iff->sent_bytes > iff->body_size)
    return AVERROR(EIO);

ret = av_get_packet(pb, pkt, PACKET_SIZE);
iff->sent_bytes += PACKET_SIZE;


> +
> +    pkt->stream_index = 0;
> +    pkt->pts = iff->audio_frame_count;

> +    iff->audio_frame_count += (ret / s->streams[0]->codec->channels);

superflous ()


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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20080329/faba229d/attachment.pgp>