Skip to main content
gamedev.net
🔒 Locked

Write bitmap file

Started by thedirt Jan 9, 2008 at 3:20 PM 17 replies 1.7k views
Original Post
thedirt
thedirt
I am creating a program and need the ability to write bmp files. It works mostly, but the bmp it creates is a little off. output (magnified 400x) See that green pixel on bottom left? Well it's not supposed to be there. I think it starts reading the pixel data one or two bytes early. My code is here: 
#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <fstream>
#include <stdio.h>

int saveBitmap( BYTE *bits, LONG width, LONG height, WORD bpp, LPCTSTR filename )
{
	using namespace std;
	BITMAPINFOHEADER infoheader = {0};
	BITMAPFILEHEADER fileheader = {0};

	infoheader.biSize = sizeof( BITMAPINFOHEADER );
	infoheader.biBitCount = bpp;
	infoheader.biClrImportant = 0;
	infoheader.biClrUsed = 0;
	infoheader.biCompression = BI_RGB;
	infoheader.biWidth = width;
	infoheader.biHeight = height;
	infoheader.biPlanes = 1;
	infoheader.biSizeImage = width * height * ( bpp / 8 );

	fileheader.bfType = 0x4D42;
	fileheader.bfOffBits = sizeof( BITMAPINFOHEADER ) + sizeof( BITMAPFILEHEADER );
	
	fileheader.bfSize = fileheader.bfOffBits + infoheader.biSizeImage; //

	HANDLE output = CreateFile( filename, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL );
	if( !output )
		return -1;

	DWORD written = 0;

	WriteFile( output, &fileheader, sizeof( BITMAPFILEHEADER ), &written, NULL );
	WriteFile( output, &infoheader, sizeof( BITMAPINFOHEADER ), &written, NULL );
	WriteFile( output, &bits, infoheader.biSizeImage, &written, NULL );
	
	return 1;
	CloseHandle( output );
	

}

int main()
{
	BYTE data[192] = { 0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0, 
					   0,0,0,   0,255,0,   0,255,0,   0,0,0,   0,0,0,   0,255,0,   0,255,0,   0,0,0, 
				       0,0,0,   0,255,0,   0,255,0,   0,0,0,   0,0,0,   0,255,0,   0,255,0,   0,0,0,   
					   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,
					   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0, 
					   0,0,0,   0,0,0,   0,255,0,   0,255,0,   0,255,0,   0,255,0,   0,0,0,   0,0,0, 
				       0,0,0,   0,0,0,   0,0,0,   0,255,0,   0,255,0,   0,0,0,   0,0,0,   0,0,0,   
					   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0 };

	return saveBitmap( data, 8, 8, 24, L"output.bmp" );
}
I was hoping someone would be able to catch my stupid mistake that I spent over an hour on. Thank you.
darookie
darookie
Why don't you simply use GDI Plus? It's much less code and offers a variety of output formats (including PNG, JPEG and GIF).
thedirt
thedirt
I am not using gdi+ for simplicity. All I need is Bmp output, and honestly, I don't consider that much code at all.
SiCrane
SiCrane
So you'd rather have buggy code than add a library dependency? I guess it takes all kinds.
Ravyne
Ravyne
I don't thing that's a problem Si, I mean, its not like BMP is a terribly complicated format(*1), and its surely a learning experience by the sounds of things. If all the OP needs is BMP support, then fixing his code is most likely the easier route. If it were something more complicated, or he were further from the goal than he is, I would agree, but I don't think that's warranted in this case.

So anyhow, lets get to the problem! First, observe three facts:
  1. The sample bitmap you linked displays upside-down.

  2. It is also shifted right by one pixel.

  3. It is red, rather than green, as your source data would indicate.


Now, also understand that bitmaps are commonly stored upside-down (because someone decided it should be that way many years ago, to the bane of everyone else.)

These four observations reveal that there is extra data being written prior to the image data. I suspect that there are two extra bytes being written, due to the shade of the blue-green pixel and the fact that windows stores color data in BGR order rather than RGB. If two extra bytes were written, it would have the effect of mis-aligning the color data such that the colors would be effected in a way which would happen to turn green pixels red (can you sense that I'm getting warm?)

Since I don't see anything wrong with the file-writing itself, and the rest of the logic seems sound this leads me to two possible conclusions: either your structures are not packed or one of the structure members is two bytes too big (ie an int that should be a short.) Both of these would cause two extra bytes to be written. My bet is on biPlanes or biBitCount being an int, since only the file header struct would need to be padded out, and any problems there would manifest in the bitmap header, rather than the image data.

I'm about 95% certain that biPlanes or biBitCount (probably the latter) is an int. Fix your bitmap info header, and the problem should be resolved.

(*1) -- The BMP format encompasses several variations for bit depth and also (IIRC) allows for RLE encoding of image data. However, so long as you need a well-defined subset of these features it shouldn't present any problem. Even supporting all variations is not a problem for an intermediate coder.

[Edited by - ravyne2001 on January 9, 2008 5:11:42 PM]
throw table_exception("(? ???)? ? ???");
SiCrane
SiCrane
That would be a far more compelling argument if you were also able to point out what the problem was in his bitmap saving code was. In comparison, here's the GDI+ version of that code:
#include <windows.h>#include <gdiplus.h>using namespace Gdiplus;int main(void) {  GdiplusStartupInput startup;  ULONG_PTR           token;  GdiplusStartup(&token, &startup, NULL);  {    BYTE data[192] = {       0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,       0,0,0,   0,255,0, 0,255,0, 0,0,0,   0,0,0,   0,255,0, 0,255,0, 0,0,0,       0,0,0,   0,255,0, 0,255,0, 0,0,0,   0,0,0,   0,255,0, 0,255,0, 0,0,0,         0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,      0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,       0,0,0,   0,0,0,   0,255,0, 0,255,0, 0,255,0, 0,255,0, 0,0,0,   0,0,0,       0,0,0,   0,0,0,   0,0,0,   0,255,0, 0,255,0, 0,0,0,   0,0,0,   0,0,0,         0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0,   0,0,0 };    Bitmap bitmap(8, 8, 24, PixelFormat24bppRGB, data);    static const CLSID clsid =      { 0x557CF400, 0x1A04, 0x11D3, { 0x9A, 0x73, 0x00, 0x00, 0xF8, 0x1E, 0xF3, 0x2E } };    bitmap.Save(L"output.bmp", &clsid);  }  GdiplusShutdown(token);  return 0;}

The hardest part about that was looking up the CLSID for the bitmap format. Given that you haven't been able to say what the problem with his original code was, I'm going to say that switching to GDI+ is easier than debugging his bitmap saving routine. You'll also note that the entire program, is actually fewer lines of code than just the bitmap saving routine in the original program.
steven katic
steven katic
writing bmp files can be quite trivial, such that it can be too much pain
for little gain if you decide to do it yourself and don't get it write first time, not to mention the reduction of any kudos you may have had in doing so.

So lets swap notes ;):
There is not enough info be be sure why your's doesn't work, so listed below
is an (un-buggy code, as far as I know [so far]) example of very specifically saving( hence some nasty hard coding) an image as a 24 bit bmp on/for Windows 3.1x, 95, NT.....

What I can see in your code ,however, is that you seem to be 'not' rounding the
Bitmap Data Size to the next 4 byte boundary (as per specs). And in the case of BYTE data[192] ,
where 8*8*3 + 54 = 192 + 54(size of header stuff) = 246
and 246%4 = 2!
so need to round up file size to 248? (how does that look..concentration is waning).

By all means use any of the code below as you wish (NB: but at you own risk!)


bool WWriteBMP(char *filename, int width, int height, unsigned char *outImage){	int colorMode = 3;	unsigned char tempColors = 0;	FILE *pFile = 0;	char FileType[2] = {'B','M'};	int totalbytes = width * height * 3 + 54;	int FileSize = 	width * height * 3 + 54;		int mod = totalbytes%4;        // Rounded size to the next 4 byte boundary if needed. 	if(mod != 0)		FileSize = totalbytes - mod + 4;		int Reserved = 0;			int DataOffset = 54;		int HeaderSize = 40;			int Width = width;			int Height = height;		short Planes = 1;				short BitsPerPixel = 24;			int Compression = 0;	// 0 == BI_RGB	int ImageSize = 0;			int XPelsPerMeter = 0;		int YPelsPerMeter = 0;		int UsedColors = 0;			int ImportantColors = 0;			// Open file for output.	pFile = fopen(filename, "wb");	// Check if the file opened or not.	if(!pFile) { fclose(pFile); return false; }		fwrite(&FileType, 1, 2, pFile);	fwrite(&FileSize, 1, 4, pFile);	fwrite(&Reserved, 1, 4, pFile);	fwrite(&DataOffset, 1, 4, pFile);	fwrite(&HeaderSize, 1, 4, pFile);	fwrite(&Width, 1, 4, pFile);	fwrite(&Height, 1, 4, pFile);	fwrite(&Planes, 1, 2, pFile);	fwrite(&BitsPerPixel, 1, 2, pFile);	fwrite(&Compression, 1, 4, pFile);	fwrite(&ImageSize, 1, 4, pFile);	fwrite(&XPelsPerMeter, 1, 4, pFile);	fwrite(&YPelsPerMeter, 1, 4, pFile);	fwrite(&UsedColors, 1, 4, pFile);	fwrite(&ImportantColors, 1, 4, pFile);      // Now switch image from RGB to BGR.      for(int index = 0; index < width * height * colorMode; index += colorMode)      {         tempColors = outImage[index];         outImage[index] = outImage[index + 2];         outImage[index + 2] = tempColors;      }	// Finally write the image.	fwrite(outImage, width * height * 3, 1, pFile);	fclose(pFile);	return true;}


[Edited by - steven katic on January 9, 2008 5:59:13 PM]
Ravyne
Ravyne
So, for the record, I added my diagnosis *after* SiCrane responded to my response. Also for the record, I agree in principle that just using the library is the preferred approach, but disagree in this case because the OP clearly has a single, very small problem in existing code that seemed easily discoverable on its face. I also assumed that the OP had a reason for avoiding the library, whatever that is, and were he ready to give up on his solution in favor of a library, he would have asked for one, rather than help solving his problem.

Assuming my diagnosis is correct, the error turns out to be small, though rather nefarious and rather well hidden if you're not familiar with a number of intricacies involving the bmp format, and even then took me 15 minutes or so to track down (though, it would have been a lot shorter had the definition of the header structs been provided, and saved me a lot of deduction and inference.)


Also, another note for the OP:

Whether you are unaware or simply ignoring it for now, the code as is will not work for images in the general case because your row data is not padded to a multiple of 4 bytes and your existing code does not provide a means to do so. Your test case happens to work since 8 times 3 is 24, which just happens to be a multiple of 4. As it stands, for 24 bit images, your code will not work correctly for any bitmap with an odd number as the horizontal dimension and only every other even horizontal dimension. Basically, it only works for images which have dimensions that are a multiple of 4. If you wish to correct this, your solution will not be able to simply write the entire bits buffer out; instead, you will have to write bits, then pad the ends until it is a multiple of 4. This also affects the calculation of your bfSize member.
throw table_exception("(? ???)? ? ???");
Ravyne
Ravyne
Steven, I'm pretty certain your understanding of the padding issue is mistaken. The format spec indicates that individual *rows* must be padded to multiples of 4, I'm not aware of anything which states that the file, as a whole, must be.
throw table_exception("(? ???)? ? ???");
steven katic
steven katic
Quote:

Steven, I'm pretty certain your understanding of the padding issue is mistaken. The format spec indicates that individual *rows* must be padded to multiples of 4, I'm not aware of anything which states that the file, as a whole, must be.


you have the specs there?

0022h Bitmap Data Size 1 dword Size of the bitmap data in bytes. This number must be rounded to the next 4 byte boundary.


But, does that not ultimately affect the actual file size?
(i.e. 0002h File Size 1 dword Complete file size in bytes.)

Unfortunately for me it is implied, where the specs are not explicit.
More importantly it does work (even though I am not happy about it).


And this is an example of exactly SiCane's point
(That is the best advice: i.e. avoid saving bmp yourself where possible?)
steven katic
steven katic
out of curiosity:

It gets worse:
Do you notice in my sample code that the ImageSize
I save is actually 0?

Are you sure, How can that be?

well if some image reader wants that, reading will fail
won't it. But it hasn't yet, suprisingly?
(e.g.Paint, photoshop and image gallery display the result fine
but it's just a matter of time...isn't it)

well not really: according to another spec:
biSizeImage Specifies the size, in bytes, of the image. It is valid to
set this member to zero if the bitmap is in the BI_RGB format.

And on and on it goes: I am just lucky the code works at all for my limited purposes aren't I?

thedirt:
So, has this convinced you not to do it?
Maybe do it better?

[Edited by - steven katic on January 9, 2008 7:24:56 PM]
Ravyne
Ravyne
Quote:
Original post by steven katic
Quote:

Steven, I'm pretty certain your understanding of the padding issue is mistaken. The format spec indicates that individual *rows* must be padded to multiples of 4, I'm not aware of anything which states that the file, as a whole, must be.


you have the specs there?

0022h Bitmap Data Size 1 dword Size of the bitmap data in bytes. This number must be rounded to the next 4 byte boundary.

Yes I have the specs and yes, the bitmap data size must be a multiple of 4, but it is a result of the fact that each scanline (row) making up the bitmap data must also be bounded on a multiple of 4.

Quote:
But, does that not ultimately affect the actual file size?
(i.e. 0002h File Size 1 dword Complete file size in bytes.)

It absolutely does. The file size is the sum of image data size (after padding, if necessary), as well as all the headers and Color palette if included.

Quote:
Unfortunately for me it is implied, where the specs are not explicit.
More importantly it does work (even though I am not happy about it).

There's a fair bit of weirdness surrounding the bmp file format. I don't think there were ever any defined validation tests for new loaders/savers... There are, frankly, alot of loaders out there that are not strict in their implementation. Its not uncommon for a loader to make certain assumptions based on certain header data, while ignoring others. Most have the basics covered, but there's a lot less compliance once you get into lesser used features like the "ColorsImportant" header section.

Quote:
And this is an example of exactly SiCane's point
(That is the best advice: i.e. avoid saving bmp yourself where possible?)


Well, like I said, I do agree in general. But it seemed like an awful waste of a learning experience when the OP was so close to a working solution (keeping in mind the additional flaws that have been pointed out that will manifest themselves later) and in addition, the OP would have asked for a library if that's what he wanted.

In the end it's pretty likely that the OP will fix his code and use it until it no longer meets his needs, at which point he'll use an available loader.
throw table_exception("(? ???)? ? ???");
steven katic
steven katic
Quote:

Well, like I said, I do agree in general. But it seemed like an awful waste of a learning experience when the OP was so close to a working solution (keeping in mind the additional flaws that have been pointed out that will manifest themselves later) and in addition, the OP would have asked for a library if that's what he wanted.

In the end it's pretty likely that the OP will fix his code and use it until it no longer meets his needs, at which point he'll use an available loader.


I have edited my above post, and don't know if you have seen it
(I know it's a bad habit; sorry; and some edits paraphrase/example your statements e.g. biSizeImage Specifies the size, in bytes, of the image. It is valid to set this member to zero if the bitmap is in the BI_RGB format.
)

yes. To be more explicit: the whole point of my contribution was to
stimulate and discuss exactly what your talking about (with examples).
It's panned out well I believe for the OP I hope. What do you think thedirt?
A bit much?

As for a solution. well that's another issue. (one of many that the OP is likely
to encounter depending how how much of the bmp spec is planned to be supported.)
thedirt
thedirt
Thanks for all the support. I haven't fixed it yet, going to work on it again today. I wasn't ignoring this thread..just sleeping. All I want is to be able to load my bmps (all the same size) with paint. My intentions were not to start a fight, sorry.
Ravyne
Ravyne
No one's fighting, we simply disagree on the merits of fixing your existing code. disagreement is usually an important part of such discourse, and ideally gives you multiple options for fixing the problem.
throw table_exception("(? ???)? ? ???");
thedirt
thedirt
WriteFile( output, &bits, infoheader.biSizeImage, &written, NULL );

needed to be:

WriteFile( output, bits, infoheader.biSizeImage, &written, NULL );

A minor typo. So I guess it was easier to do that than integrate a library. How about that.
steven katic
steven katic
Quote:

How about that.


life can be so ironic sometimes.

Quote:

A minor typo. So I guess it was easier to do that than integrate a library.


...and thats a great punch line to finish with!

Anything else we can help you with? lol
Ravyne
Ravyne
That strikes me as incredibly odd. You're taking the address of the address to data, which happens to be a parameter on the stack, it really should have blown up big time, or at least given garbage in the image. Peculiar.

Anyhow, now you have all the other problems to fix [wink]
throw table_exception("(? ???)? ? ???");
steven katic
steven katic
Quote:

Anyhow, now you have all the other problems to fix


woooooo..is that a sadistic little smilie I see there?

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.