[FFmpeg-devel] [PATCH] PC Paintbrush PCX decoder

Michael Niedermayer michaelni
Wed Dec 26 18:47:09 CET 2007


On Wed, Dec 26, 2007 at 04:54:31PM +0100, Ivo wrote:
> On Wednesday 26 December 2007 14:50, Michael Niedermayer wrote:
[...]
> > > +    } else {    /* planar, 4, 8 or 16 colors */
> > > +        uint8_t scanline[bytes_per_scanline];
> > > +        int i, m;
> > > +
> > > +        for (y=0; y<h; y++) {
> > > +            buf = pcx_rle_decode(buf, scanline, bytes_per_scanline,
> > > 0); +
> > > +            m = 0x80;
> > > +            for (x=0; x<w; x++) {
> > > +                int v = 0;
> > > +                for (i=nplanes - 1; i>=0; i--) {
> > > +                    v <<= 1;
> > > +                    v  += !!(scanline[i*bytes_per_line + (x>>3)] & m);
> > > +                }
> > > +                ptr[x] = v;
> > > +                m >>= 1;
> > > +                m += !m * 0x80;
> > > +            }
> >
> > for (x=0; x<w; x++) {
> >     int m= 0x80 >> (x&7);
> >     for (i=nplanes - 1; i>=0; i--)
> >         put_bits1(pb, !!(scanline[i*bytes_per_line + (x>>3)] & m));
> > }
> 
> I'm not sure if put_bits() will help a lot. I think the overhead of the 
> context, the accounting and the size of the put_bits() code is overkill 
> here. I either need to reinit the context for every pixel or pad the output 
> with zeroes. Or am I missing something?
> 
> I did clean up this part though.
> 
> New patch attached.
[...]
> +/**
> + * @return advanced src pointer
> + */
> +static char *pcx_rle_decode(uint8_t *src, uint8_t *dst,
> +                            unsigned int bytes_per_scanline,
> +                            unsigned int dst_size) {
> +    unsigned int i = 0;
> +    unsigned char c, d;
> +
> +    if (!dst_size) dst_size = bytes_per_scanline;
> +
> +    while (i<bytes_per_scanline) {
> +        c = 1;
> +        d = *src++;
> +        if (d >= 0xc0) {
> +            c = d & 0x3f;
> +            d = *src++;
> +        }
> +        while (i<bytes_per_scanline && c--) {
> +            if (i < dst_size) dst[i] = d;
> +            i++;
> +        }
> +    }

why are there 2 sizes? dst_size and bytes_per_scanline serve the same
purpose, remove one and pass the proper (smaller) size ... but isnt
it an error anyway if bytes_per_scanline > dst_size ?


also s/c/run/
and maybe s/d/color or value or something/


[...]
> +        }
> +
> +    } else if (nplanes == 1) {   /* all packed formats, max. 16 colors */

Odd empty line? or is this intended? (its fine if its intended of course ...)


[...]
> +        pcx_palette(buf, (uint32_t *) p->data[1], 256);
> +        buf += 768;

pass &buf and pcx_palette() will update it automatically


[...]
> +    *picture = *(AVFrame *)&s->picture;
> +    *data_size = sizeof(AVPicture);

Why the cast? And dont you think it looks strange that the sizeof is of a
different struct?

patch ok , except the above things

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

It is not what we do, but why we do it that matters.
-------------- 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/20071226/e4f4edf1/attachment.pgp>



More information about the ffmpeg-devel mailing list