Memory leak in setImageTexture

I am experiencing a memory leak in ImageBox::setImageTexture. I already found a topic about this, dating back to 2012 - apparently the issue is still not fixed. So I thought to collect as much useful information as possible in this topic.

Versions affected:

I reproduced the leak in MyGUI 3.2.0 and in MyGUI SVN (pulled today)

Leak type:

The memory is freed properly on program exit, so leak checking tools will not detect it. However, the program's memory consumption grows steadily whenever setImageTexture is used.

To reproduce the leak:

- Add an imagebox widget in one of your layouts.

<Widget type="ImageBox" skin="ImageBox" position="13 146 65 12" align="Left Bottom" name="testimage">
</Widget>

- Each frame, call setImageTexture (it doesn't matter if using an actual texture name, or just "" - both leak).


testimage->setImageTexture("");


- To exaggerate the leak effect, so it can be measured better, you can use setImageTexture multiple times, e.g.


for (int i=0; i<100; ++i)
testimage->setImageTexture("");


For me, using setImageTexture 100 times per frame at 40 fps makes the program's memory consumption grow by ~20 MB per second.

To the developers:
- Is there anything we can do to help get this fixed? Would a reproduction sample in MyGUI Demos help?
- Can you suggest any workarounds? I tried to destroy and re-create the widget before using setImageTexture, but that doesn't help either. The memory is apparently not reclaimed. This is what I tried:

MyGUI::Widget* parent = testimage->getParent();
MyGUI::IntCoord coord = testimage->getCoord();
MyGUI::Align align = testimage->getAlign();
MyGUI::Gui::getInstance().destroyWidget(testimage);
testimage = parent->createWidget<ImageBox>("ImageBox", coord, align);

testimage->setImageTexture("");


I've taken a little closer look.

The root cause appears to be LayerNode::addToRenderItem(ITexture* _texture, bool _firstQueue, bool _manualRender). To get the leak to happen, the _firstQueue parameter has to be true, which is the case for an ImageBox (SubSkin::createDrawItem).

First, a few observations:

1. Created RenderItems are never deleted until the program quits.[/*:m]
   
2. MyGUI attempts to reuse RenderItems using the same texture. This is to batch draw calls.[/*:m]
   
3. Parameter _manualRender appears to be unused in MyGUI (always false)?[/*:m]
   
4. If parameter _firstQueue is false, reusing RenderItems appears to work fine. [/*:m]
   
5. If parameter _firstQueue is true, the code path for reusing RenderItems looks very strange: [/*:m][/list:u]
   
   
VectorRenderItem::reverse_iterator iter = mFirstRenderItems.rbegin();
if ((*iter)->getNeedVertexCount() == 0)
{
while (true)
{
VectorRenderItem::reverse_iterator next = iter + 1;
if (next != mFirstRenderItems.rend())
{
if ((*next)->getNeedVertexCount() == 0)
{
iter = next;
continue;
}
else if (!(*next)->getManualRender() && (*next)->getTexture() == _texture)
{
iter = next;
}
}

break;
}

(*iter)->setTexture(_texture);

mOutOfDate = false;

return (*iter);
}
// последний буфер с нужной текстурой
else if (!(*iter)->getManualRender() && (*iter)->getTexture() == _texture)
{
mOutOfDate = false;

return *iter;
}

   This will instantly bail out (and as result, allocate a new RenderItem) if the last render item that was added is not unused (getNeedVertexCount() != 0).
   I *think* the reason behind this logic is to keep the original draw order intact, that is, widgets that were created first should be rendered first. The order of the mFirstRenderItems vectors is actually what determines the drawing order of widgets that have the same parent (child widgets are always rendered after their parent, because they are in a child LayerNode).
   
   How can we fix this? The least intrusive fix I have in mind would be to wrap RenderItems in a SharedPtr and automatically clean them up when they're no longer referenced from outside.

1. Parameter _manualRender appears to be unused in MyGUI (always false)?

It is always false for all core subskins. It is only used in FilterNone subskin, that disable all filtering and should be rendered in separate batch.[/*:m][/list:u]

1. If parameter _firstQueue is true, the code path for reusing RenderItems looks very strange: [/*:m][/list:u]
   ...
   This will instantly bail out (and as result, allocate a new RenderItem) if the last render item that was added is not unused (getNeedVertexCount() != 0).

No, it doesn't bail out, and actually never create new RenderItem if _firstQueue is true. That loop finds last empty item or item before those if it have same texture. And then return that RenderItem.

I *think* the reason behind this logic is to keep the original draw order intact, that is, widgets that were created first should be rendered first. The order of the mFirstRenderItems vectors is actually what determines the drawing order of widgets that have the same parent (child widgets are always rendered after their parent, because they are in a child LayerNode). Correct.

How can we fix this? The least intrusive fix I have in mind would be to wrap RenderItems in a SharedPtr and automatically clean them up when they're no longer referenced from outside.I'm still not sure what actually cause leak, so I'll try to work on this. As far as I remember simply changing ImageBox texture to empty and to sime texture every frame reproduces the issue.

No, it doesn't bail out, and actually never create new RenderItem if _firstQueue is true. That loop finds last empty item or item before those if it have same texture. And then return that RenderItem.


That's not what I'm seeing. Check this:
VectorRenderItem::reverse_iterator iter = mFirstRenderItems.rbegin();
if ((*iter)->getNeedVertexCount() == 0)
{
...
(*iter)->setTexture(_texture);

mOutOfDate = false;

return (*iter);
}
// последний буфер с нужной текстурой
else if (!(*iter)->getManualRender() && (*iter)->getTexture() == _texture)
{
mOutOfDate = false;

return *iter;
}

// создаем новый буфер
RenderItem* item = new RenderItem();
item->setTexture(_texture);
item->setManualRender(_manualRender);
mFirstRenderItems.push_back(item);

mOutOfDate = false;

return item;


A new RenderItem is always allocated in the following case:
- Last RenderItem in mFirstRenderItems has non-null vertex count and:
- Last RenderItem in mFirstRenderItems does not have the needed texture

And here is reproduction case, which confirms my synopsis.

This is all you need to add to Demo_Colour.


Index: Demos/Demo_Colour/DemoKeeper.cpp
===================================================================
--- Demos/Demo_Colour/DemoKeeper.cpp (revision 5294)
+++ Demos/Demo_Colour/DemoKeeper.cpp (working copy)
@@ -25,6 +25,9 @@

void DemoKeeper::createScene()
{
+
+ MyGUI::Gui::getInstance().eventFrameStart += MyGUI::newDelegate(this, &DemoKeeper::notifyFrameStart);
+
base::BaseDemoManager::createScene();
MyGUI::LayoutManager::getInstance().loadLayout("Wallpaper.layout");
const MyGUI::VectorWidgetPtr& root = MyGUI::LayoutManager::getInstance().loadLayout("HelpPanel.layout");
@@ -37,15 +40,30 @@
mEditPanel = new EditPanel();

mColourPanel->eventColourAccept = MyGUI::newDelegate(this, &DemoKeeper::notifyColourAccept);
+
+ mParent = MyGUI::Gui::getInstance().createWidget<MyGUI::Widget>("Widget", 0, 0, 42, 42, MyGUI::Align::Default, "Overlapped");
+ mImage1 = mParent->createWidget<MyGUI::ImageBox>("ImageBox", 0, 0, 42, 42, MyGUI::Align::Default);
+ mImage2 = mParent->createWidget<MyGUI::ImageBox>("ImageBox", 0, 0, 42, 42, MyGUI::Align::Default);
}

void DemoKeeper::destroyScene()
{
+ MyGUI::Gui::getInstance().eventFrameStart -= MyGUI::newDelegate(this, &DemoKeeper::notifyFrameStart);
+
delete mEditPanel;
mEditPanel = nullptr;
delete mColourPanel;
mColourPanel = nullptr;
+
+
+ MyGUI::Gui::getInstance().destroyWidget(mParent);
}
+
+ void DemoKeeper::notifyFrameStart(float _time)
+ {
+ mImage1->setImageTexture("Pick.png");
+ mImage2->setImageTexture("Colours.png");
+ }

void DemoKeeper::notifyColourAccept(ColourPanel* _sender)
{
Index: Demos/Demo_Colour/DemoKeeper.h
===================================================================
--- Demos/Demo_Colour/DemoKeeper.h (revision 5294)
+++ Demos/Demo_Colour/DemoKeeper.h (working copy)
@@ -25,6 +25,12 @@
private:
void notifyColourAccept(ColourPanel* _sender);
virtual void setupResources();
+
+ void notifyFrameStart(float _time);
+
+ MyGUI::Widget* mParent;
+ MyGUI::ImageBox* mImage1;
+ MyGUI::ImageBox* mImage2;

private:
ColourPanel* mColourPanel;


Your case was fixed by ignoring texture reset when the same texture is used:
void LayerItem::setRenderItemTexture(ITexture* _texture)
{
if (mTexture == _texture)
return;There also was more complex case, when more that one RenderItem per frame is released. For some reason my.name assumed that system would never get more than one empty buffer per frame and this was causing not more than one RenderItem per frame being reused (while others were "leaking").

That should fix the majority of cases, but I'm not completely happy about this fix yet. It relies on the render update (updateCompression is called in renderToTarget). For example, what if the user has currently paused rendering (e.g. due to minimized game window) but game logic and UI are still updating?

That should fix the majority of cases, but I'm not completely happy about this fix yet. It relies on the render update (updateCompression is called in renderToTarget). For example, what if the user has currently paused rendering (e.g. due to minimized game window) but game logic and UI are still updating?

Reported an issue (test case attached): https://github.com/MyGUI/mygui/issues/20

Committed a fix