Re: working with bitmaps in C

Malcolm McLean wrote:

This is incorporated in my book Basic Algorithms, so any bug reports
particularly welcome.

Broken for 24-bit and 32-bit, and for 4-bit old-style. See below.

Since it's intended to be pedagogical, I also have a number of serious
reservations about the style and design, but to be honest I don't have
the energy to discuss them at length.

/*********************************************************
* bmp.c - Microsoft bitmap loading functions. *
*********************************************************/

....and saving functions. It'd probably be a good idea to document which
of these functions is part of the public API of bmp.c, how they are
meant to be used, what data formats they expect or return, and so on.

unsigned char *loadbmp(char *fname, int *width, int *height);
unsigned char *loadbmp8bit(char *fname, int *width, int *height, unsigned char *pal);
unsigned char *loadbmp4bit(char *fname, int *width, int *height, unsigned char *pal);

These get (unnecessary) prototypes, but the corresponding save functions
don't.

int bmpgetinfo(char *fname, int *width, int *height)
{
FILE *fp;
BMPHEADER bmphdr;
fp = fopen(fname, "rb");
if(!fp)
return 0;
if(loadheader(fp, &bmphdr) == -1)
{
fclose(fp);
return 0;
}
fclose(fp);
if(width)
*width = bmphdr.width;
if(height)
*height = bmphdr.height;
return bmphdr.bits;
}

What's up with the indention? Some vertical space would be nice, too.

/************************************************************
* loadbmp() - load any bitmap *
* Params: fname - pointer to file path *
* width - return pointer for image width *
* height - return pointer for image height *
* Returns: malloced pointer to image, 0 on fail *
************************************************************/

This description is inadequate for an API function. What loadbmp()
returns is a 24-bit bitmap written as an array of 3-byte pixels in BGR
order, with row span of 3 * width, and origin in the upper left corner
of the image. Callers need this level of detail in order to know how
to use what loadbmp() returns.

unsigned char *loadbmp(char *fname, int *width, int *height)
{ [...]
switch(bmpheader.bits)
{ [...]
case 4:
if(bmpheader.core)
loadpalettecore(fp, pal, 256);

This'll fail. Should be 16 colors, not 256.

for(i=0;i<bmpheader.height;i++)
{
for(ii=0;ii<bmpheader.width;ii++)
{

How about y and x as the loop variables here and elsewhere?

/************************************************************
* loadbmp4bit() - load a 4-bit bitmap from disk. *
* Params: fname - pointer to the file path. *
* width - return pointer for image width. *
* height - return pointer for image height. *
* pal - return pointer for 16 rgb palette entries. *
* Returns: malloced pointer to 4-bit image data. *
************************************************************/

What this actually returns is 4-bit data expanded into 8-bit pixels.

/***********************************************************
* save a24-bit bmp file. *
* Params: fname - name of file to save. *
* rgb - raster data in rgb format *
* width - image width *
* height - image height *
* Returns: 0 on success, -1 on fail *
***********************************************************/
int savebmp(char *fname, unsigned char *rgb, int width, int height)
{ [...]
for(ii=0;ii<width;ii++)
{
fputc(rgb[2], fp);
fputc(rgb[1], fp);
fputc(rgb[0], fp);
rgb += 3;
}

Oops. Here's where careful documentation of your internal bitmap format
would come in handy. loadbmp() writes pixels to the unsigned char array
in BGR order, but savebmp() assumes that they're written in RGB order.

Did you actually test this code by calling loadbmp() and savebmp() and
looking at the output? Because I think you'll find that your load and
save aren't inverses.

/***************************************************************
* save a 2-bit palettised bitmap. *

Actually, this saves a 1-bit bitmap.

/**************************************************************
* loadpalette() - load palette for a new format BMP. *
* Params: fp - pointer to an open file. *
* pal - return pointer for palette entries. *
* entries - number of entries in palette. *
**************************************************************/
static void loadpalette(FILE *fp, unsigned char *pal, int entries)
{
int i;
for(i=0;i<entries;i++)
{
pal[2] = fgetc(fp);
pal[1] = fgetc(fp);
pal[0] = fgetc(fp);
fgetc(fp);
pal += 3;
}
}

Wait, the plot thickens. The palette channels are inverted here, so
loadbmp() will convert indexed color images to 24-bit in RGB order, but
will store 24-bit and 32-bit BMPs in BGR order.

static int loadheader(FILE *fp, BMPHEADER *hdr)
{ [...]
if(hdrsize == 40)
{ [...]
}
else if(hdrsize == 12)
{ [...]
}
else
return 0;
if(ferror(fp))
return -1;
return 0;
}

This returns 0, your indication of success, if the header size is
neither 40 nor 12 and therefore a header you don't process.

static void saveheader(FILE *fp, int width, int height, int bits)
{ [...]
/* pels per metre */
fput32le(600000, fp);
fput32le(600000, fp);

Why 600000? That's 15240 DPI. Since you don't allow the caller to set
this, the safest choice is 0.

/*****************************************************
* get the size of the file to be written. *
* Params: width - image width *
* height - image height *
* bits - image type *
* Returns: size of image data (excluding headers) *
*****************************************************/
static long getfilesize(int width, int height, int bits)
{
long answer = 0;
switch(bits)
{
case 1:
answer = (width + 7)/8;
answer = (answer + 3)/4 * 4;
answer *= height;
answer += 2 * 4;
break;
case 4:
answer = 16 * 4 + (width + 1)/2;
answer = (answer + 3)/4 * 4;
answer *= height;
answer += 16 * 4;
break;
case 8:
answer = (width + 3)/4 * 4;
answer *= height;
answer += 256 * 4;
break;
case 16:
answer = (width * 2 + 3)/4 * 4;
answer *= height;
break;
case 24:
answer = (width * 3 + 3)/4 * 4;
answer *= height;
break;
case 32:
answer = width * height * 4;
break;
default:
return 0;
}

return answer;
}

All of this could be replaced with

palsize = depth <= 8 ? ( 1 << depth ) * 4 : 0;
rowspan = (( w * depth + 31 ) / 32 ) * 4;
return rowspan * height + palsize;

In fact, since you calculate palsize and rowspan separately at a number
of different points in the code, it might be a good idea to turn each of
the first two lines into a macro or a function, so that you only have to
get them right once.

/***************************************************************
* swap an area of memory *
* Params: x - pointer to first buffer *
* y - pointer to second buffer *
* len - length of memory to swap *
***************************************************************/
static void swap(void *x, void *y, int len)
{
unsigned char *ptr1 = x;
unsigned char *ptr2 = y;
unsigned char temp;
int i;

for(i=0;i<len;i++)
{
temp = ptr1[i];
ptr1[i] = ptr2[i];
ptr2[i] = temp;
}
}

Rather than the load functions calling this to convert bottom-up
scanline order to top-down, wouldn't it be faster, and not much harder,
simply to load the pixels into your bitmap in the order you want in the
first place?

You could also invert the process so that you can write canonical BMPs
in bottom-up order, with a positive BITMAPINFOHEADER.biHeight.

- Ernie http://home.comcast.net/~erniew
.

Relevant Pages