[FFmpeg-devel] [PATCH] SIFF demuxer and decoder

[Michael Niedermayer]

On Mon, Oct 15, 2007 at 02:59:47PM +0300, Kostya wrote:
> Here is demuxer and video decoder for SIFF format
> based on description sent to me by anonymous
> contributor.
[...]
> +static void vb_decode_palette(VBDecContext *c, uint8_t *buf)
> +{
> +    int start, size, i;
> +    int r, g, b;
> +
> +    start = *buf++;
> +    size = ((*buf++) - 1) & 0xFF;
> +    for(i = start; i <= start + size; i++){

> +        r = *buf++;
> +        g = *buf++;
> +        b = *buf++;
> +        c->pal[i] = (r << 16) | (g << 8) | b;

bytestream_get_be24()


[...]

> +    for(i = 0; i < avctx->height; i++){
> +        memcpy(outptr, srcptr, avctx->width);
> +        srcptr += avctx->width;
> +        outptr += c->pic.linesize[0];
> +    }
> +
> +    memcpy(c->prev_frame, c->frame, avctx->width * avctx->height);

no, please remove all frame memcpy()


[...]
> +typedef struct SIFFContext{
> +    int frames;
> +    int cur_frame;
> +    int rate;
> +    int block_align;

> +    int has_video;

isnt that always 1 ?


[...]
> +static int siff_parse_vbv1(AVFormatContext *s, SIFFContext *c, ByteIOContext *pb)
> +{
> +    AVStream *st, *ast;
> +    uint32_t tag, size;
> +    int width, height, bits;
> +
> +    tag = get_le32(pb);
> +    if (tag != TAG_VBHD){
> +        av_log(s, AV_LOG_ERROR, "Header chunk is missing\n");
> +        return -1;
> +    }
> +    size = get_be32(pb);
> +    if(size != 32){
> +        av_log(s, AV_LOG_ERROR, "Header chunk size is incorrect\n");
> +        return -1;    
> +    }
> +    if(get_le16(pb) != 1){
> +        av_log(s, AV_LOG_ERROR, "Incorrect header version\n");
> +        return -1;    
> +    }
> +    width = get_le16(pb);
> +    height = get_le16(pb);
> +    url_fskip(pb, 4);
> +    c->frames = get_le16(pb);
> +    if(!c->frames){
> +        av_log(s, AV_LOG_ERROR, "File contains no frames ???\n");
> +        return -1;
> +    }
> +    bits = get_le16(pb);
> +    c->rate = get_le16(pb);
> +    c->block_align = c->rate * (bits >> 3);
> +
> +    url_fskip(pb, 16); //zeroes
> +
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +        return -1;
> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
> +    st->codec->codec_id   = CODEC_ID_VB;
> +    st->codec->codec_tag  = MKTAG('V', 'B', 'V', '1');
> +    st->codec->width      = width;
> +    st->codec->height     = height;
> +    st->codec->pix_fmt    = PIX_FMT_PAL8;
> +    av_set_pts_info(st, 33, 1, 12);
> +
> +    c->has_audio = !!c->rate;
> +    if (c->has_audio){
> +        ast = av_new_stream(s, 0);
> +        if (!ast)
> +            return -1;
> +        ast->codec->codec_type      = CODEC_TYPE_AUDIO;
> +        ast->codec->codec_id        = CODEC_ID_PCM_U8;
> +        ast->codec->channels        = 1;
> +        ast->codec->bits_per_sample = bits;
> +        ast->codec->sample_rate     = c->rate;
> +        ast->codec->frame_size      = c->block_align;
> +        av_set_pts_info(ast, 33, 1, c->rate);
> +    }
> +    
> +    c->cur_frame = 0;
> +    c->has_video = 1;
> +    return 0;
> +}
> +
> +static int siff_parse_soun(AVFormatContext *s, SIFFContext *c, ByteIOContext *pb)
> +{
> +    AVStream *st;
> +    uint32_t tag, size;
> +    int bits;
> +
> +    tag = get_le32(pb);
> +    if (tag != TAG_SHDR){
> +        av_log(s, AV_LOG_ERROR, "Header chunk is missing\n");
> +        return -1;
> +    }
> +    size = get_be32(pb);
> +    if(size != 8){
> +        av_log(s, AV_LOG_ERROR, "Header chunk size is incorrect\n");
> +        return -1;    
> +    }
> +    url_fskip(pb, 4); //unknown value
> +    c->rate = get_le16(pb);
> +    bits = get_le16(pb);
> +    c->block_align = c->rate * (bits >> 3);
> +
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +        return -1;
> +    st->codec->codec_type      = CODEC_TYPE_AUDIO;
> +    st->codec->codec_id        = CODEC_ID_PCM_U8;
> +    st->codec->channels        = 1;
> +    st->codec->bits_per_sample = bits;
> +    st->codec->sample_rate     = c->rate;
> +    st->codec->frame_size      = c->block_align;
> +    av_set_pts_info(st, 33, 1, c->rate);
> +
> +    return 0;
> +}

this looks quite duplicated


[...]

> +            if(av_get_packet(&s->pb, pkt, size) != size)
> +                return AVERROR(EIO);

memleak


> +            if(c->has_audio){
> +                c->blkdata = av_realloc(c->blkdata, size);
> +                memcpy(c->blkdata, pkt->data, pkt->size);
> +            }
> +            flags = pkt->data[0];
> +            c->next_audio = !!(flags & VB_HAS_AUDIO);
> +        }else{
> +            //parse block
> +            flags = c->blkdata[0];
> +            snddata = c->blkdata + 2 + ((flags & VB_HAS_GMC) ? 4 : 0);
> +            size = AV_RL32(snddata) - 4;
> +            if (av_new_packet(pkt, size) < 0)
> +                return AVERROR(ENOMEM);
> +            memcpy(pkt->data, snddata + 4, size);
> +        }

this looks wrong, it passes the audio to the video decoder
it also does a lot of unneeded memcpy()
please clean this up
video -> video stream
audio -> audio stream
and no memcpy


[...]

> +        size = av_get_packet(&s->pb, pkt, c->block_align);
> +        if(size <= 0)
> +            return AVERROR(EIO);
> +        pkt->stream_index = 0;

> +        pkt->size = size;

unneeded


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20071015/60dd3656/attachment.pgp>