Jump to content

  • Log In with Google      Sign In   
  • Create Account


Memory management, mainly using SDL


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
17 replies to this topic

#1 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 06 November 2012 - 05:24 AM

Hello!

I'm currently trying to reduce the memory leak my program is causing and the process has me kind of dumbfounded.

I understand that everything you create using "new" you have to delete eventually so that there is no memory leak. The same goes for temporary pointers (right)?

Anyway I've located a part of my code that is leaking, to no surprise it's where i update one of the UI elements in my game. I'm going to simplify the code here because the question isn't about only that code but rather how i handle a situation like this (i have loads of them in my code at the moment).

So here goes:
void update(int timer)
{
	 //this is my update function which updates this certain element
	 //i have a reference to my loader class already loaded in, it's called imageLoader in this example. imageLoader stores all of the loaded
	 //images and can return a pointer to a certain image on function calls.

	 SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");
	 //tempSurface now stores a pointer to the image that is returned from loadImg();

	 SDL_BlirSurface(tempSurface, NULL, screen, NULL);
	 //now I've used the img and the local tempSurface is useless

	 //Now, if tempSurface only was a int* i could have just ignored it since it's just a pointer (right)?
	 //but as i understands it a SDL_Surface creates other pointers within itself that must be destroyed, and if i call
	 //SDL_FreeSurface(tempSurface); i will destroy the surface that it's pointing to, creating problems for other classes.
		  
	 //memory is leaking here, and my guess is on the "tempSurface" which i am unable to destroy without killing it's reference!!!
}

How do i make sure i free all the memory possible without corrupting/deleting the reference?

I want to make sure that i have this same problem with TTF_Font, which is part of a "sub-library" to SDL.

I really appreciate any help i can get! :)

Edited by Tallkotten, 06 November 2012 - 05:25 AM.


Sponsor:

#2 BeerNutts   Crossbones+   -  Reputation: 2813

Like
1Likes
Like

Posted 06 November 2012 - 11:28 AM

Hi Tallkotten,

Is imageLoader loading/allocating the image every time (ie calling SDL_LoadImage or whatever)? That would be the only way you would be leaking. However, I would assume imageLoader keeps track of which images have been loaded and it just returns a pointer to that surface. In that case, you should NOT be calling SDL_FreeImage. Only the imageLoader should ever call SDL_FreeImage, and only then when the user explicitly tells it to.

You are also incorrect about the int*. An int* is a pointer to an int, but if it was created with a new or malloc, then it must be delete'd or free'd, otherwise it is still taking up room on the heap.

You should not be generating new SDL_Surface's for each update. A good solution is to create an ImageManager, which handles all the images you need. Something like this:

// in TImageManager.h
class TImageManager {
private:
  // this maps the filename to a pre-defined SDL_Surface
  std::map<std::string, SDL_Surface*> ImageMap;

public:
  SDL_Surface* GetImage(std::string imageFileName);
};

// in TImageManager.cpp
TImageManager::GetImage(std::string imageFileName)
{
  // check if this image has already been loaded
  if (ImageMap.find(imageFilename) == ImageMap.end()) {
    // It hasn't been loaded, load it here and store in map
    ImageMap[imageFileName] = SDL_LoadImage(imageFileName.c_str()); // or whatever the SDL function name is
  }

  return ImageMap[imageFileName];
}

My Gamedev Journal: 2D Game Making, the Easy Way

---(Old Blog, still has good info): 2dGameMaking
-----
"No one ever posts on that message board; it's too crowded." - Yoga Berra (sorta)

#3 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 06 November 2012 - 12:05 PM

Hello BeerNutts!

My loader class is essentially a imageManager since it does basically everything you wrote down in that example of yours. It also doesn't load a new image each time, it keeps track of what has been loaded and returns a pointer if it exists.

Why i chose to allocate a new SLD_Surface in the example is because in a few of my line of code maybe 4 things were going to use the same image (within the same func) i found it easier to create a new variable instead of typing that lengthy "imageLoader.loadImg("example_image.png")" each time. Maybe that is the wrong way to go :/

Instead of doing either of the above maybe i should just do what you said, load all the images i need to display in that class globally within the class so that i never loads something new.

This rises up another problem thought, this is a quick example (i think i got this but i just want to be sure);

class.h
#include "loader.h"

class Example
{
	 public:
		  draw(SDL_Surface* screen);		

	 private:
		  SDL_Surface* imageToUseInClass;
		  Loader imageLoader;
}

class.cpp
void Example::draw(SDL_Surface* screen)
{
	 //for a shorter example i update the surface in the draw func, will have a special function for this in the real program
	 //if i want to change a surface that is global within the class this would be a way of doing it, right?
	 //I first try to load the new image and then if it succeeds i free the memory from the last one and eventually change the image while i
	 //free the temporary loaded img. This should free all the memory that is needed and wont result in a memory leak.

	 SDL_Surface* newimage = imageLoader.load("image.png");
	 if(newImage != NULL)
	 {
		  SDL_FreeSurface(imageToUseInClass);
		  imageToUseInClass = newImage;
		  SDL_FreeSurface(newImage);
	 }
	 else
	 {
		  SDL_FreeSurface(newImage);
	 }


	 SDL_BlitSurface(imageToUseInClass, NULL, screen, NULL);
}


#4 BeerNutts   Crossbones+   -  Reputation: 2813

Like
0Likes
Like

Posted 06 November 2012 - 02:08 PM

//for a shorter example i update the surface in the draw func, will have a special function for this in the real program
//if i want to change a surface that is global within the class this would be a way of doing it, right?
//I first try to load the new image and then if it succeeds i free the memory from the last one and eventually change the image while i
//free the temporary loaded img. This should free all the memory that is needed and wont result in a memory leak.


Firstly, ImageLoader should be a singular entity, there should not be multiple imageLoaders roaming around. The ImageLoader (what i call ImageManager) needs to be created once, and then given to other classes. Or, you could not even go the class route, and just use a normal function LoadImage(), which checks a statically held list of images; otherwise, you're going to have multiple images being loaded when you should only have one. This is actually what I do in my games; I have a Utilities file that is just functions for different things, and not wrapped in a class.

Second, No, you should never be freeing a surface outside of your imageLoader class. That's going to cause problems. The imageLoader class should be the only one ever doing loads and free or surfaces. If you know an image won't be used anymore in your program, you can call imageLoader->Deallocate(imageName), and have it free it and remove it from it's list of open images.

Third, your example class is wrong:
         if(newImage != NULL)
		 {
				  SDL_FreeSurface(imageToUseInClass);
				  imageToUseInClass = newImage;
				  SDL_FreeSurface(newImage);
		 }
		 else
		 {
				  SDL_FreeSurface(newImage);
		 }
You cannot assign the pointer to a surface, and then Free the surface; the pointer imageToUseInClass now points to memory which ash been freed, and any access to it will be invalid.

Also, If newImage is NULL, you are trying to Free the NULL surface, which is bad.
My Gamedev Journal: 2D Game Making, the Easy Way

---(Old Blog, still has good info): 2dGameMaking
-----
"No one ever posts on that message board; it's too crowded." - Yoga Berra (sorta)

#5 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 06 November 2012 - 03:03 PM

Firstly, ImageLoader should be a singular entity, there should not be multiple imageLoaders roaming around. The ImageLoader (what i call ImageManager) needs to be created once, and then given to other classes. Or, you could not even go the class route, and just use a normal function LoadImage(), which checks a statically held list of images; otherwise, you're going to have multiple images being loaded when you should only have one. This is actually what I do in my games; I have a Utilities file that is just functions for different things, and not wrapped in a class.


I dont agree here. I have different loaders for different stuff. Like a loader for the UI and another one to load player related stuff, i never load an image twice thought.

At first i explored a few ways of doing this and the static solution was definitely on my mind for a long time but i ended up doing this instead after taking some advice from people.

Second, No, you should never be freeing a surface outside of your imageLoader class. That's going to cause problems. The imageLoader class should be the only one ever doing loads and free or surfaces. If you know an image won't be used anymore in your program, you can call imageLoader->Deallocate(imageName), and have it free it and remove it from it's list of open images.


I see the problem it can cause, but i am truly out of clues why my program would be leaking memory if it isn't that i didn't free the surfaces.

Third, your example class is wrong:

		 if(newImage != NULL)
		 {
				  SDL_FreeSurface(imageToUseInClass);
				  imageToUseInClass = newImage;
				  SDL_FreeSurface(newImage);
		 }
		 else
		 {
				  SDL_FreeSurface(newImage);
		 }
You cannot assign the pointer to a surface, and then Free the surface; the pointer imageToUseInClass now points to memory which ash been freed, and any access to it will be invalid.

Also, If newImage is NULL, you are trying to Free the NULL surface, which is bad.


That is true... Then this goes back to my original problem, how do i free the surface newImage without screwing imageToUseInClass up? Because it's situations like this that is leaking memory (i'm guessing, couldn't be anything else).

#6 BeerNutts   Crossbones+   -  Reputation: 2813

Like
0Likes
Like

Posted 06 November 2012 - 04:06 PM

It's obvious you don't understand the point of an image manager if you are trying to free an image outside of it, and yet you want to disagree with me on having multiple instances of it (which defeats the purpose of having a resource manager in the first place!); I guess you have to make your own mistakes to learn them.

If it is truly the function you showed is causing a leak (BTW, how did you find out it's causing a memory leak?), then you have not written your imageloader properly; it MUST be loading the same image over and over, because this is all you're doing:
SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");

SDL_BlirSurface(tempSurface, NULL, screen, NULL);

Which will not leak any memory, again, assuming imageLoader.loadImg() is written correctly.
My Gamedev Journal: 2D Game Making, the Easy Way

---(Old Blog, still has good info): 2dGameMaking
-----
"No one ever posts on that message board; it's too crowded." - Yoga Berra (sorta)

#7 ultramailman   Prime Members   -  Reputation: 1563

Like
0Likes
Like

Posted 06 November 2012 - 06:25 PM

Hello
I think freeing surfaces should only be done when you are absolutely sure it will never be used again. Are you really going to use this example_image.png once? If you are sure, then the correct way to do it is to use the complement of imageLoader.loadImg

imageLoader.unloadImg("example_image.png");

If you don't have that function for your loader class, then you need to write one, because maker and unmaker functions should always come in pairs. It should undo whatever imageLoader.loadImg did to change your loader object, and deallocates whatever was allocated by imageLoader.loadImg.

Examples of such pairs: new and delete, new[] and delete[], malloc and free, IMG_Load and SDL_FreeSurface.

What you should never do is mixing these maker and unmaker functions. That means something created by new should not be destroyed by free, it has to be done by delete. A surface created IMG_Load should not be destroyed by free either, it should be done by SDL_FreeSurface. Likewise, something gotten from imageLoader.loadImg should not be destroyed by SDL_FreeSurface, it should only be done by imageLoader.unloadImg.

SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");
//tempSurface now stores a pointer to the image that is returned from loadImg();
.......
//Now, if tempSurface only was a int* i could have just ignored it since it's just a pointer (right)?


That's not quite right. Both SDL_Surface* and int* are pointers. That means tempSurface is as much a pointer as any int*

How do i make sure i free all the memory possible without corrupting/deleting the reference?


There aren't any references in your code, only pointers D:

But to answer your question, one way to prevent memory leaks is to unload all the surfaces you've loaded when the program ends.

#8 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 07 November 2012 - 07:58 AM

If it is truly the function you showed is causing a leak (BTW, how did you find out it's causing a memory leak?), then you have not written your imageloader properly; it MUST be loading the same image over and over, because this is all you're doing:

SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");

SDL_BlirSurface(tempSurface, NULL, screen, NULL);


I know it's a memory leak because i am tracking the resources it uses with windows resource monitor. All i see is a steady rise of KB used until it has eaten up most of the memory and crashes.

I agree with you that the code you wrote shouldn't cause any memory loss, but it still does (or i am just missing something mayor). Take a look at this code, this is causing me some memory leak.

enemy.h
class Enemy
{
	 public:
		  
		  draw();			   //draws everything including hpNumber
		  hpInfo();		  //updates the graphics for the HP to be displayed

	 private:
		  
		  SDL_Surface hpNumber;
		  SDL_Rect hpNumberPos;

}

a function in enemy.cpp
void Enemy::hpInfo()
{
	 //I load the font and color that I've saved in the respective loaders. These are references to loaders i pass around to any class that need
	 //to use them
	 TTF_Font* sFont8B = const_cast<TTF_Font*>(m_fontLoader.loadFont("sfont8b"));
	 const SDL_Color textWhite = m_colorLoader.loadColor("white");
	  
	 //create a string from a int, just takes an int and return a normal string
	 string hpS = mEntity.gEntity.intToString(m_currentHp, false);

	 //i update the Surface with the new text to be shown, i have to change the string to a Char* to use the funcion
	 hpNumber = TTF_RenderText_Solid(sFont8B, mEntity.gEntity.stringToChar(hpS,"",""), textWhite);

	 //calculate how wide and high the text is
	 TTF_SizeText(sFont8B, mEntity.gEntity.stringToChar(hpS,"",""), &w, &h);

	 //assing the new position based on the With and Height of the text written.
	 SDL_Rect temp5 = {200,m_nameBox.x+m_nameBox.h+10,w,h};
	 m_hpBox = temp5;

}

now... if i free the surface before i assign a new surface to it the memory loss is lower. I know that this time the image isn't loaded from a imageManager but still worth noting.

I then assume that the decrease in memory stacking up (while doing nothing in the game) is a sign that the surface is no longer causing any memory loss. The memory still leaking must then be caused by something else, and my only guess is the Font.

In case you want to see my loadFont and loadImg functions:

const SDL_Surface* Loader::loadImg(const string name)
{
	//check if the file exists, if it does return it
	map<string, SDL_Surface*>::const_iterator it = m_loadedImages.find(name);
	if(it != m_loadedImages.end())
	{
		return it->second;
	}
	SDL_Surface* surface = NULL;
	//else load the file
	string loadPath = m_prePath + name;
	surface = lEntity.loadImage(loadPath.c_str(), true);
	if(surface == NULL)
	{
		//load a default texture if that fails?
	}
	//put the loaded img in the map
	m_loadedImages.insert(pair<string, SDL_Surface*>(name, surface));

	return surface;
}


const TTF_Font* Loader::loadFont(const string name)
{
	//check if the file exists, if it does return it
	map<string, TTF_Font*>::const_iterator it = m_loadedFonts.find(name);
	if(it != m_loadedFonts.end())
	{
		return it->second;
	}

	 //if not, return NULL
	 return NULL;
}

This is my first time ever working with memory management since this is also my first "real" project ever. So this is totally new territory for me and i am completely stuck since i cant seem to figure out what is wrong.

#9 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 07 November 2012 - 08:03 AM

There aren't any references in your code, only pointers D:


Yeah, my mind were some place else.

I think freeing surfaces should only be done when you are absolutely sure it will never be used again. Are you really going to use this example_image.png once? If you are sure, then the correct way to do it is to use the complement of imageLoader.loadImg


This is true and i plan on adding this function. But i am planning on writing a system which takes care of what is in use and what's not so i am waiting with this. There really aren't many images now to load either so the unload functions isn't needed at the moment.

Thanks for your reply!

#10 Karsten_   Members   -  Reputation: 1575

Like
0Likes
Like

Posted 07 November 2012 - 01:10 PM

Hi, if you are using C++, perhaps you can use a smart pointer for this (These two lines looks ugly as sin, but provides memory safety I have yet to see with any other language / platform)...

std::tr1::shared_ptr<SDL_Surface> surface;
surface.reset(SDL_LoadImage(imageFileName.c_str()), std::tr1::bind(SDL_FreeSurface, std::tr1::placeholders::_1));
// Now you can do whatever and RAII will take care of the memory management.

Remember: You must set the deleter to be SDL_FreeSurface, because otherwise (and in std::auto_ptr) the c++ delete will just be called, which will cause undefined behaviour with things not created via new.

I generally do this with OpenGL textures but there is no reason this wont work for SDL textures.

If you are not using the very latest compiler you will need to include the technical report 1 versions of memory and functional.
#include <tr1/functional>
#include <tr1/memory>

Edited by Karsten_, 07 November 2012 - 01:15 PM.

Mutiny - Open-source C++ Unity re-implementation.
Defile of Eden 2 - FreeBSD and OpenBSD binaries of our latest game.


#11 ultramailman   Prime Members   -  Reputation: 1563

Like
0Likes
Like

Posted 07 November 2012 - 07:46 PM

Hello again
You are sure this snippet is leaking.
SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");
SDL_BlirSurface(tempSurface, NULL, screen, NULL);

Does that mean loadImg creates a new surface pointer everytime you call
imageLoader.loadImg("example_image.png");
? If so, then you really have to free that surface via imageLoader.unload as soon as you don't need it (i.e. in the update(int) function), because as soon as you exit that update function, you lose access to that surface. If however I guessed wrong, and your loader does cache the surface pointers, then it's all cool. However you did observe steadly growing memory usage, so I'm pretty sure you are creating a new copy with every call to the loader.

#12 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 08 November 2012 - 03:16 AM

Hi, if you are using C++, perhaps you can use a smart pointer for this (These two lines looks ugly as sin, but provides memory safety I have yet to see with any other language / platform)...

std::tr1::shared_ptr<SDL_Surface> surface;
surface.reset(SDL_LoadImage(imageFileName.c_str()), std::tr1::bind(SDL_FreeSurface, std::tr1::placeholders::_1));
// Now you can do whatever and RAII will take care of the memory management.

Remember: You must set the deleter to be SDL_FreeSurface, because otherwise (and in std::auto_ptr) the c++ delete will just be called, which will cause undefined behaviour with things not created via new.

I generally do this with OpenGL textures but there is no reason this wont work for SDL textures.


Yes that is one option i might go with later on when i add the manager system. Like i described earlier i dont really have to unload images at the moment since i have so few loaded.

But i'll definitely consider it!

If you are not using the very latest compiler you will need to include the technical report 1 versions of memory and functional.
#include <tr1/functional>
#include <tr1/memory>


How do i know if i have to do that or not? And what difference does it make?

I am running codeblocks

#13 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 08 November 2012 - 04:18 AM

I just found out where one of the leaks were located... In one of my functions where i turn a string to a char* array i never used delete after new...

Thanks everyone! Hopefully i wont get stuck on the rest of the code (that i have yet to check for leaks :)).

#14 Karsten_   Members   -  Reputation: 1575

Like
0Likes
Like

Posted 09 November 2012 - 10:09 AM

How do i know if i have to do that or not? And what difference does it make?


If you get a compiler error stating that std::tr1::shared_ptr does not exist, then you will need to include the tr1 versions of the header.

If you are using the CodeBlocks IDE, that probably means you are using GCC/G++ as your compiler, so it is quite likely you will need to include them.

Mutiny - Open-source C++ Unity re-implementation.
Defile of Eden 2 - FreeBSD and OpenBSD binaries of our latest game.


#15 Álvaro   Crossbones+   -  Reputation: 12920

Like
1Likes
Like

Posted 09 November 2012 - 12:41 PM

So why are you using `new' and `delete' for things like a "char * array"? You probably should be using std::vector or std::string instead of using new to allocate arrays. It's much harder to have a memory leak if you don't use `new'.

#16 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 09 November 2012 - 12:58 PM


How do i know if i have to do that or not? And what difference does it make?


If you get a compiler error stating that std::tr1::shared_ptr does not exist, then you will need to include the tr1 versions of the header.

If you are using the CodeBlocks IDE, that probably means you are using GCC/G++ as your compiler, so it is quite likely you will need to include them.


Ok thanks! Yes i am using GCC/G++ :)

So why are you using `new' and `delete' for things like a "char * array"? You probably should be using std::vector or std::string instead of using new to allocate arrays. It's much harder to have a memory leak if you don't use `new'.


Well that is old code and when i looked up how to do it that was one of the only ways. Also TTF_RenderText_Solid requires a char*. http://www.libsdl.org/projects/SDL_ttf/docs/SDL_ttf_43.html

#17 SiCrane   Moderators   -  Reputation: 9559

Like
1Likes
Like

Posted 09 November 2012 - 01:00 PM

No, it requires a const char *, which you can from a std::string with the .c_str() member function.

#18 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 09 November 2012 - 04:40 PM

No, it requires a const char *, which you can from a std::string with the .c_str() member function.


Well... f*ck me...

I've used that to some extent but now i just feel silly... Is it even worth going back and using that instead of my newly implemented system?




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS