Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

FIX: Crash when unloading fonts #2371

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Owner

koying commented Mar 4, 2013

No description provided.

@ghost ghost assigned theuni Mar 4, 2013

Fix is correct, though a crash is pointing to something using UI resources from two different threads.

Yea, i assume this is happening when we hide the XBMC window? A backtrace would probably tell the whole story here if you can reproduce.

Owner

koying replied Mar 5, 2013

Sorry for the lack of details, I thought it would have been facepalm obvious to theUni ;-)

The (random) crash is in CGUIFontTTFBase::RemoveReference, called from CGUIFont::SetFont(NULL), indeed when hiding XBMC (e.g. going to the launcher).
Unless mistaken, the CGUIFontTTFBase's whose reference are removed are the same that are deleted just above, from m_vecFontFiles...

Member

theuni commented Mar 5, 2013

@koying: This should of course be fixed, but I think it might be possible to skip the unload/load entirely. It was first added because when we destroyed the window we destroyed the skin as well. Since we don't do that anymore, I think the fonts should persist just fine. Worth a shot, anyway.

Member

jmarshallnz commented Mar 5, 2013

Fix is fine in the meantime, but yes, there really shouldn't be any deletion of fonts here as they're supposed to be refcounted.

Member

theuni commented Mar 5, 2013

The following works fine after a few min of testing, launching random apps and returning to xbmc about 50 times

diff --git a/xbmc/Application.cpp b/xbmc/Application.cpp
index cac91db..92f30a8 100644
--- a/xbmc/Application.cpp
+++ b/xbmc/Application.cpp
@@ -921,13 +921,11 @@ bool CApplication::InitWindow()
   }
   // set GUI res and force the clear of the screen
   g_graphicsContext.SetVideoResolution(g_guiSettings.m_LookAndFeelResolution);
-  g_fontManager.ReloadTTFFonts();
   return true;
 }

 bool CApplication::DestroyWindow()
 {
-  g_fontManager.UnloadTTFFonts();
   return g_Windowing.DestroyWindow();
 }

Owner

koying commented Mar 6, 2013

@theuni Obviously your call ;)
Will your fix also be valid for 12.1?

Member

jmarshallnz commented Mar 6, 2013

Is DestroyWindow called when resizing the UI? That's when fonts need unloading.

Member

theuni commented Mar 6, 2013

No, what's being created/destroyed has no size. Once we have the window, the rendering surface is bound to it and dimensions are set.

I guess it's possible, though unlikely, that these could vary. Something like (in android): launch xbmc -> launch settings -> change resolution -> back to XBMC. In that case, incoming/outgoing res would be different and fonts would need to be reloaded I suppose?

Owner

koying commented Mar 15, 2013

Ping @theuni . So, what do we do?

Member

theuni commented Mar 16, 2013

I'd like to hear from @jmarshallnz on this one. As far as I know, we never reloaded fonts this way before 7d32a8c . It was added at that point because we destroyed the window very violently at the time, we're much more gentle now :)

@jmarshallnz We call DestroyWindow(), but in our case, that translates down to only a very specific low-level destruction (egl context and egl surface), then we instantly drop to headless mode.

Our textures don't get nuked, and the context/surface are recreated before jumping back into the render loop. So I'm not sure of anything that would require that we reload?

Member

jmarshallnz commented Mar 16, 2013

As long as the context is recreated so that the textures are still fine, it should be OK, yup.

The functionality is not used anywhere else.

Member

theuni commented Mar 19, 2013

Ok, well there's our answer then. Let's just revert to pre-7d32a8c, and drop the load/unload.

Owner

Memphiz commented Jul 3, 2013

Is that crash still an issue @theuni @koying ?

Member

jmarshallnz commented Aug 5, 2013

As far as I can tell, the fix that @theuni suggests should go in. This one could also go in, but only once the other has been taken care of, as otherwise it hides the issue.

Owner

koying commented Aug 14, 2013

I confirm the issue is still there.
@jmarshallnz or @theuni, are you going to make a PR for the root cause? I'd hate to do a PR for something I don't understand ;)

Owner

MartijnKaijser commented Oct 12, 2013

@jmarshallnz look at this

Member

jmarshallnz commented Oct 13, 2013

#3420 to replace this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment