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

[Ivo]

On Wednesday 26 December 2007 14:50, Michael Niedermayer wrote:
> On Tue, Dec 25, 2007 at 01:45:06PM +0100, Ivo wrote:
> > +    avcodec_get_frame_defaults((AVFrame*)&s->picture);
> > +    avctx->coded_frame= (AVFrame*)&s->picture;
>
> what are these casts doing?

No idea :) I copied that part from another decoder. There are quite some 
occurances of these lines in libavcodec. Anyway, I removed the casts,

> > +    s->picture.data[0] = NULL;
>
> why?

Same as above (copied from another decoder). Removed.

> > +static void pcx_ega_palette(uint8_t *src, uint32_t *dst) {
> > +    int i;
> > +
> > +    for (i=0; i<16; i++, dst++, src+=3)
> > +        *dst = (src[0]<<16) + (src[1]<<8) + src[2];
>
> *dst++ = bytestream_get_be24(&src);
[..]
> > +    memset(dst, 0, 240 * 4);
>
> s/4/sizeof(*dst)/

Done.

> > +            for (x=0, b=0; x<w*3; x+=3, b++) {
> > +                ptr[x  ] = scanline[b                    ];
> > +                ptr[x+1] = scanline[b+ bytes_per_line    ];
> > +                ptr[x+2] = scanline[b+(bytes_per_line<<1)];
> > +            }
>
> for (x=0; x<w; x++) {
>     ptr[3*x  ] = scanline[x                    ];
>     ptr[3*x+1] = scanline[x+ bytes_per_line    ];
>     ptr[3*x+2] = scanline[x+(bytes_per_line<<1)];
> }

Done.

> > +        for (y=0; y<256; y++, ptr+=4, buf+=3)
> > +            *(uint32_t *)ptr = (buf[0]<<16) + (buf[1]<<8) + buf[2];
>
> duplicate of pcx_ega_palette()
>
> and pcx_ega_palette() can be factored out of the 3 if()

Done both.

> > +
> > +    } else if (nplanes == 1) {   /* all packed formats, max. 16 colors
> > */ +        static const char params[3][4] = { {3,2,1}, {7,3,1},
> > {1,3,15} }; +        uint8_t scanline[bytes_per_scanline];
> > +        int i = bits_per_pixel == 4 ? 2 : bits_per_pixel-1;
> >
> > +        int d = params[0][i], m = params[1][i], n = params[2][i];
> > +
> > +        for (y=0; y<h; y++) {
> > +            buf = pcx_rle_decode(buf, scanline, bytes_per_scanline,
> > 0); +
> > +            for (x=0; x<w; x++) {
> > +                int v = scanline[x>>d], s = (m-(x&m)) *
> > bits_per_pixel; +                ptr[x] = (v>>s) & n;
> > +            }
>
> get_bits()

Done. This indeed makes the code a lot cleaner. I totally forgot about 
get_bits.

> > +    } 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.

--Ivo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_pcx
Type: text/x-diff
Size: 9062 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071226/571f4673/attachment.diff>