How to resolve small memory leak when using SDL_CreateRGBSurface in a particular context?

[electrosys]

I’m seeing what appears to be a very small memory leak when I call s = SDL_CreateRGBSurface(0, 1, 1, 32, 0, 0, 0, 0); repeatedly. If I call s = SDL_CreateRGBSurface(0, 0, 0, 32, 0, 0, 0, 0) there is no leak, but also I don’t see the surface I need either, which is what we would expect with a 0 height and 0 width surface.

I have a Sprite object which contains a Timer object and the Timer object contains a ProgressBar object. The ProgressBar uses a surface to draw a color over the same area as the Sprite. This is basically a debug feature I’m using to track the progress of a timer.

When I create a Sprite/Timer/ProgressBar object over and over again I see this leak.

I create a Sprite below and I only see the leak if I recreate hundreds of sprites. Also note that each time I create a new sprite I’m seeing the destructor for Sprite getting called so I’m assuming the old surface is getting cleaned up each time.

Any ideas on why I would see a leak in this case?

Please also let me know if you need more context.

spriteBackground = ley::Sprite(TextureManager::Instance()->getTexture(background_level.c_str()), 0, {start_rect});


ley::Sprite::Sprite()
:
Renderable(),
fader(1000,{0,0,0,0}) {
    SDL_Log("Sprite() called");
}

ley::Sprite::Sprite(SDL_Texture * t, unsigned int s, std::vector<SDL_Rect> v)
: 
Renderable(),
animSpeed(s),
texture(t),
fader{1000,{0,0,0,0}} {

    SDL_Log("Sprite(SDL_Texture * t, unsigned int s, std::vector<SDL_Rect> v) called");

    if (!texture) {
        return;//EARLY EXIT
    }

    bool multiframe = false;
    
    if(v.size() > 0) {
        fader(1000,{v.at(0)}); // fader is a Timer
    }

    multiframe = v.size() > 1;
    
    SDL_Rect source_rect;
    dest_rect.y = source_rect.y = 0;
    if(multiframe) {
        for(auto rect : v) {
            source_rect.x = rect.x;
            source_rect.y = rect.y;
            source_rect.w = dest_rect.w = rect.w;
            source_rect.h = dest_rect.h = rect.h;
            frames.push_back(source_rect);
        }
    } else {
        SDL_QueryTexture(texture, NULL, NULL, &source_rect.w, &source_rect.h);
        dest_rect.w = source_rect.w;
        dest_rect.h = source_rect.h;
        dest_rect.x = source_rect.x = 0;
    }
}

ley::Timer::Timer(unsigned int m, SDL_Rect rect) 
: Renderable(),
mili(m),
active(true), 
expired(false), 
sdlTimerReady(true),
progressBar(rect) {
    
    if(SDL_Init(SDL_INIT_TIMER) >= 0) {
        // if all good
    } else {
        printf("Can't Initialize SDL2 Timer");
        sdlTimerReady = 0;
    }
}

ley::ProgressBar::ProgressBar(SDL_Rect rect)
: Renderable(),
border(rect), 
progress(border) { 

  s = SDL_CreateRGBSurface(0, 1, 1, 32, 0, 0, 0, 0); //This seems to cause the leak
  SDL_FillRect(s, NULL, SDL_MapRGB(s->format, 255, 0, 0));

}

ley::ProgressBar::~ProgressBar() {

  SDL_DestroyTexture(t);
  SDL_Log("~ProgressBar() dtor called");
}

[Peter87]

Do you ever call SDL_FreeSurface to free the surface that you have created?

[electrosys]

I have tried using SDL_FreeSurface(). If I add SDL_FreeSurface() to the destructor of the ProgressBar I get an exception in the ProgressBar::render() method for some reason. How is it possible that ProgressBar::render() is being called after the destructor for ProgressBar has already been called?

Note that when render() is called with an invalid surface, the surface seems to be mostly valid except for the format which is 0x0.

ley::ProgressBar::~ProgressBar() {

  SDL_FreeSurface(s);
  SDL_DestroyTexture(t);
  SDL_Log("~ProgressBar() dtor called");
}

void ley::ProgressBar::render(SDL_Renderer* r) {

  SDL_SetRenderDrawColor(r, 255, 255, 255, SDL_ALPHA_OPAQUE);
  SDL_RenderDrawRect(r, &border);

  //If texture is empty, populate it the first time we have the rendering context available
  
  if(!t) {
     t = SDL_CreateTextureFromSurface(r,s); //Exception here when calling SDL_FreeSurface() from ~ProgressBar() destructor
  }

  //Render the texture to the portion of progress bar we want to fill in.
  SDL_RenderCopy(r, t, NULL, &progress);
}

[Peter87]

@electrosys

How is it possible that ProgressBar::render() is being called after the destructor for ProgressBar has already been called?

My guess is that you accidentally create a copy of your ProgressBar object. When one object is destroyed it will free the surface which means that the other object ends up with a dangling surface pointer.

To avoid such issues you should either implement proper copy operations for your class (see rule of three/five) or disable copying by declaring the copy constructor and copy assignment operator as deleted.

class ProgressBar
{
	...
	ProgressBar(const ProgressBar&) = delete;
	ProgressBar& operator=(const ProgressBar&) = delete;
};

---

I wonder if there is a reason why you hold on to the surface and don’t create the texture in the constructor? I think it would simplify your code if the surface was not a member of the class and you just created the surface, loaded the texture and then freed the surface all in the constructor. Note that there is SDL_RenderDrawRect which allow you to draw a rectangle directly with the renderer so if you just want to draw a rectangle of a single colour you probably don’t even need a texture.

[electrosys]

Thank you for this information, its really useful. I was starting to discover some of this information around rule of three/five after seeing this article c++ - Freeing a valid SDL surface results in segfault - Stack Overflow . Your confirmation helps a lot.

I don’t create the texture in the constructor because I don’t have a rendering context in the constructor. The rendering context is passed into the ProgressBar object on the first call to ProgressBar::render().

I agree that I can likely simplify this by creating and freeing the surface in the same method, possibly the constructor as you suggest. I’m holding on to the surface as I thought it would be more efficient as I only need to create the surface once to create the texture. I can also look into simply drawing a filled rectangle without a texture as well. I have the propensity to explore all avenues though and hope to learn more about the assignment operator.

In an effort to learn more about the assignment operator I can see that creating my own assignment operator clears up the segfault and allows me to call SDL_FreeSurface in the ~ProgressBar() destuctor which in turn clears up the memory leak.

Do you know why I can not copy the texture in my user defined assignment method the same way I copy the surface?

Also note that I see an exception on SDL_FreeSurface() when I exit the program now.

ley::ProgressBar::~ProgressBar() {

  SDL_FreeSurface(s); //This causes an exception when I exit the program now.
  s = nullptr;
  SDL_DestroyTexture(t);
  t = nullptr;
  SDL_Log("~ProgressBar() dtor called");
}

ley::ProgressBar & ley::ProgressBar::operator= (const ProgressBar &pb) {
  
  if(this != &pb) {
    border = pb.border;
    progress = pb.progress; 
    *s = *(pb.s);
    //*t = *(pb.t); //Why can't I copy the texture the same way I copy the surface here?
  }

  return *this;
}

I feel like I’m learning something new and significant here. So far this has been a very valuable thread.

[Peter87]

@electrosys

Do you know why I can not copy the texture in my user defined assignment method the same way I copy the surface?

Probably because SDL_Texture is an opaque type but the way you copy the surface is not correct either.

Note that SDL is a C library so the SDL types don’t have constructors, destruction or any custom assignment operators. That’s why they provide Create and Free/Destroy functions for many types instead.

The way you copy the SDL_Surface will just do a member-wise copy, as if you had written:

s->flags = pb.s->flags;
s->format = pb.s->format;
s->w = pb.s->w;
s->h = pb.s->h;
s->pitch = pb.s->pitch;
s->pixels = pb.s->pixels;
And so on...

This is not going to work because some of them are pointers so you will end up with two surfaces pointing to the same stuff. This would also be a memory leak because you lose track of the things that the *s pointed to before.

The simplest way I know to copy a SDL_Surface is to create a new one with SDL_CreateRGBSurface and then use SDL_BlitSurface to copy the content.

It’s possible to have multiple objects share the same surface, either by incrementing the surface’s refcount field or by using std::shared_ptr, but that essentially means if one object modifies the surface it will affect both objects. If the surface is never modified this is not a problem. Otherwise you would have to do copy-on-write.

This is getting quite complicated. Maybe it’s time to take a step back and ask yourself if you really need ProgressBar to be copyable? Wasn’t the copying of it just a mistake on your part and it would have been better to just make the class uncopyable (by declaring the copy constructor and copy assignment operator as deleted as I described above) and modify the code so that it doesn’t try to copy the objects? I think so. Copying surfaces and textures is normally not something you want to do unless necessary. You could of course implement these things as an exercise if you want but it’s perhaps not the easiest thing to start with.

[electrosys]

1.) Using only SDL_RenderDrawRect() and SDL_RenderFillRect() and omitting the surface and texture is probably the way to go. In this particular case I’m only drawing a solid color rect and don’t need a surface or texture anyway.

void ley::ProgressBar::render(SDL_Renderer* r) {

  SDL_SetRenderDrawColor(r, 255, 255, 255, SDL_ALPHA_OPAQUE);
  SDL_RenderDrawRect(r, &border);
  SDL_SetRenderDrawColor(r, 255, 0, 0, SDL_ALPHA_OPAQUE);
  SDL_RenderFillRect(r, &progress);

}

2.) If I really need a texture I can simply create and free the surface in the same method that initializes the texture.

void ley::ProgressBar::render(SDL_Renderer* r) {

  SDL_SetRenderDrawColor(r, 255, 255, 255, SDL_ALPHA_OPAQUE);
  SDL_RenderDrawRect(r, &border);

  //If texture is empty, populate it the first time we have the rendering context available
  if(!t) {
    SDL_Surface* s = SDL_CreateRGBSurface(0, 1, 1, 32, 0, 0, 0, 0); //This seems to cause the leak
    SDL_FillRect(s, NULL, SDL_MapRGB(s->format, 255, 0, 0));
    t = SDL_CreateTextureFromSurface(r,s);
    SDL_FreeSurface(s);
  }

  //Render the texture to the portion of progress bar we want to fill in.
  SDL_RenderCopy(r, t, NULL, &progress);

}

ley::ProgressBar::~ProgressBar() {

  SDL_DestroyTexture(t);
  t = nullptr;
}

3.) Updating the assignment operator to this certainly resolves the exception I was seeing when the program was exiting.

ley::ProgressBar & ley::ProgressBar::operator= (const ProgressBar &pb) {
  
  if(this != &pb) {
    border = pb.border;
    progress = pb.progress;
    s = SDL_CreateRGBSurface(0, 1, 1, 32, 0, 0, 0, 0);
    SDL_FillRect(s, NULL, SDL_MapRGB(s->format, 255, 0, 0));
  }

  return *this;
}

4.) Deleting the assignment operator is probably one of the best approaches but I receive a number of compiler errors when I do that as the Sprite has a Timer which has a ProgressBar.

Code like this will require the ProgressBar to be copyable. I have to look at initializing the Sprite differently so that they don’t need to be copyable.

spriteBackground = ley::Sprite(TextureManager::Instance()->getTexture(background_level.c_str()), 0, {start_rect});

ProgressBar & operator= (const ProgressBar &pb) = delete;

@Peter87 Thanks for taking some time to help me take a look at this. You offered a lot of great advice. I’ll continue to look into trying to delete the assignment operator for ProgressBar as I think your right, these objects probably shouldn’t need to be copyable.

Electrosys