MRPT / mrpt

:zap: The Mobile Robot Programming Toolkit (MRPT)
https://docs.mrpt.org/reference/latest/
BSD 3-Clause "New" or "Revised" License
1.93k stars 630 forks source link

Memory leak or not the right way of using CImage with opengl::CTexturedPlane #1136

Closed nrasulnrasul closed 3 years ago

nrasulnrasul commented 3 years ago

Hi,

I am using gui::CDisplayWindow3D and opengl::CTexturedPlanefor visualizing maps(colour maps not just gray).
I observed that there was memory leak in opengl::CTexturedPlane::assignImage.
If the below program run for 1000 loops (x =1000), it will fill the 16gb ram and the system hangs.

I have attached the program with cmake, so that you can build and verify the issue easily. Just run the program and observe the memory using htop.

ZIP file

Is it the correct way of using CImage with CTexturedPlane.

Thanks.

#include <iostream>
#include <mrpt/img/CImage.h>
#include <mrpt/opengl/CTexturedPlane.h>
#include <mrpt/gui/CDisplayWindow3D.h>

using namespace mrpt;
using namespace mrpt::img;
using namespace mrpt::gui;
using namespace mrpt::opengl;
int main()
{
long x = 0;

CDisplayWindow3D *window3D = new CDisplayWindow3D();
window3D->setWindowTitle("Visualization");
window3D->resize(800, 600);

opengl::CTexturedPlane::Ptr objs = opengl::CTexturedPlane::Create();
objs->setName("map");
objs->setPlaneCorners(0, 0, 0, 0);

opengl::COpenGLScene::Ptr &ptrScene = window3D->get3DSceneAndLock();
ptrScene->insert(objs);
window3D->unlockAccess3DScene();
window3D->forceRepaint();

while (x< 500)
{

    //mrpt::img::CImage::Ptr imgColor = std::make_shared<mrpt::img::CImage>(1600, 1600, mrpt::img::TImageChannels::CH_RGB);
    //mrpt::img::CImage::Ptr imgTrans = std::make_shared<mrpt::img::CImage>(1600, 1600, mrpt::img::TImageChannels::CH_GRAY);

    mrpt::img::CImage imgColor(1600, 1600, mrpt::img::TImageChannels::CH_RGB);  
    mrpt::img::CImage imgTrans(1600, 1600, mrpt::img::TImageChannels::CH_GRAY);

    for (uint32_t j = 0; j < 1600; j++) {
            for (uint32_t i = 0; i < 1600; i++) {

            //imgTrans->setPixel(i, j, img::TColor::red());
            //imgColor->setPixel(i, j, img::TColor::green());
            imgColor.setPixel(i, j, img::TColor::red());
            imgTrans.setPixel(i, j, img::TColor::red());
        }
    }

    {
        opengl::COpenGLScene::Ptr &ptrScene = window3D->get3DSceneAndLock();
        opengl::CTexturedPlane::Ptr ptrPlane = std::dynamic_pointer_cast<opengl::CTexturedPlane>(ptrScene->getByName("map"));
        ptrPlane->setPlaneCorners(0, 0, 0, 0);
        ptrPlane->setPlaneCorners(0, 0, 1599, 1599);

        //ptrPlane->assignImage(*imgColor, *imgTrans);
        ptrPlane->assignImage(imgColor, imgTrans); // causing memory leak
        ptrPlane->notifyChange();
        window3D->unlockAccess3DScene();
        window3D->forceRepaint();
    }

    std::cout << "x = " << ++x <<std::endl;
}

if(window3D) delete window3D;
return 0;
}
jlblancoc commented 3 years ago

Thanks! Confirmed, I can see the memory leak. I'll look at it...

jlblancoc commented 3 years ago

Fixed. See: a4b8c37b81de7340d5f334f0d9eb67be5244a8c1 and the updated source code: main.zip

Cheers

nrasulnrasul commented 3 years ago

Thanks for the fix and updated code. I couldnt verify it now becuase i am using mrpt from ppa:joseluisblancoc/mrpt repository in Ubuntu 20.04.

jlblancoc commented 3 years ago

The most relevant wrong part in your code was the wrong order of xMin, xMax setting the plane dimensions, you can change that locally.

nrasulnrasul commented 3 years ago

I have tested your code, but I could still see the memory leak with looping 1000 times. i.e while (x < 1000) { // Line 48

jlblancoc commented 3 years ago

Ok, I see... you were right, thanks a lot for testing and reporting! This serious leak is now fixed in the branch develop.