search

gazebosim / gz-gui

Builds on top of Qt to provide widgets which are useful when developing robotics applications, such as a 3D view, plots, dashboard, etc, and can be used together in a convenient unified interface.
https://gazebosim.org
Apache License 2.0
73 stars 43 forks source link

Memory leak in ImageDisplay with non-RGB data #341

Closed peci1 closed 2 years ago

peci1 commented 2 years ago

Environment

Description

Steps to reproduce

gui_leak.sdf ``` 0.001 1.0 ogre2 true 0 0 10 0 0 0 0.8 0.8 0.8 1 0.2 0.2 0.2 1 1000 0.9 0.01 0.001 -0.5 0.1 -0.9 true 20 20 0.1 20 20 0.1 2.5 0 1.5 0 0.0 3.14 0.05 0.05 0.05 0 0 0 10 depth_camera 1.05 320 240 R_FLOAT32 0.1 10.0 true ```
  1. ign gazebo -v4 gui_leak.sdf -r -s
  2. ign gui -v4 -s ImageDisplay
  3. Watch memory consumption of GUI in e.g. htop

Analysis

This happens in GUI 4.4.0, but not in 4.3.0. My guess is that #212 is the problematic commit, but I haven't found a way to use git bisect with Ignition (it jumps between gui 3 and gui 4 versions, which is not really easy to test).

This happens for depth and thermal cameras. It does not happen for RGB cameras.

iche033 commented 2 years ago

could be fixed by https://github.com/ignitionrobotics/ign-gui/pull/287

peci1 commented 2 years ago

Probably not everything is fixed by #287 - 4.6.0 is also affected...

peci1 commented 2 years ago

6.2.0 is not affected

peci1 commented 2 years ago

5.3.0 is also good, so this is only a problem in Dome.

iche033 commented 2 years ago

hmm I see that ign-gui 4.6.0 was released with the mem leak fix. If it's still leaking, the bug could be somewhere else. https://github.com/ignitionrobotics/ign-gui/pull/340/files#diff-c275fec9c453c2a42515bc5ab47e30fa4130ff1426aef2f6e000a9a34e122cb8R19

peci1 commented 2 years ago

As said earlier, ImageDisplay started leaking with non-RGB images in 4.4.0 (the big refactoring to start using convertToRgb()).

@darksylinc any idea why is it still leaking even using 4.6.0 with #212 merged?

darksylinc commented 2 years ago
  1. Are we sure 4.6.0 is using that fix?
  2. I remember ign/src/ign-common/graphics/src/Image.cc having a ton of leaks which I fixed in ign-common#240 (it was also causing slight mem. corruption) when manipulating non-RGB images. It might be related to that? I don't know if those classes are being used by ign-gui (or maybe the leak is in the server? I also don't know if the server is using that class)
darksylinc commented 2 years ago

Yep, ign-gui is definitely using common::Image and look at that! It's in ImageDisplay!

That's most likely the cause.

peci1 commented 2 years ago

212 is definitely merged in gui 4.6.0. However, the leak-fixing ign-common PR you mention was against ign-common4, while Dome still uses ign-common3. I don't see anything in the changelog that would suggest the PR was backported. Good news is ign-common3 is still alive and not EOLed, so if the bug is really there, fixing it could even fix Dome =) I'll give it a try to backport the fix.

peci1 commented 2 years ago

@darksylinc Thanks a lot for your help. I backported your ign-common fix ( ignitionrobotics/ign-common#287 ) and now memory consumption is stable even in Dome!

chapulina commented 2 years ago

Just catching up with this, can this issue be closed?

peci1 commented 2 years ago

Yes!