[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