[PATCH] PC Paintbrush PCX decoder
Hi, Here's a decoder for PC Paintbrush PCX files. http://wiki.multimedia.cx/index.php?title=PCX Samples are uploaded to: http://samples.mplayerhq.hu/MPlayer/incoming/pcx Merry christmas, --Ivo
Ivo wrote:
Hi,
Here's a decoder for PC Paintbrush PCX files.
Cool, I haven't seen those since cutting my teeth on hacking graphics apps in DirectX 3.0. I know they're much older, but that's the first time I was old enough to be aware of them. Brings back memories :-) -- Med venlig hilsen Christian Iversen
On Tuesday 25 December 2007 16:31, Christian Iversen wrote:
Ivo wrote:
Hi,
Here's a decoder for PC Paintbrush PCX files.
Cool, I haven't seen those since cutting my teeth on hacking graphics apps in DirectX 3.0. I know they're much older, but that's the first time I was old enough to be aware of them.
Brings back memories :-)
Yeah, it's a pretty old file format :) But I think it would be a Good Thing if ffmpeg supported as many file formats as possible. Writing to old formats is probably not that useful, but even years from now, being able to read and convert them can be very helpful. --Ivo
Ivo wrote:
On Tuesday 25 December 2007 16:31, Christian Iversen wrote:
Ivo wrote:
Hi,
Here's a decoder for PC Paintbrush PCX files. Cool, I haven't seen those since cutting my teeth on hacking graphics apps in DirectX 3.0. I know they're much older, but that's the first time I was old enough to be aware of them.
Brings back memories :-)
Yeah, it's a pretty old file format :) But I think it would be a Good Thing if ffmpeg supported as many file formats as possible. Writing to old formats is probably not that useful, but even years from now, being able to read and convert them can be very helpful.
I agree completely. I think it's excellent work! -- Med venlig hilsen Christian Iversen
On Tuesday 25 December 2007 13:45, Ivo wrote:
Samples are uploaded to:
Ehm, that should have been ftp://... Also, something went wrong while uploading the files and I could not cd into incoming/pcx again, so there's also a pcx2 directory which containt the proper files. --Ivo
On Wed, 26 Dec 2007 13:58:22 +0100 Ivo <ivop at euronet.nl> wrote:
On Tuesday 25 December 2007 13:45, Ivo wrote:
Samples are uploaded to:
Ehm, that should have been ftp://... Also, something went wrong while uploading the files and I could not cd into incoming/pcx again, so there's also a pcx2 directory which containt the proper files.
Moves to samples/pcx Ciao, Roberto -- Better is the enemy of good enough.
On Tue, Dec 25, 2007 at 01:45:06PM +0100, Ivo wrote:
Hi,
Here's a decoder for PC Paintbrush PCX files.
http://wiki.multimedia.cx/index.php?title=PCX
Samples are uploaded to:
http://samples.mplayerhq.hu/MPlayer/incoming/pcx
Merry christmas, --Ivo
[...]
+typedef struct PCXContext { + AVFrame picture; +} PCXContext; + +static int pcx_init(AVCodecContext *avctx) { + PCXContext *s = avctx->priv_data; + + avcodec_get_frame_defaults((AVFrame*)&s->picture); + avctx->coded_frame= (AVFrame*)&s->picture;
what are these casts doing?
+ s->picture.data[0] = NULL;
why? [...]
+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)/ [...]
+ 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)]; }
+ + ptr += stride; + } + + } else if (nplanes == 1 && bits_per_pixel == 8) { [...] + 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()
+ + } 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()
+ ptr += stride; + } + + pcx_ega_palette(bufstart+16, (uint32_t *) p->data[1]); + + } 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)); } [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The misfortune of the wise is better than the prosperity of the fool. -- Epicurus
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
On Wednesday 26 December 2007 16:54, Ivo wrote:
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.
Forgot one instance of a useless cast. I also fixed the *picture= assignment later on and updated the Sun Rasterfile code. --Ivo
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.
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
On Wed, Dec 26, 2007 at 07:56:52PM +0100, Ivo wrote:
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.
it can be forced but it has other disadvantages, that is you loose direct rendering support ... anyway, i do not think the check per sample is faster than a buffer and memcpy you could also try: if(i<dst_size) memset(dst+i, d, FFMIN(c, dst_size-i)); i+=c; [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates
On Wednesday 26 December 2007 21:00, Michael Niedermayer wrote:
On Wed, Dec 26, 2007 at 07:56:52PM +0100, Ivo wrote:
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.
it can be forced but it has other disadvantages, that is you loose direct rendering support ...
anyway, i do not think the check per sample is faster than a buffer and memcpy
you could also try:
if(i<dst_size) memset(dst+i, d, FFMIN(c, dst_size-i)); i+=c;
Ok. I chose the buffer/memcpy way as that simplifies rle_decode and speeds up all non-pal8 cases. The code is also ~50 bytes smaller (and the memset code was 150+ bytes larger). Also, most runs for a typical file will be relatively small or no run at all, so memset might actually hurt. Comitted. --Ivo
On Tue, Dec 25, 2007 at 01:45:06PM +0100, Ivo wrote:
Here's a decoder for PC Paintbrush PCX files.
--- libavcodec/pcx.c (revision 0) +++ libavcodec/pcx.c (revision 0) @@ -0,0 +1,254 @@ +/* + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * The decoder does not support CGA palettes. I am unable to find samples + * and Netpbm cannot generate them.
For consistency's sake, please put this comment above the licensing block. Other than that the patch looks OK. Diego
+ * The decoder does not support CGA palettes. I am unable to find samples + * and Netpbm cannot generate them.
I have a contact creating some authentic CGA samples. I will upload them when they are ready. Are there any other types of samples you need? -- -Mike Melanson
On Thursday 27 December 2007 07:37, Mike Melanson wrote:
+ * The decoder does not support CGA palettes. I am unable to find samples + * and Netpbm cannot generate them.
I have a contact creating some authentic CGA samples. I will upload them when they are ready. Are there any other types of samples you need?
No, CGA paletted images were the only ones I could not find. Thanks! --Ivo
Ivo wrote:
No, CGA paletted images were the only ones I could not find.
Here you go: http://samples.mplayerhq.hu/pcx/cga/
From my contact: "These all load perfectly under *REAL* PC Paintbrush 4.0 in DOS on a CGA card, so if your decoder screws it up, it's the fault of your decoder!"
2008 is almost here-- we need CGA PCX support in FFmpeg fast! :) -- -Mike Melanson
On Saturday 29 December 2007 22:21, Mike Melanson wrote:
Ivo wrote:
No, CGA paletted images were the only ones I could not find.
Here you go:
http://samples.mplayerhq.hu/pcx/cga/
From my contact: "These all load perfectly under *REAL* PC Paintbrush 4.0 in DOS on a CGA card, so if your decoder screws it up, it's the fault of your decoder!"
2008 is almost here-- we need CGA PCX support in FFmpeg fast! :)
Thanks! Alas, there's a problem. From ZSoft's specs:
CGA Color Palette Information
NOTE: This is no longer supported for PC Paintbrush IV/IV Plus.
The sample files: CGA_BW.PCX - Renders correctly with the current code (contains 2 RGB triplets in the header) CGA_FSD.PCX - Renders correctly with the current code (contains 4 RGB triplets in the header specifying the most common palette) The other two do not render correctly, either without or with added code to handle the CGA palette according to the spec (4 bits of byte 16 determine the background color and 3 bits of byte 19 determine foreground palette). For example, the intensity bit is set in both files, but the text in the images clearly states that one has it on and the other has it off. The choice of 4-color CGA palette seems to go well, but that might just be pure luck. And both files have the color_burst bit set, which should have been off. All in all, I can't make it work for all cases. Perhaps it would be possible to write exactly the same image (preferably CGA_TST1.PCX) in all eight formats? color_burst color/monochrome, intensity dim/bright and palette yellow/white makes eight combinations. Maybe I can find out how PC Paintbrush >= 4 stores that information. If possible, it would also be nice to have the same images written by Paintbrush < 4 :-) --Ivo
Ivo wrote:
If possible, it would also be nice to have the same images written by Paintbrush < 4 :-)
So you know, my contact is even more fanatical about ancient formats than me. He has procured a copy of Paintbrush 1.0 from eBay for this task. Further samples in a few days. -- -Mike Melanson
On Friday 04 January 2008 16:31, Mike Melanson wrote:
Ivo wrote:
If possible, it would also be nice to have the same images written by Paintbrush < 4 :-)
So you know, my contact is even more fanatical about ancient formats than me. He has procured a copy of Paintbrush 1.0 from eBay for this task. Further samples in a few days.
:-)))) Looking forward to the sample files. It would be really nice to support them all. --Ivo
participants (6)
-
chrivers＠iversen-net.dk -
diego＠biurrun.de -
ivop＠euronet.nl -
michaelni＠gmx.at -
mike＠multimedia.cx -
rxt＠rtogni.it