My memory is not free'd

Question

I have a rather large program that generally runs wonderfully, but uses insane amounts of memory to run. This is a machine learning approach that collects a lot of data, and thus that is generally okay, but the memory grows and grows very fast even after all the data is collected, and so I used valgrind massif to find out what's going wrong. The top of massif's heap tree looks like this:

99.52% (60,066,179B) (heap allocation functions) malloc/new/new[], --alloc-fns, etc.
->43.50% (26,256,000B) 0x439785: Image::Image(SDL_Surface*) (Image.cpp:95)
| ->43.50% (26,256,000B) 0x437277: EncodedFeature::forwardPass() (EncodedFeature.cpp:65)
....

So I thought, hmm, maybe the constructed Image is not free'd, but no:

void EncodedFeature::forwardPass()
{
    // Get image:
    Image* img = new Image(screen);

    // Preprocess:
    if(preprocessor)
        preprocessor->process(img);

    // Do forward pass:
    encoder->encode(img, features);

    delete img;
}

So to the Image constructor:

Image::Image(SDL_Surface* surface)
{
    this->width = surface->w;
    this->height = surface->h;
    pixels = new int*[width];
    for(int i = 0; i < width; i++)
        pixels[i] = new int[height];

    for(int x = 0; x < surface->w; x++)
        for(int y = 0; y < surface->h; y++)
            pixels[x][y] = getPixelFromSDLSurface(surface, x, y);
}

Just allocates a pixel array that gets freed in the destructor later on:

Image::~Image()
{
    if(!pixels)
        return;

    for(int x = 0 ; x < width; x++)
        delete[] pixels[x];

    delete[] pixels;
}

So the last culprit:

Uint32 Image::getPixelFromSDLSurface(SDL_Surface *surface, int x, int y)
{
    if(!surface)
        return 0;

    // Got this method from http://www.libsdl.org/cgi/docwiki.fcg/Pixel_Access
    int bpp = surface->format->BytesPerPixel;
    /* Here p is the address to the pixel we want to retrieve */
    Uint8 *p = (Uint8 *)surface->pixels + y * surface->pitch + x * bpp;

    switch(bpp) {
    case 1:
        return *p;
        break;

    case 2:
        return *(Uint16 *)p;
        break;

    case 3:
        if(SDL_BYTEORDER == SDL_BIG_ENDIAN)
            return p[0] << 16 | p[1] << 8 | p[2];
        else
            return p[0] | p[1] << 8 | p[2] << 16;
        break;

    case 4:
        return *(Uint32 *)p;
        break;

    default:
        return 0;       /* shouldn't happen, but avoids warnings */
    }
}

As mentioned in the comment, I got that one from the SDL wiki, so I hope there is nothing leaking there. bpp in my case is actually always 1, so it just returns the int at address p, which doesn't sound leaky to me.

I'm at the end of my wits here. Can anyone think of where the memory went? I mean, massif points specifically to the Image constructor, but I can't see anything wrong there...

Thanks a lot for looking at my problem!

Max

---

Answering your comments:

You are right, I did not need img as a pointer. I come from a Java background so I just want everything to be pointers :) Changed it but didn't help.

Line 95 is inside the first for loop of the constructor: pixels[i] = new int[height];

In one of the preprocessors I do resize the image, but I do this calling my reset function, which should make sure that the old array is deleted:

void Image::reset(int width, int height)
{
    if(pixels)
    {
        // Delete old image:
        for(int x = 0 ; x < width; x++)
            delete[] pixels[x];

        delete[] pixels;
    }

    this->width = width;
    this->height = height;
    pixels = new int*[width];
    for(int i = 0; i < width; i++)
        pixels[i] = new int[height];
}

After which I refill the pixel values...

No exceptions are being thrown anywhere.

Where would you suggest I use smart pointers?

Thanks for all the answers!

Answer 1

I don't think you are representing pixels correctly in your image class. I think you can use a simple 1-dimensional vector of the correct unsigned type (uint32_t?).

(in header):

class Image {
protected:
    std::vector<uint32_t> pixels;
    ...
};

(in implementation file)

size_t Image::offset(unsigned x, unsigned y) {
    return (y * width) + x;
}

Image::Image(const SDL_Surface* surface)
{
    width = surface->w;
    height = surface->h;
    pixels.reserve(width * height);
    for(unsigned x = 0; x < width; x++)
        for(unsigned y = 0; y < height; y++)
            pixels[offset(x, y)] = getPixelFromSDLSurface(surface, x, y);
}

Your destructor would then be completely empty, as there is nothing for it to do:

Image::~Image() {
}

Note you need to use the offset() method to get the correct index into the vector for any given x/y pair.

Answer 2

The most likely scenario is that somehow your reset or destructor isn't called in some code path, leaking that memory.

Luckily C++ has a built-in capability to prevent such leaks: It's called vector.

So first you make your pixels look like this:

typedef std::vector<int> PixelCol;
typedef std::vector<PixelCol> Pixels;
Pixels pixels;

Now we size it in the constructor:

Image::Image(SDL_Surface* surface)
: pixels(surface->w, PixelCol(surface->h))
{
    this->width = surface->w;
    this->height = surface->h;

    for(int x = 0; x < surface->w; x++)
        for(int y = 0; y < surface->h; y++)
            pixels[x][y] = getPixelFromSDLSurface(surface, x, y);
}

Finally we update reset:

void Image::reset(int width, int height)
{
    Pixels(width, PixelCol(height)).swap(pixels);
}

By letting the language manage your memory, you completely eliminate any such memory management bug sin your code.