Bug in SDL_image PCX loader

Mike_Battersby · February 22, 2001, 2:15am

support in the SDL_image library is incorrect. For bit depths < 8 it
should use the palette in the PCX header. The following patch seems
correct to me:

(My newsreader has probably stuffed up the tabbing, but fortunately the
patch is small enough that you can apply it by hand anyway.)

— IMG_pcx.c.orig Wed Feb 21 22:52:40 2001
+++ IMG_pcx.c Wed Feb 21 22:54:32 2001
@@ -152,15 +152,15 @@

/* Look for the palette, if necessary */
switch (surface->format->BitsPerPixel) {

```
  case 1: {
```

```
  case 1:
```
```
  case 2:
```

  case 4: {
  SDL_Color *colors = surface->format->palette->colors;>From my reading of the PCX spec the palette color handling of the PCX

```
  colors[0].r = 0x00;
```
```
  colors[0].g = 0x00;
```
```
  colors[0].b = 0x00;
```
```
  colors[1].r = 0xFF;
```
```
  colors[1].g = 0xFF;
```
```
  colors[1].b = 0xFF;
```

  for (i=0; i<surface->format->palette->ncolors; ++i) {

```
  	colors[i].r = pcxh.Colormap[3*i];
```
```
  	colors[i].g = pcxh.Colormap[3*i+1];
```
```
  	colors[i].b = pcxh.Colormap[3*i+2];
```
```
  }
  }
  break;
```

Mike

Mattias_Engdegard · February 22, 2001, 11:18am

support in the SDL_image library is incorrect. For bit depths < 8 it
should use the palette in the PCX header.

i’m not sure if this suffices for <8 bit surfaces — we have to decide
whether to expand them to 8bpp or keep them at their original depth and
in the latter case there’s bit/nybble order to think about

in general the support for <8bpp surfaces in SDL is spotty at best and
not really well-defined. it’s something we should think hard about for
1.3>>From my reading of the PCX spec the palette color handling of the PCX

slouken · February 22, 2001, 5:48pm

support in the SDL_image library is incorrect. For bit depths < 8 it
should use the palette in the PCX header.

i’m not sure if this suffices for <8 bit surfaces — we have to decide
whether to expand them to 8bpp or keep them at their original depth and
in the latter case there’s bit/nybble order to think about

in general the support for <8bpp surfaces in SDL is spotty at best and
not really well-defined. it’s something we should think hard about for
1.3

What has been done in the past is expand the surfaces to 8 bpp with
a 256 entry palette that only has colors in the first 2^N entries.
This works very well with SDL.

See ya,
-Sam Lantinga, Lead Programmer, Loki Entertainment Software> >>From my reading of the PCX spec the palette color handling of the PCX

David_Olofson · February 22, 2001, 10:00pm

More than one pixel/byte… Great fun! Reminds me of the bitplane
based Amiga graphics chipset. hehe

However, it’s not all that different from doing 24 bits/3 bytes/pixel
efficiently with 32 or 64 bit operations. I’m just wondering if it’s
worth the effort… When do you have too low bandwidth or too hard
memory constraints to deal with 8 bit surfaces?

Not arguing against <8 bit/pixel support, though - it’s just not very
likely that I’ll be the one to hack the code.

//David

.- M A I A -------------------------------------------------.
| Multimedia Application Integration Architecture |
| A Free/Open Source Plugin API for Professional Multimedia |
----------------------> http://www.linuxaudiodev.com/maia -' .- David Olofson -------------------------------------------. | Audio Hacker - Open Source Advocate - Singer - Songwriter |--------------------------------------> david at linuxdj.com -'On Thursday 22 February 2001 12:18, Mattias Engdeg?rd wrote:

From my reading of the PCX spec the palette color handling of the
PCX

support in the SDL_image library is incorrect. For bit depths < 8
it should use the palette in the PCX header.

i’m not sure if this suffices for <8 bit surfaces — we have to
decide whether to expand them to 8bpp or keep them at their
original depth and in the latter case there’s bit/nybble order to
think about

in general the support for <8bpp surfaces in SDL is spotty at best
and not really well-defined. it’s something we should think hard
about for 1.3

Mattias_Engdegard · February 23, 2001, 3:13pm

What has been done in the past is expand the surfaces to 8 bpp with
a 256 entry palette that only has colors in the first 2^N entries.
This works very well with SDL.

indeed and it reduces surprises for the user. On the other hand we probably
want better support for <8bpp surfaces in 1.3 (mainly for low-depth devices
such as handhelds)

slouken · February 23, 2001, 3:48pm

indeed and it reduces surprises for the user. On the other hand we probably
want better support for <8bpp surfaces in 1.3 (mainly for low-depth devices
such as handhelds)

Yup.
-Sam Lantinga, Lead Programmer, Loki Entertainment Software

Bill_Kendrick · February 23, 2001, 5:51pm

indeed and it reduces surprises for the user. On the other hand we probably
want better support for <8bpp surfaces in 1.3 (mainly for low-depth devices
such as handhelds)

Yes! 16 greyshades, damnit!

-bill!

Mike_Battersby · February 26, 2001, 12:41am

While I appreciate all the design discussions, I have an immediate
problem in that a 1 bit PCX file I’m loading with SDL_image doesn’t get
the palette loaded.

Is the patch I sent sufficient to go in to SDL_image? If not can someone
please tell me what’s wrong with it so I can write one that is.

MikeOn 22 Feb 2001 09:47:28 -0800, Sam Lantinga wrote:

i’m not sure if this suffices for <8 bit surfaces — we have to decide
whether to expand them to 8bpp or keep them at their original depth and
in the latter case there’s bit/nybble order to think about

What has been done in the past is expand the surfaces to 8 bpp with
a 256 entry palette that only has colors in the first 2^N entries.
This works very well with SDL.

Mattias_Engdegard · February 26, 2001, 9:32am

While I appreciate all the design discussions, I have an immediate
problem in that a 1 bit PCX file I’m loading with SDL_image doesn’t get
the palette loaded.

would you prefer a <8bpp file to be loaded as a 1 or 8bpp surface? should
we settle on a bit/nybble order for all surfaces or augment the SDL_format
struct to contain a flag for this? what about the bitmap unit, should it
be a fixed 8 or 32 bits or perhaps another parameter (like XImage has it)?

i’d be happy to hard-code a format (and it would really help the
implementation) if someone can convince me that all low-depth framebuffers
that we would like to support have the same format. for the record,
atari st planar format is out, and so are the speccy’s three venetian blinds

Is the patch I sent sufficient to go in to SDL_image? If not can someone
please tell me what’s wrong with it so I can write one that is.

no worry, i’ll whip up a solution as soon as i’ve decided what it should be
should be; i’ll probably convert it to a less controversial 8bpp for the
time being

Mattias_Engdegard · February 26, 2001, 3:23pm

i’m trying to do this properly, so can someone please tell me what
depth/plane combinations of PCX files are actually used?
it seems that the set includes

1 plane, 8 bits/plane (ordinary 8bpp)
3 planes, 8 bits/plane (planar 24bpp)
1…4 planes, 1 bit/plane
1 plane, 1,2,4 bits/plane

The first two cases are obvious and already supported in SDL_image. It seems
that Gimp also supports 1 and 4 planes, 1 bit/plane, and netpbm can
read/write the 1 plane, 1,2,4 bits/plane format. The pcx docs I have
(from www.wotsit.org) also mentions planar representation with a fourth
"intensity" plane, but i’m not sure how to interpret that

test images would be appreciated

Mike_Battersby · February 27, 2001, 2:10am

While I appreciate all the design discussions, I have an immediate
problem in that a 1 bit PCX file I’m loading with SDL_image doesn’t get
the palette loaded.

would you prefer a <8bpp file to be loaded as a 1 or 8bpp surface? should
we settle on a bit/nybble order for all surfaces or augment the SDL_format
struct to contain a flag for this? what about the bitmap unit, should it
be a fixed 8 or 32 bits or perhaps another parameter (like XImage has it)?

For my purposes, as long as I can color-key blit it into an RGBA surface
then I don’t care (it’s for texture building for OpenGL). Practically
speaking I think it’s reasonable given the current SDL implementation
to convert <8bpp images into 8bpp surfaces, but I’ll leave the details
to the SDL gurus as there may be issues with (e.g.) handheld devices that
need considering.

no worry, i’ll whip up a solution as soon as i’ve decided what it should be
should be; i’ll probably convert it to a less controversial 8bpp for the
time being

Okay, I’ll look forward to it.

Mike

Mike_Battersby · February 27, 2001, 2:16am

i’m trying to do this properly, so can someone please tell me what
depth/plane combinations of PCX files are actually used?

I’m working off the netpbm format sets, as it’s the most comprehensive
PCX handler I’ve been able to get hold of in terms of handling all of
the in-the-wild PCX images I’ve found. I’ll mail you a couple of sample
images but they’re probably nothing you can’t create yourself with netpbm.

(from www.wotsit.org) also mentions planar representation with a fourth
"intensity" plane, but i’m not sure how to interpret that

No idea.

MikeOn 26 Feb 2001 07:31:39 -0800, Mattias Engdeg?rd wrote:

Mattias_Engdegard · February 27, 2001, 8:18pm

here’s the patch, sorry for the delay. there’s also a small update to
showimage to use a hwpalette when needed (I’ll see if I can find a way
around this later)

I didn’t bother to add support for packed-pixel types that gimp won’t
read, assuming them to be rare (none of the test images I received
were of that kind). thanks everyone for the help

Index: IMG_pcx.c===================================================================
RCS file: /cvs/SDL_image/IMG_pcx.c,v
retrieving revision 1.3
diff -u -r1.3 IMG_pcx.c
— IMG_pcx.c 2000/11/29 11:55:32 1.3
+++ IMG_pcx.c 2001/02/27 19:52:21
@@ -22,9 +22,19 @@
slouken at devolution.com
*/

-/* This is a PCX image file loading framework */

+/*

- PCX file reader:
- Supports:
- 1…4 bits/pixel in multiplanar format (1 bit/plane/pixel)
- 8 bits/pixel in single-planar format (8 bits/plane/pixel)
- 24 bits/pixel in 3-plane format (8 bits/plane/pixel)
- Doesn’t support:
- single-planar packed-pixel formats other than 8bpp
- 4-plane 32bpp format with a fourth “intensity” plane
*/
#include <stdio.h>
+#include <stdlib.h>

#include “SDL_endian.h”

@@ -77,24 +87,19 @@
Uint32 Gmask;
Uint32 Bmask;
Uint32 Amask;

SDL_Surface *surface;

SDL_Surface *surface = NULL;
int width, height;

int i, index, x, y;
int count;
Uint8 *row, ch;
int read_error;
/* Initialize the data we will clean up when we’re done */
surface = NULL;
read_error = 0;

int y, bpl;
Uint8 *row, *buf = NULL;
char *error = NULL;
int bits, src_bits;

/* Check to make sure we have something to do */
if ( ! src ) {
goto done;
}
/* Read and convert the header */
if ( ! SDL_RWread(src, &pcxh, sizeof(pcxh), 1) ) {

```
  error = "file truncated";
  goto done;
```
}
pcxh.Xmin = SDL_SwapLE16(pcxh.Xmin);
@@ -106,8 +111,13 @@
/* Create the surface of the appropriate type */
width = (pcxh.Xmax - pcxh.Xmin) + 1;
height = (pcxh.Ymax - pcxh.Ymin) + 1;

Rmask = Gmask = Bmask = Amask = 0 ;
if ( pcxh.BitsPerPixel * pcxh.NPlanes > 16 ) {

Rmask = Gmask = Bmask = Amask = 0;
src_bits = pcxh.BitsPerPixel * pcxh.NPlanes;
if((pcxh.BitsPerPixel == 1 && pcxh.NPlanes >= 1 && pcxh.NPlanes <= 4)

 || (pcxh.BitsPerPixel == 8 && pcxh.NPlanes == 1)) {

```
  bits = 8;
```
} else if(pcxh.BitsPerPixel == 8 && pcxh.NPlanes == 3) {

  bits = 24;
  if ( SDL_BYTEORDER == SDL_LIL_ENDIAN ) {
  	Rmask = 0x000000FF;
  	Gmask = 0x0000FF00;

@@ -117,83 +127,108 @@
Gmask = 0x00FF00;
Bmask = 0x0000FF;
}

} else {
```
  error = "unsupported PCX format";
```
```
  goto done;
```
}
surface = SDL_AllocSurface(SDL_SWSURFACE, width, height,

  	pcxh.BitsPerPixel*pcxh.NPlanes,Rmask,Gmask,Bmask,Amask);

if ( surface == NULL ) {
```
  IMG_SetError("Out of memory");
```

  		   bits, Rmask, Gmask, Bmask, Amask);

if ( surface == NULL )
goto done;

}
/* Decode the image to the surface */

bpl = pcxh.NPlanes * pcxh.BytesPerLine;
buf = malloc(bpl);
row = surface->pixels;
for ( y=0; yh; ++y ) {

```
  for ( i=0; i<pcxh.NPlanes; ++i ) {
```

  	row = (Uint8 *)surface->pixels + y*surface->pitch;

```
  	index = i;
```
```
  	for ( x=0; x<pcxh.BytesPerLine; ) {
```

  		if ( ! SDL_RWread(src, &ch, 1, 1) ) {

```
  			read_error = 1;
```

  /* decode a scan line to a temporary buffer first */

```
  int i, count = 0;
```
```
  Uint8 ch;
```

  Uint8 *dst = (src_bits == 8) ? row : buf;

```
  for(i = 0; i < bpl; i++) {
```
```
  	if(!count) {
```
```
  		if(!SDL_RWread(src, &ch, 1, 1)) {
```

  			error = "file truncated";
  			goto done;
  		}

```
  		if ( (ch & 0xC0) == 0xC0 ) {
```
```
  			count = ch & 0x3F;
```
```
  			SDL_RWread(src, &ch, 1, 1);
```
```
  		} else {
```

```
  		if( (ch & 0xc0) == 0xc0) {
```
```
  			count = ch & 0x3f;
```
```
  			if(!SDL_RWread(src, &ch, 1, 1)) {
```
```
  				error = "file truncated";
```
```
  				goto done;
```
```
  			}
```
```
  		} else
  			count = 1;
```
```
  	}
```
```
  	dst[i] = ch;
```
```
  	count--;
```
```
  }
```
```
  if(src_bits <= 4) {
```
```
  	/* expand planes to 1 byte/pixel */
```
```
  	Uint8 *src = buf;
```
```
  	int plane;
```

  	for(plane = 0; plane < pcxh.NPlanes; plane++) {

```
  		int i, j, x = 0;
```

  		for(i = 0; i < pcxh.BytesPerLine; i++) {

```
  			Uint8 byte = *src++;
```
```
  			for(j = 7; j >= 0; j--) {
```
```
  				unsigned bit = (byte >> j) & 1;
```
```
  				row[x++] |= bit << plane;
```
```
  			}
  		}
```

```
  		while ( count-- ) {
```
```
  			row[index] = ch;
```
```
  			++x;
```
```
  			index += pcxh.NPlanes;
```

```
  	}
```
```
  } else if(src_bits == 24) {
```
```
  	/* de-interlace planes */
```
```
  	Uint8 *src = buf;
```
```
  	int plane;
```

  	for(plane = 0; plane < pcxh.NPlanes; plane++) {

```
  		int x;
```
```
  		dst = row + plane;
```
```
  		for(x = 0; x < width; x++) {
```
```
  			*dst = *src++;
```

  			dst += pcxh.NPlanes;
  		}
  	}
  }

```
  row += surface->pitch;
```
}

/* Look for the palette, if necessary */
switch (surface->format->BitsPerPixel) {
```
  case 1: {
```

if(bits == 8) {
SDL_Color *colors = surface->format->palette->colors;
```
  int nc = 1 << src_bits;
```
```
  int i;
```

```
  colors[0].r = 0x00;
```
```
  colors[0].g = 0x00;
```
```
  colors[0].b = 0x00;
```
```
  colors[1].r = 0xFF;
```
```
  colors[1].g = 0xFF;
```
```
  colors[1].b = 0xFF;
```
```
  }
```
```
  break;
```
```
  case 8: {
```

  SDL_Color *colors = surface->format->palette->colors;

  surface->format->palette->ncolors = nc;

```
  if(src_bits == 8) {
```
```
  	Uint8 ch;
```
```
  	/* look for a 256-colour palette */
```
```
  	do {
```

  		if ( !SDL_RWread(src, &ch, 1, 1)) {

```
  			error = "file truncated";
```
```
  			goto done;
```
```
  		}
```
```
  	} while ( ch != 12 );
```

```
  /* Look for the palette */
```
```
  do {
```

  	if ( ! SDL_RWread(src, &ch, 1, 1) ) {

```
  		read_error = 1;
```
```
  		goto done;
```

```
  	for(i = 0; i < 256; i++) {
```

  		SDL_RWread(src, &colors[i].r, 1, 1);

  		SDL_RWread(src, &colors[i].g, 1, 1);

  		SDL_RWread(src, &colors[i].b, 1, 1);
  	}

```
  } while ( ch != 12 );
```
```
  /* Set the image surface palette */
```
```
  for ( i=0; i<256; ++i ) {
```

  	SDL_RWread(src, &colors[i].r, 1, 1);

  	SDL_RWread(src, &colors[i].g, 1, 1);

  	SDL_RWread(src, &colors[i].b, 1, 1);

```
  } else {
```
```
  	for(i = 0; i < nc; i++) {
```

  		colors[i].r = pcxh.Colormap[i * 3];

  		colors[i].g = pcxh.Colormap[i * 3 + 1];

  		colors[i].b = pcxh.Colormap[i * 3 + 2];

```
  	}
  }
```

```
  }
```
```
  break;
```
```
  default: {
```
```
  /* Don't do anything... */;
```
```
  }
```
```
  break;
```
}

done:

if ( read_error ) {

free(buf);
if ( error ) {
SDL_FreeSurface(surface);

  IMG_SetError("Error reading PCX data");

  IMG_SetError(error);
  surface = NULL;

}
return(surface);

Index: showimage.c

RCS file: /cvs/SDL_image/showimage.c,v
retrieving revision 1.6
diff -u -r1.6 showimage.c
— showimage.c 2001/01/31 00:46:16 1.6
+++ showimage.c 2001/02/27 19:56:01
@@ -118,6 +118,8 @@
if ( (image->format->BytesPerPixel > 1) && (depth == 8) ) {
depth = 32;
}

```
  if(depth == 8)
```

  	flags |= SDL_HWPALETTE;
  screen = SDL_SetVideoMode(image->w, image->h, depth, flags);
  if ( screen == NULL ) {
  	fprintf(stderr,"Couldn't set %dx%dx%d video mode: %s\n",