Memory Leak!

Started by
7 comments, last by Russell 22 years, 9 months ago
So I''m chatting on Instant Messenger, and I see that some of my icons have gone black. So I check my system resources, and it says that I have less than 1% free system resources...yikes! So I notice that my windows program has been running in the background for a few minutes, and I close it. All of the sudden, my system resources are back up to 55% free. Memory leak? I''d say so. My program is a chess program, and the leaks is occuring when I call the draw_board function. My free system resources go down a few percent everytime the board gets re-drawn (for example, each time I maximize the window, move it, etc. causing it to be re-drawn). Here''s the code from the draw_board function. I''m sure I need to be deleting/releasing a device context or bitmap, but I don''t know which, since I''m not really into windows programming. I''m just doing it for the GUI for my chess program.
  
void draw_board(HDC hdc, HBRUSH light_col, HBRUSH dark_col)
{
	RECT border;
	border.top = 0;
	border.left = 0;
	border.bottom = (52*8) + 30;
	border.right = (52*8) + 30;

	FillRect(hdc, &border, (HBRUSH)GetStockObject(BLACK_BRUSH));

	int x, y;
	int color = 1;

	for(y = 0; y < 8; y++)
	{
		for(x = 0; x < 8; x++)
		{
			RECT rect;
			rect.top = (y * 52) + border_size;
			rect.left = (x * 52) + border_size;
			rect.bottom = ((y * 52) + 52) + border_size;
			rect.right = ((x * 52) + 52) + border_size;

			if(color == 1)
				FillRect(hdc, &rect, dark_col);
			else
				FillRect(hdc, &rect, light_col);
			color *= -1;
		}
		color *= -1;
	}

	for(x = 0; x < 8; x++)
	{
		HBITMAP white_pawn = (HBITMAP)LoadImage((HINSTANCE)global_instance, MAKEINTRESOURCE(WHITE_PAWN), IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION);
	}
	
	HBITMAP white_rook = (HBITMAP)LoadImage((HINSTANCE)global_instance, MAKEINTRESOURCE(WHITE_ROOK), IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION);

	HBITMAP white_knight = (HBITMAP)LoadImage((HINSTANCE)global_instance, MAKEINTRESOURCE(WHITE_KNIGHT), IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION);
}
  
Thanks for any help.
Advertisement
Hmm. Possibly more a resource leak. Windows has a finite number of handles for things.

Why do you need (at least) 8 copies of one bitmap? When you initialize, load up one copy, and use that copy from then on (put your needed bitmaps into a member variable, or global if you''re not using classes). How many times are you calling this function?

It''s also possible that you''re leaking elsewhere, I doubt this is the only thing in your app.

-fel

~ The opinions stated by this individual are the opinions of this individual and not the opinions of her company, any organization she might be part of, her parrot, or anyone else. ~
Well, I''m pretty sure that this is the code that is causing the leak, because when I commented out this code, the leak did not occur.

I also do not understand what you suggest when you say, "Why do you need (at least) 8 copies of one bitmap?" I have bitmaps for each different piece, how could I use one bitmap for all of those different bitmaps?

I am calling this function in handling my WM_PAINT message. As far as I know it is only getting called when the window needs to be re-drawn (i.e. after it has been maximized, moved, resized, etc.).

Do I need to call DeleteObject() on the bitmaps that I load in this function?
Felisandria noticed that you''re loading the bitmap for white_pawn 8 times. As a result, Windows is most likely loading that image into memory 8 times, but since you keep overwriting the variable dedicated to keeping track of the image''s handle, you only will have the handle to the last instance--1 of those 8 images. And when you try to free from that handle, you have no way of referencing those other 7 instances of the image. Memory leak.

Just out of curiosity, does this app of yours actually draw the pieces, or did you just omit the drawing code from the source you posted? I ask because I was going to recommend that you load all of your images only once and all in one place--perhaps in some function you call when the app starts--and free them all in another single place.

Eventually, yes, those bitmap handles must be deleted. If you''re not doing that, there''s your leak right there. Furthermore, the first time you call SelectObject for a particular device context you need to store the handle it returns, and before deleting that device context you need to call SelectObject with that stored handle. And you need to do this with all of the device contexts you create. Also, don''t forget that you use ReleaseDC on device contexts you obtain with GetDC, and you use DeleteDC with device contexts you create. I''m not saying that you have any or all of these problems; I''m just sharing things that tripped me up a million and one times.
Okay! I think I got it taken care of. I used DeleteObject() to delete the bitmaps that I was loading, and I did not lose any system resources when re-drawing my board over and over again.

To Merlin, yes, I ommited some of the code. Here is the entire code used to draw the board, in case you are interested (it now includes the calls to DeleteObject()). Perhaps you could offer some pointers if you spot anything that I''m not doing correctly. My app is basically a bare bones window with an MDI client and MDI children windows that are created by creating a new game. So far that''s all that is there. All my app does is load up the windows and create a child window when you click on "File > New Game" in the menu. Then it opens up a child window and draws the board using the following two functions. None of the actual chess code has been implemented into this program yet, so the problem doesn''t lie there. I am writing that as a console program first, and will implement it into my GUI after both are complete.

Here''s the code. BTW, I got the second function, DrawTransparentBitmap() from an article on MSDN. I tried to use TransparentBlt() but I got linker errors when I tried to do that. Maybe I need to download a service pack or something for MSVC++ for it to work. Who knows. This function seems to work fine though.

  int border_size = 15;extern HINSTANCE global_instance;#define COL_TO_SCREEN(n) (((7 - n) * 52) + border_size + 2)#define ROW_TO_SCREEN(n) (((7 - n) * 52) + border_size + 4)void draw_board(HDC hdc, HBRUSH light_col, HBRUSH dark_col){	RECT border;	border.top = 0;	border.left = 0;	border.bottom = (52*8) + 30;	border.right = (52*8) + 30;	FillRect(hdc, &border, (HBRUSH)GetStockObject(BLACK_BRUSH));	int x, y;	int color = 1;	for(y = 0; y < 8; y++)	{		for(x = 0; x < 8; x++)		{			RECT rect;			rect.top = (y * 52) + border_size;			rect.left = (x * 52) + border_size;			rect.bottom = ((y * 52) + 52) + border_size;			rect.right = ((x * 52) + 52) + border_size;			if(color == 1)				FillRect(hdc, &rect, dark_col);			else				FillRect(hdc, &rect, light_col);			color *= -1;		}		color *= -1;	}	for(x = 0; x < 8; x++)	{				HBITMAP white_pawn = (HBITMAP)LoadImage((HINSTANCE)global_instance, MAKEINTRESOURCE(WHITE_PAWN), IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION);		DrawTransparentBitmap(hdc, white_pawn, COL_TO_SCREEN(x), ROW_TO_SCREEN(1), RGB(0, 0, 255));		DeleteObject(white_pawn);	}		HBITMAP white_rook = (HBITMAP)LoadImage((HINSTANCE)global_instance, MAKEINTRESOURCE(WHITE_ROOK), IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION);	DrawTransparentBitmap(hdc, white_rook, COL_TO_SCREEN(0), ROW_TO_SCREEN(0), RGB(0, 0, 255));	DrawTransparentBitmap(hdc, white_rook, COL_TO_SCREEN(7), ROW_TO_SCREEN(0), RGB(0, 0, 255));	HBITMAP white_knight = (HBITMAP)LoadImage((HINSTANCE)global_instance, MAKEINTRESOURCE(WHITE_KNIGHT), IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION);	DrawTransparentBitmap(hdc, white_knight, COL_TO_SCREEN(1), ROW_TO_SCREEN(0), RGB(0, 0, 255));	DrawTransparentBitmap(hdc, white_knight, COL_TO_SCREEN(6), ROW_TO_SCREEN(0), RGB(0, 0, 255));	DeleteObject(white_rook);	DeleteObject(white_knight);	}void DrawTransparentBitmap(HDC hdc, HBITMAP hBitmap, short xStart,                           short yStart, COLORREF cTransparentColor)   {   BITMAP     bm;   COLORREF   cColor;   HBITMAP    bmAndBack, bmAndObject, bmAndMem, bmSave;   HBITMAP    bmBackOld, bmObjectOld, bmMemOld, bmSaveOld;   HDC        hdcMem, hdcBack, hdcObject, hdcTemp, hdcSave;   POINT      ptSize;   hdcTemp = CreateCompatibleDC(hdc);   SelectObject(hdcTemp, hBitmap);   // Select the bitmap   GetObject(hBitmap, sizeof(BITMAP), (LPSTR)&bm);   ptSize.x = bm.bmWidth;            // Get width of bitmap   ptSize.y = bm.bmHeight;           // Get height of bitmap   DPtoLP(hdcTemp, &ptSize, 1);      // Convert from device                                     // to logical points   // Create some DCs to hold temporary data.   hdcBack   = CreateCompatibleDC(hdc);   hdcObject = CreateCompatibleDC(hdc);   hdcMem    = CreateCompatibleDC(hdc);   hdcSave   = CreateCompatibleDC(hdc);   // Create a bitmap for each DC. DCs are required for a number of   // GDI functions.   // Monochrome DC   bmAndBack   = CreateBitmap(ptSize.x, ptSize.y, 1, 1, NULL);   // Monochrome DC   bmAndObject = CreateBitmap(ptSize.x, ptSize.y, 1, 1, NULL);   bmAndMem    = CreateCompatibleBitmap(hdc, ptSize.x, ptSize.y);   bmSave      = CreateCompatibleBitmap(hdc, ptSize.x, ptSize.y);   // Each DC must select a bitmap object to store pixel data.   bmBackOld   = (HBITMAP)SelectObject(hdcBack, bmAndBack);   bmObjectOld = (HBITMAP)SelectObject(hdcObject, bmAndObject);   bmMemOld    = (HBITMAP)SelectObject(hdcMem, bmAndMem);   bmSaveOld   = (HBITMAP)SelectObject(hdcSave, bmSave);   // Set proper mapping mode.   SetMapMode(hdcTemp, GetMapMode(hdc));   // Save the bitmap sent here, because it will be overwritten.   BitBlt(hdcSave, 0, 0, ptSize.x, ptSize.y, hdcTemp, 0, 0, SRCCOPY);   // Set the background color of the source DC to the color.   // contained in the parts of the bitmap that should be transparent   cColor = SetBkColor(hdcTemp, cTransparentColor);   // Create the object mask for the bitmap by performing a BitBlt   // from the source bitmap to a monochrome bitmap.   BitBlt(hdcObject, 0, 0, ptSize.x, ptSize.y, hdcTemp, 0, 0,          SRCCOPY);   // Set the background color of the source DC back to the original   // color.   SetBkColor(hdcTemp, cColor);   // Create the inverse of the object mask.   BitBlt(hdcBack, 0, 0, ptSize.x, ptSize.y, hdcObject, 0, 0,          NOTSRCCOPY);   // Copy the background of the main DC to the destination.   BitBlt(hdcMem, 0, 0, ptSize.x, ptSize.y, hdc, xStart, yStart,          SRCCOPY);   // Mask out the places where the bitmap will be placed.   BitBlt(hdcMem, 0, 0, ptSize.x, ptSize.y, hdcObject, 0, 0, SRCAND);   // Mask out the transparent colored pixels on the bitmap.   BitBlt(hdcTemp, 0, 0, ptSize.x, ptSize.y, hdcBack, 0, 0, SRCAND);   // XOR the bitmap with the background on the destination DC.   BitBlt(hdcMem, 0, 0, ptSize.x, ptSize.y, hdcTemp, 0, 0, SRCPAINT);   // Copy the destination to the screen.   BitBlt(hdc, xStart, yStart, ptSize.x, ptSize.y, hdcMem, 0, 0,          SRCCOPY);   // Place the original bitmap back into the bitmap sent here.   BitBlt(hdcTemp, 0, 0, ptSize.x, ptSize.y, hdcSave, 0, 0, SRCCOPY);   // Delete the memory bitmaps.   DeleteObject(SelectObject(hdcBack, bmBackOld));   DeleteObject(SelectObject(hdcObject, bmObjectOld));   DeleteObject(SelectObject(hdcMem, bmMemOld));   DeleteObject(SelectObject(hdcSave, bmSaveOld));   // Delete the memory DCs.   DeleteDC(hdcMem);   DeleteDC(hdcBack);   DeleteDC(hdcObject);   DeleteDC(hdcSave);   DeleteDC(hdcTemp);   }   
What Merlin said. Also... hmm. How to explain this...

Don''t load bitmaps in the middle of your draw code. Ever. Load them once, and keep a handle to them as a global or member. Partially because it''s really slow to keep loading them like that. Partially because it usually causes memory leaks trying to keep track of them all.

You only need one copy of a bitmap loaded, ever. That''s the point of having a handle to it... you can keep using that bitmap wherever you want. Loading it 8 times doesn''t do anything but waste resource handles. Loading it then deleting it 8 times doesn''t do anything other than waste time. With the knowledge that you were using that code to respond to WM_PAINT, it was probably more like 32,000 handle allocations, no wonder you ran out of resources. Think of it as a rubber stamp... once you''ve got the rubber stamp you can stamp tons of things with it. You don''t need a new rubber stamp every time you want to stamp something.

So, make an init function and load up your bitmaps there, then just blit the bitmaps when you''re drawing.

-fel
~ The opinions stated by this individual are the opinions of this individual and not the opinions of her company, any organization she might be part of, her parrot, or anyone else. ~
You''ll also experience a significant speed boost if you do what fel and I suggested: doing all of your image loading in a single place, namely some function called when your app starts (and, of course, make a corresponding function for cleanup). So, just pull that initialization and cleanup stuff out of there, and I think you''ve got some good stuff there. Are you going to post a link or something so some of us can try this app?
Update:

I have it working without any memory leaks as far as I can tell. I also implemented a sort of double buffering, by drawing everything to an offscreen device context and blitting it all at once. My app was drawing all of the piece bitmaps one by one, and after I got all 32 pieces on the board, it was pretty noticable that each piece was being drawn one by one. Now there is a slight delay before the pieces and board are drawn, but it draws it in one swoop. I think I might be able to optimize my double buffering method, by making the memory device context a global (I''m re-initializing it everytime currently). I made one attempt at getting it to work with a global memory device context and bitmap handles (for the offscreen DC), but it didn''t work, and it''s 4AM, so I didn''t feel like giving it much more thought tonight. I also might be able to optimize the DrawTransparentBitmap() function that I got from MSDN.

I''ll try to upload something tomorrow and post the link here. I don''t have a webpage at the moment, but I can get a free one and uplaod my program.

Thanks for both of your help on my program. I really appreciate it.

I''m glad to hear that things are working out. I look forward to seeing your work, so keep it up.

This topic is closed to new replies.

Advertisement