Followers 0

# Memory management, mainly using SDL

## 17 posts in this topic

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:
[CODE]
void update(int timer)
{
//this is my update function which updates this certain element
//images and can return a pointer to a certain image on function calls.

//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!!!
}
[/CODE]

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
0

##### Share on other sites
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:

[code]
// 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)
{
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];
}
[/code]
1

##### Share on other sites
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
[CODE]

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

private:
SDL_Surface* imageToUseInClass;
}
[/CODE]

class.cpp
[CODE]
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.

if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}

SDL_BlitSurface(imageToUseInClass, NULL, screen, NULL);
}
[/CODE]
0

##### Share on other sites
[quote]
//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.
[/quote]

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:
[code]
if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}
[/code]
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.
0

##### Share on other sites
[quote name='BeerNutts' timestamp='1352232496' post='4998178']
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.
[/quote]

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.

[quote name='BeerNutts' timestamp='1352232496' post='4998178']
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.

[/quote]

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.

[quote name='BeerNutts' timestamp='1352232496' post='4998178']
Third, your example class is wrong:
[code]
if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}
[/code]
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.
[/quote]

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).
0

##### Share on other sites
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:
[code]

SDL_BlirSurface(tempSurface, NULL, screen, NULL);
[/code]

Which will not leak any memory, again, assuming imageLoader.loadImg() is written correctly.
0

##### Share on other sites
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

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.

[quote name='Tallkotten' timestamp='1352201082' post='4997979']
[CODE]
//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)?
[/CODE]
[/quote]

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

[quote name='Tallkotten' timestamp='1352201082' post='4997979']
How do i make sure i free all the memory possible without corrupting/deleting the reference?
[/quote]

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.
0

##### Share on other sites
[quote name='BeerNutts' timestamp='1352239582' post='4998219']
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:
[code]

SDL_BlirSurface(tempSurface, NULL, screen, NULL);
[/code]
[/quote]

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
[CODE]
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;

}
[/CODE]

a function in enemy.cpp
[CODE]
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

//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;

}
[/CODE]

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.

[CODE]
{
//check if the file exists, if it does return it
{
return it->second;
}
SDL_Surface* surface = NULL;
string loadPath = m_prePath + name;
if(surface == NULL)
{
//load a default texture if that fails?
}
//put the loaded img in the map

return surface;
}

{
//check if the file exists, if it does return it
{
return it->second;
}

//if not, return NULL
return NULL;
}
[/CODE]

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.
0

##### Share on other sites
[quote name='ultramailman' timestamp='1352247935' post='4998277']
There aren't any references in your code, only pointers D:
[/quote]

Yeah, my mind were some place else.

[quote name='ultramailman' timestamp='1352247935' post='4998277']
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
[/quote]

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.

0

##### Share on other sites
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)...

[code]
std::tr1::shared_ptr<SDL_Surface> surface;
// Now you can do whatever and RAII will take care of the memory management.
[/code]

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

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_
0

##### Share on other sites
Hello again
You are sure this snippet is leaking.
[CODE]
SDL_BlirSurface(tempSurface, NULL, screen, NULL);
[/CODE]

0

##### Share on other sites
[quote name='Karsten_' timestamp='1352315450' post='4998532']
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)...

[code]
std::tr1::shared_ptr<SDL_Surface> surface;
// Now you can do whatever and RAII will take care of the memory management.
[/code]

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

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

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!

[quote name='Karsten_' timestamp='1352315450' post='4998532']
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>
[/quote]

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

I am running codeblocks
0

##### Share on other sites
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 ).
0

##### Share on other sites
[quote name='Tallkotten' timestamp='1352366168' post='4998799']
How do i know if i have to do that or not? And what difference does it make?
[/quote]

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.
0

##### Share on other sites
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'.
1

##### Share on other sites
[quote name='Karsten_' timestamp='1352477385' post='4999333']
[quote name='Tallkotten' timestamp='1352366168' post='4998799']
How do i know if i have to do that or not? And what difference does it make?
[/quote]

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.
[/quote]

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

[quote name='Álvaro' timestamp='1352486514' post='4999377']
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'.
[/quote]

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
0

##### Share on other sites
No, it requires a [i]const[/i] char *, which you can from a std::string with the .c_str() member function.
1

##### Share on other sites
[quote name='SiCrane' timestamp='1352487659' post='4999386']
No, it requires a [i]const[/i] char *, which you can from a std::string with the .c_str() member function.
[/quote]

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?
0

## Create an account

Register a new account