GUIInfoManager: revert 8809df940d265f79e3d529b6591a2c8333ed06d6, this…

[FernetMenta]

reverts #8766

fixes segfault

@DaveTBlake @popcornmix in order to fix the original issue, the DB operations should be offloaded

[DaveTBlake]

@FernetMenta ensuring that CGUIInfoManager::SetCurrentItem() happens on the main thread will indeed avoid the crash that occurs when g_infoManager.ResetCurrentItem() is called by the main thread while SetCurrentItem() is happening asynchronously, but any slowness in SetCurrentItem() will once again delay the rendering.

Reading from the db is part of the item update, so I don't see how it can be off loaded. Sure we can read from the db asynchronously, but the item being populated needs to exist, not be reset (causing the tag the data is being written into to vanish).

[FernetMenta]

@DaveTBlake we can run the db query on a job. when finished send item via TMSG_UPDATE_CURRENT_ITEM. then it gets updated by main thread

[DaveTBlake]

OK I see your idea @FernetMenta. We will need to take care not to end in a circle: TMSG_UPDATE_CURRENT_ITEM <-> SetCurrentItem.

Meanwhile , I don't think the GUI_MSG_UPDATE_ITEM processing in the app should be causing GUI SetCurrentItem (and all the db reading that involves). The GUI response to GUI_MSG_UPDATE_ITEM, before I moved it, was to just do an UpdateInfo that invalidated things so they repainted. I think that should be looked at too. It would avoid the current crash, but obviously not the underlying design flaw. It could also help with speed e.g. not doing db read + messaging unnecessarily, even if that read is a separate job.

[FernetMenta]

We will need to take care not to end in a circle

create an additional msg: TMSG_UPDATE_CURRENT_ITEM_DB

[koying]

Sorry, I lost track of the convo.
What is the con of just taking a deep copy of the fileitem passed to CGUIInfoManager::SetCurrentItem and offloading?
That also both solves the crash and the delay, afaict

[FernetMenta]

the issue is not realted to deep copy or not. CGUIInfoManager::m_fileItem must not be accessed by other threads than main thread.
means if we offload the db operation, the result has to be sent back via app messenger

[bacon-cheeseburger]

Is this still a fix-in-progress? This bug has caused the WAF to plummet and I'm wondering if applying cc809c0 locally will fix it? It's not clear if cc809c0 is complete or incomplete.

[FernetMenta]

Ok, I merge it. I think nobody object that fixing a crash has higher prio than some video stuttering. Further, a revert is no personal attack. I supported the reverted change myself but now I have to admit that it was wrong. We have to fix this problem differently.

[DaveTBlake]

Thanks @FernetMenta
It seems that the potential for a seg fault was spotted back in Jan, and a different solution proposed #13373 that was not completed.