Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fixed memory leak on minimized due to missing deletion of unused textures (fixes #14245) #2540

Merged
merged 1 commit into from

8 participants

@wsoltys
Collaborator

I made this a pr since it looks like m_AppActive is also used for SDL and android.

Using multiimage controls lead to heavy memory leaking when being minimized because we don't free the unused textures anymore. I just removed the hole block because on windows doing a FreeUnusedTextures() inside that block costs more cpu power than process the whole Render path (7% to 5%).
Given that we just want to leave early to save resources this makes no sense at least on win. What I need to know is if this has counter effects on the other platforms using m_AppActive.

@theuni
Owner

Pretty sure we can drop the m_AppActive diddling for android, we handle it with a bigger hammer now (m_renderGUI). Looks like each platform has implemented this idea on its own at some point, it's probably really in need of some unification.

Fine for android.

@wsoltys
Collaborator

I've added m_AppActive to the lowfps bool which lowers resources needed when running minimized.

@MartijnKaijser

@wsoltys
this indeed fixes the huge mem leak however there's still another small leakage present in the orders of 10mb/minute.
Same testcase as in ticket (or perhaps also set The Big pictures screensaver and minimize)

Edit:
could be unrelated and something else leaking however when minimized it got from 100mb to 175mb.

@theuni
Owner

@wsoltys If you're going to go to the trouble of trying to minimize usage, I highly recommend taking a look at m_renderGUI (and SetRenderGUI). With this set to false, XBMC is essentially running headless. We use it in Android and on tiny arm cpus, we get down to ~0% cpu usage. It takes care of sleeping.

If we could unify this behavior in CApp, then all platforms could just call CApp::SetRenderGUI(false) when they don't need to render (minimized, obscured, etc).

@wsoltys
Collaborator

@theuni: thanks, will look into it to unify it across platforms. but this shouldn't hinder this bugfix to go in first.

@MartijnKaijser: one step after another. if its a one time peak and go down when maximized than its no real problem. For the other issue since python is involved you should extend the ticket with the test case and add @jimfcarroll as well.

@MartijnKaijser

@wsoltys
ofcourse. thx for tracking this one down and i'll search for other test case and if needed make new ticket

@davilla
if you can confirm this can be merged?
this should also be backported to 12.2

@ulion
Collaborator

who eat the memory? why not stop it from source?

@davilla
Collaborator

yes, memory leak can cause crashes and crash fixes are certainly 12.2 material. please queue it in the 12.2 backport thread.

@t-nelson

I agree with ulion here. This only seems to hide the real leak. The commit is probably something we want. But the message doesn't seem accurate.

@wsoltys
Collaborator

Dudes its really difficult to keep the fun for what we're doing here. I'm no professional dev doing this in rl for years so just give me the right text for that commit and I'll change it.
This here is quite simple you allocate textures and if you won't deallocate your eating up memory. I can't tell why we still store them in m_unusedTextures but that wasn't the point of this pr. Leaving the render path too early just won't free the textures.
Probably one can have a look why the are allocated when minimized but thats more than a bugfix. Feel free to do so.

@wsoltys
Collaborator

@theuni : Using SetRenderGUI() seems to work fine when we still process CWinEvents::MessagePump(); which you currently won't do.

@jmarshallnz
Owner

Regarding the memleak, I think moving the call for FreeUnusedTextures() from Render() to ProcessSlow() (where the large texture manager is done) will do the trick? This should definitely be done - we don't want to rely on Render() being done to cleanup unused textures.

@wsoltys
Collaborator

closing in favor of #2546

@wsoltys wsoltys closed this
@wsoltys wsoltys reopened this
@wsoltys wsoltys merged commit 8074b0b into from
@FernetMenta
Collaborator

I am not sure if this is correct. I was chasing ASIC hangs on AMD hw which was related to deleting textures before they actually were rendered. The problem appeared again some time ago. Not sure but I guess it came back with enabling dirty regions. Before deleting a texture you have to make sure it is not in use anymore. You know this after buffers have been swapped:
FernetMenta@55935b9

@jmarshallnz
Owner

Hmm, I'm not sure how you can have a texture in the m_unusedTextures list that can still be in use by the GPU - if a texture is no longer being used by all controls, then there would be a dirty region set as the last control gives up the texture, so that a flip will occur anyway, and the texture would then be unused at next ProcessSlow() ?

(Note that moving the freeing of unused textures to ProcessSlow will inadvertently hide any bug that exists here as it's only called every 500ms rather than every app loop)

@FernetMenta
Collaborator

I am just a bit scared that this could make the problem even harder to track down. Once every 500ms we have the same sequence as before, right?

I don't see any obvious problems either, just investigating in this area. There may be an issue with dr but I am far away from being sure. A user reports losing vsync on Linux (Nvidia) once every 30 hours. The last tests with dr disabled have shown a negative (no issue). Not confirmed, needs much more testing.

@jmarshallnz
Owner

For sure it would make it harder to track. :) It won't make you feel any better, but I think there'd already be a small chance of a hard to track problem anyway :). The large texture manager is handled the same way, though that has at least a 2 second delay between when the texture is last used by a control and the texture being deleted, which makes it somewhat unlikely that it'll still be used by the GPU.

Still, if you think there's potentially a real issue, then one option is to move it back to Render() and then get rid of the code block that was causing Render() to be skipped when minimised in the Frodo branch. This would also mean that we don't reduce texture removal to only every 500ms. The master branch doesn't need this fix at all, so it could be reverted there.

@FernetMenta FernetMenta referenced this pull request in OpenELEC/OpenELEC.tv
Closed

ASIC hang back again #2296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2013
  1. fixed memory leak on minimized due to missing deletion of unused text…

    wsoltys authored
    …ures (fixes parts of #14245)
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 2 deletions.
  1. +2 −2 xbmc/Application.cpp
View
4 xbmc/Application.cpp
@@ -2413,8 +2413,6 @@ void CApplication::Render()
g_graphicsContext.Flip(dirtyRegions);
CTimeUtils::UpdateFrameTime(flip);
- g_TextureManager.FreeUnusedTextures();
-
g_renderManager.UpdateResolution();
g_renderManager.ManageCaptures();
@@ -5182,6 +5180,8 @@ void CApplication::ProcessSlow()
if (!IsPlayingVideo())
g_largeTextureManager.CleanupUnusedImages();
+ g_TextureManager.FreeUnusedTextures();
+
#ifdef HAS_DVD_DRIVE
// checks whats in the DVD drive and tries to autostart the content (xbox games, dvd, cdda, avi files...)
if (!IsPlayingVideo())
Something went wrong with that request. Please try again.