Keep GUIControls alive after window deinit #1283
Conversation
|
Ok, main thing is I think we need to clearly identify where we need overriding in the subclasses. Seems to me we have:
... window cycle ...
Not sure about 2/3 vs 5/6 |
|
I don't understand what You mean with "python label parsing" (or I'm propably totally unaware that we do something like that?). I don't think we should change where/when we call existing callbacks (this would propably cause serious regressions which I already faced once and it took me quite a long time to finalize it). But adding new callbacks would be ok I think (and wouldn't hurt). |
|
IIRC winxml windows parse for existing Localise stuff and bumps the string IDs up as needed? |
|
Right, found it - CGUIPythonWindowXML is overriding CGUIWindow::LoadXML. It manipulates directly on xml string (before parsing) to replace SCRIPT### to proper ID - do we want callback that will pass xml as string so it can be manipulated? Doesn't seem nice to me. |
|
Added bunch of changes (additional commits will be squashed later). Wanted to keep comment history so we can track what is already taken care of. |
|
Looks like we can do cases 2 and 3 above via OnInitWindow, as that runs every time, right? So we could do: DerivedClass::OnInitWindow() BaseClass::OnInitWindow() // sets up animations etc. // do whatever needs doing in 3 So I don't think we need a new callback there. |
|
Changes look good |
ghost
assigned 
…values into map<conditionID:int, value:bool> store conditions and values used to resolve includes in CGUIWindow added CGUIInfoManager::ConditionsChangedValues method
added additional checks in AllocResources - we have to load .xml file if: - load type is set to LOAD_EVERY_TIME (all windows use it by default) - window is loaded and any of include condition have changed value - window isn't loaded we have to unload window before loading if window wasn't unloaded before and include conditions have change value(s)
this allows to avoid reading and parsing xml file in windows we already used before and are loaded on demand or their include conditions have changed values
…to force restoring static items before restoring control states ( in CGUIWindow::OnInitWindow, SetInitialVisibility() is called before RestoreControlStates() ) not need to call UpdateStaticItems() if we are loading static items from .xml as we do it before SetInitialVisibility is called and it will be called there
…ion of dialog - we need additional members to remember window coordinates (these are fetched while loading window .xml file and later are changed when setting position of ContextMenu) and size of background image - we don't need to call SetInitialVisibility - it's alreadu done in CGUIWindow::OnWindowInit - we need to manually remove choice buttons in OnDeinitWindow - we need to store size of background image to properly set its size it in consequent showings
…n initing window and if dialog is already active
…w, clear it reset it when we unload window
…window, fill/clear protocol spinner when we init/deinit window
…e to not consuming GUI_MSG_WINDOW_INIT message also move init actions to OnInitWindow callback
…e deinit order a little and let generic window code to handle control unloading/freeing
…iguration to OnInitWindow
…it, clear rule and operand spinner on deinit and try to change value button to edit control just once on window load
Keep GUIControls alive after window deinit
|
fixed in 8b796ee |
|
just tried it. it's not entirely fixed: the media flags still don't update correctly. Eg. the first one I view is bluray, the next one not, but the flag is still there. EDIT: same with the music albums. Try an album with cover art, then another without. The first album art is still displayed... |
|
@Voyager-xbmc |
|
@pieh - thanks for the great work. I was close to the same solution (only the 2nd if though) but I think this is it. I'll play with it - happy to report experiences after the weekend :-) |
|
This seems to suggest some other problem is present to me. If the texture With that said, at least the second bit shouldn't be a problem as Another thing I'm not sure about is why the same image path would be used On Thursday, September 13, 2012, Voyager-xbmc wrote:
|
|
@jmarshallnz - the issue is that pieh's PR altered the way windows get loaded. Previously they were loaded every time, now they stay in memory and get reused, thereby speeding up the GUI. This means that controls don't get reinitialised via constructor. In between inits an image on a window should be able to change, e.g. the album art, the media flag images... The problem was that if we went from an "old" image with an actual texture in it (e.g the 720.png flag) to a "new" image path that was empty (e.g. no flag set), somehow it didn't change the texture. By debugging I found that the m_currentTexture would indeed be empty, but the m_texture.m_info filename was still the old one (e.g. 720.png). It turned out m_texture.SetFileName wasn't called, nor did we mark the dirtyregion. Edit: |
|
@jmarshallnz I will check it after weekend and do proper PR for it.
|
|
Perhaps we should clear out the texture info in free texture then? This On Friday, September 14, 2012, Michal Piechowiak wrote:
|
|
tried this, and it works. Just don't empty the m_currentTexture anymore, so the comparison with strFileName in ::SetFileName works properly. I checked and m_currentTexture isn't much used anywhere else. I also tried messing with clearing out the texture info (m_info.filename) in the free texture method, but that creates a lot of issues with the gui... void CGUIImage::FreeTextures(bool immediately /* = false */)
{
m_texture.FreeResources(immediately);
for (unsigned int i = 0; i < m_fadingTextures.size(); i++)
delete m_fadingTextures[i];
m_fadingTextures.clear();
//m_currentTexture.Empty(); <-- remove this!
} |
|
I'd think that calling m_texture.SetFileName would do the trick, and seems On Friday, September 14, 2012, Voyager-xbmc wrote:
|
|
the problem is more than just keeping them in sync. |
|
and when you move the m_texture.SetFileName(m_currentTexture); as the first line to CGUIImage::AllocResources, results are even worse. Lots of textures missing. http://imageshack.us/a/img152/6313/screenshot000u.png Somehow, I think that my proposal to NOT clear m_currentTexture is the least intrusive. This member variable is just used to carry the latest texture filename used, nothing else. Not clearing it means you can appropriately track a change from a non-empty to an empty picture... |
|
@Voyager-xbmc this won't make textures dissapear - problem with disappearing textures is that if texture path is constant (as in no InfoLabels are used, so all confluence textures) we will only update texture once - when we create image control. This won't break it because constant texture path doesn't use m_currentFile (leaving it empty) so when I try to set file name in ::FreeTextures it won't have any effect (as new and old path are both empty) it still isn't as nice as I would like it (mostly part that it won't break constant textures "only by accident"), but at least better than previous "fix" |
|
Isn't that identical to calling m_texture.SetFilename("") after resetting m_current texture ? |
|
@jmarshallnz : I would say it's almost identical, with the exception of two things: checking the value already in m_currentTexture (don't do anything if already empty) and the dirty region handling. With pieh's solution working fine, I have adapted it as follows. Note that without the if (!m_currentTexture.Equals("")) you get again a lot of empty/missing images as in my screenshots above. void CGUIImage::FreeTextures(bool immediately /* = false */)
{
m_texture.FreeResources(immediately);
for (unsigned int i = 0; i < m_fadingTextures.size(); i++)
delete m_fadingTextures[i];
m_fadingTextures.clear();
if (!m_currentTexture.Equals(""))
{
m_currentTexture = "";
if (m_texture.SetFileName(m_currentTexture))
MarkDirtyRegion();
}
} |
|
You needn't mark the region dirty.
|
|
You're right. Reduced it to the following, and still works ok: void CGUIImage::FreeTextures(bool immediately /* = false */)
{
m_texture.FreeResources(immediately);
for (unsigned int i = 0; i < m_fadingTextures.size(); i++)
delete m_fadingTextures[i];
m_fadingTextures.clear();
if (!m_currentTexture.IsEmpty())
{
m_currentTexture.Empty();
m_texture.SetFileName(m_currentTexture);
}
}So it seems that the if guard is what is making the difference. Probably FreeTextures is called when m_currentTexture is already empty but m_texture is not, in which case without the if guard we would empty the m_texture's filename, thus causing the missing images from my screenshots above. |
|
Additional info: So it seems to work fine with the code I posted above. However, there are now still cases where m_currentTexture is empty while m_texture's filename is not. Apparently m_texture's filename is set in a number of places in CGUIImage, without syncing m_currentTexture, e.g. the contstructors, CGUIImage::Process and CGUIImage::SetInfo. I tried to make sure we sync it in all those places, but then the problem I described with the screenshots reappears. Similarly, I removed m_currentTexture everywhere and replaced it with m_texture.GetFileName() - same issue. I don't know the code enough, but it seems that those two are not really meant to be in sync at all times. |
There are 2 cases: we use constant texture path: f.e. "floor.png" or we use dynamic texture path: f.e. "$INFO[ListItem.Icon]". First one will just set texture's filename in CGUI::SetInfo without using m_currentTexture at all (so it will stay empty) - in this case we can't simply reset texture's filename as we won't update texture next time we load window (that's why there are missing textures as texture has empty path). Second one will be evaluated each frame and compared to m_currentTexture and update both m_currentTexture and texture object if it changed - so as long as we keep m_texture and m_currentTexture in sync it will be good. So IMO good solution would be to both clear m_currentTexture and m_texture and set texture path in method that is called when window is inited. There's obviously catch as this method would be ::AllocResources but CGUIImage has DynamicResourceAlloc flag set which means AllocResoures won't be called on window init and image control itself is in charge of allocating resources. I will try to get around this and see what I can come up with. |
|
here's another take on it: pieh@a5b3b36 |
|
tried it, and looks good. I don't know if there is a real reason to keep m_currentTexture empty for the constant textures. In any case I tried your solution, it works. I have also tried modifying it to also sync m_currentTexture in ::AllocResources and that works too. if (m_info.IsConstant())
{
m_texture.SetFileName(m_info.GetLabel(0));
m_currentTexture = m_texture.GetFileName();
} |
|
if (!m_info.IsConstant()) // constant textures never change In the same place we reset m_currentTexture.
|
|
@jmarshallnz - just to make sure, you mean it like this? void CGUIImage::FreeTextures(bool immediately /* = false */)
{
m_texture.FreeResources(immediately);
for (unsigned int i = 0; i < m_fadingTextures.size(); i++)
delete m_fadingTextures[i];
m_fadingTextures.clear();
if (!m_info.IsConstant()) // constant textures never change
{
m_currentTexture.Empty();
m_texture.SetFileName(m_currentTexture);
}
} |
|
Put the m_currentTexture.Empty() outside the if I think? On Wed, Sep 19, 2012 at 5:50 PM, Voyager-xbmc notifications@github.comwrote:
|
|
@jmarshallnz - got it. This works too. Thanks!! void CGUIImage::FreeTextures(bool immediately /* = false */)
{
m_texture.FreeResources(immediately);
for (unsigned int i = 0; i < m_fadingTextures.size(); i++)
delete m_fadingTextures[i];
m_fadingTextures.clear();
m_currentTexture.Empty();
if (!m_info.IsConstant()) // constant textures never change
m_texture.SetFileName("");
} |
|
@jmarshallnz push them or they need more work? |
|
The first 3 look fine. Not sure the 4th is needed - see my comment there. On Thu, Sep 20, 2012 at 8:11 AM, Michal Piechowiak <notifications@github.com
|
|
This causes issue #13351 (http://trac.xbmc.org/ticket/13351). I would fix that with an PR but I don't know what is the desired way ;-) |
|
Yeah I saw this late last night and couldn't be arsed to hunt it at the On Wed, Sep 26, 2012 at 3:30 PM, Tristan Fischer
|
When you have a scrolling list, Application will try first to send the message to the window rather than processing it. in the case of 'h' key, and on preplay window, the BUILTIN message will come down to preplay window and be handled by a controlgroup that will put it in an onclick message forwarded to the controls. this is pretty much insane. So we just say that we dont wanna handle builtins, then Application will handle them properly.
This is resurrection of #127 in much more complete state:
Window's controls are being deleted on window deinit. This results in loading xml, resolving includes, parsing resolved xml and creating controls everytime user activate window.
This will allow to keep controls objects in memory on window deinit. It also will store xml windows root node that could be used if load type is set to load_on_demand (old style - few windows are still like this) or if include condition have changed value.
Some numbers (tested on raspberry pi with slow sd card where it will have biggest impact I guess): compare alloc resources time for first and second window inits. (I didn't really gather data, just ran xbmc 3 times and given that results were similliar I called it done)
On android (skin.retouched) Home.xml was loaded 1st time: 126ms and 2nd time: 5ms (that's total alloc resources time). On my desktop with ssd - home.xml = 1st time: 42ms, 2nd time: 4ms. So it's not that spectacular everywhere. Note: alloc resources times don't include time spent in OnWindowInit (f.e. video nav window fetch directory there)!
Here's list of windows that weren't yet adjusted/tested if they will work "as-is":
And finally - my branch currently contains 2(3) last temporary commits (they will be removed later) to
(3). add window loadtype to log on window load (depends on how verbose we want to be)