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

[Ivo]

On Wednesday 26 December 2007 18:47, Michael Niedermayer wrote:
> On Wed, Dec 26, 2007 at 04:54:31PM +0100, Ivo wrote:
> > +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 ?

Scanlines in PCX files are padded at the right side, so it's possible 
decoded bytes will be written outside of the dst buffer while decoding the 
last scanline (only the PAL8 case which decodes directly to 
s->picture->data[0], others decode to an intermediate scanline buffer that 
is always large enough).

Is there a way to force s->picture->linesize[0] to bytes_per_scanline during 
allocation? In that case I could simplify the code and get rid of dst_size.

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

Done.

> > +        }
> > +
> > +    } else if (nplanes == 1) {   /* all packed formats, max. 16 colors
> > */
>
> Odd empty line? or is this intended? (its fine if its intended of course
> ...)

Yes, it was intended so I could spot the different '} else if {' branches 
more easily :)

> > +        pcx_palette(buf, (uint32_t *) p->data[1], 256);
> > +        buf += 768;
>
> pass &buf and pcx_palette() will update it automatically

Done.

> > +    *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?

Yes. I already removed the cast, but the sizeof indeed looks strange. I 
grepped around the libavcodec sources and both sizeof(AVFrame) and 
sizeof(AVPicture) occur at the same places in different files (i.e. at the 
end of decode_frame). I changed it to sizeof(AVFrame) here.

> patch ok , except the above things

I'll wait for you answer on my question about forcing a larger linesize than 
strictly necessary for a given image width before commiting.

--Ivo