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

[Michael Niedermayer]

On Wed, Oct 17, 2007 at 02:54:20PM +0300, Kostya wrote:
> On Mon, Oct 15, 2007 at 11:42:31PM +0200, Michael Niedermayer wrote:
> > 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)
> [...]
> > 
> > bytestream_get_be24()
> 
> Now decoder uses bytestream reader (except video decoding) 
>  
> > [...]
> > 
> > > +    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()
> 
> frame swapping now done via pointer swapping but I will save those
> internal buffer as motion compensation expects no gaps between lines 
>  
> [...]
> > > +    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
> 
> moved to the separate function
>  
> 
> [...]
> > 
> > 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
> 
> reworked 
>  
> > [...]
> > 
> > > +        size = av_get_packet(&s->pb, pkt, c->block_align);
> > > +        if(size <= 0)
> > > +            return AVERROR(EIO);
> > > +        pkt->stream_index = 0;
> > 
> > > +        pkt->size = size;
> > 
> > unneeded
> 
> dropped 
>  
> > [...]
> > -- 
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > 
> > No snowflake in an avalanche ever feels responsible. -- Voltaire
> 
> 
> 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at mplayerhq.hu
> > http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

[...]

> +static void vb_decode_palette(VBDecContext *c)
> +{
> +    int start, size, i;
> +
> +    start = bytestream_get_byte(&c->stream);
> +    size = (bytestream_get_byte(&c->stream) - 1) & 0xFF;

> +    for(i = start; i <= start + size; i++)
> +        c->pal[i] = bytestream_get_be24(&c->stream);

exploitable, please be more carefull when writing data from a file somewhere!

if(start+size > 255)
    return -1;
for(i = 0; i <= size; i++)
    c->pal[start+i] = bytestream_get_be24(&c->stream);


also the bytestream_get_byte are inconsistant with the *buf++ in some other
part of the file (i prefer *buf++ but thats just me, its your code ...)


[...]
> +    c->pic.reference = 1;
> +    if(avctx->get_buffer(avctx, &c->pic) < 0){
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    memcpy(c->pic.data[1], c->pal, AVPALETTE_SIZE);
> +    c->pic.palette_has_changed = flags & VB_HAS_PALETTE;
> +
> +    outptr = c->pic.data[0];
> +    srcptr = c->frame;
> +    
> +    for(i = 0; i < avctx->height; i++){
> +        memcpy(outptr, srcptr, avctx->width);
> +        srcptr += avctx->width;
> +        outptr += c->pic.linesize[0];
> +    }

just return the frame you use internally and dont call get/release_buffer()


[...]
> +    av_set_pts_info(ast, 33, 1, c->rate);
                            ^^
this looks wrong


[...]
> +    av_set_pts_info(st, 33, 1, 12);
                           ^^
this looks wrong as well 

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- 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/20071018/1ae7e667/attachment.pgp>