Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

offload videodb task from mainthread to job #13652

Merged
merged 1 commit into from Mar 21, 2018

Conversation

@FernetMenta
Copy link
Member

commented Mar 15, 2018

see title

@popcornmix @DaveTBlake I did this for video here. audio needs to be done same way

@da-anda

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

probably also ping @garbear to apply it for games as well?

@@ -3909,6 +3925,11 @@ bool CApplication::OnMessage(CGUIMessage& message)
g_infoManager.SetCurrentItem(*m_itemCurrentFile);

This comment has been minimized.

Copy link
@da-anda

da-anda Mar 15, 2018

Member

I think additional changes are needed here now, because setCurrentItem will notify observers about a changed file, which after your changes will not be set, since you no longer call "setCurrentMovie" which updated m_currentFile of GUIInfomanager. So all this method now does is to reset m_currentFile of the Infomanger and notify observers, while no new data is available yet for them to consume. So I suppose that this call will become somewhat obsolete and the observers would need to be notified on CGUIInfomanger::updateInfo() instead?
I'm not too familiar with how the Infomanager works, so I could be wrong on the impact of triggering the observers while no data is yet available in Infomanager.

@ksooo

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

I'm pretty sure this breaks other functionality. Can you please check whether pvr channel preview is still working after this change ... or even a simple channel playback or channel switch? SetCurrentMovie was the (neither nice nor obvious I admit) fallback for everything not carrying a musictag or a gametag - for example pvr channels.

*m_currentFile = item;

/* also call GetMovieInfo() when a VideoInfoTag is already present or additional info won't be present in the tag */
if (!m_currentFile->HasPVRChannelInfoTag())

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Mar 15, 2018

Author Member

@ksooo I am pretty sure that breaks nothing pvr related

}

void CGUIInfoManager::SetCurrentItem(const CFileItem &item)
{
ResetCurrentItem();

CFileItem newItem(item);
*m_currentFile = item;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Mar 15, 2018

Author Member

@ksooo thanks, I pulled this up to this level

This comment has been minimized.

Copy link
@ksooo

ksooo Mar 15, 2018

Member

SetCurrentSong and SetCurrentGame some lines below also set m_currentFile, which could be removed now.

@FernetMenta FernetMenta force-pushed the FernetMenta:videodb branch from b8c4ef1 to 252fa00 Mar 16, 2018

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

@FernetMenta thank you for the example of what approach you have in mind for getting all the db access made by set current item off the main thread again. Like @ksooo I have concerns that as is this breaks other functionality.

What about the other calls to g_infoManager.SetCurrentItem() ? For example
https://github.com/FernetMenta/xbmc/blob/252fa00b15397a2a5f2390bd28b8e7aab2e5ce28/xbmc/Application.cpp#L2050
That is related to video playing.
CVideoPlayerRadioRDS, and PVRGUIChannelNavigator also call SetCurrentItem(), but I don't know the context.

Also both upnp and AirTunes server send TMSG_UPDATE_CURRENT_ITEM messages which results in CGUIInfoManager::SetCurrentItem() being called. If the item is video then the a call to CVideoLibrarySetFileItemJob is needed to replace the old SetCurrentMovie processing.

Is it intended to replace CGUIInfoManager::SetCurrentItem entirely with separate calls to the appropriate video/audio/game job?

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2018

What about the other calls to g_infoManager.SetCurrentItem() ? For example
https://github.com/FernetMenta/xbmc/blob/252fa00b15397a2a5f2390bd28b8e7aab2e5ce28/xbmc/Application.cpp#L2050
That is related to video playing.

read the comment of the line you linked:

// Mirror changes to GUI item

guiinfo is just a mirror of Capp. all code that does not consider this is wrong and has to be fixed.

pvr changes the mirror temporarily but can restore from capp later.

TMSG_UPDATE_CURRENT_ITEM won't ever need the database. This is for updating gui with info that is not stored anywhere. i.e. the son title of current shoutcast song
TMSG_UPDATE_CURRENT_ITEM is hacky, this is why I created a new message. This should be sent to Capp first and update Capp or external clients won't get the info via APIs.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

guiinfo is just a mirror of Capp. all code that does not consider this is wrong and has to be fixed.

How does that apply to CApp handling of ACTION_INCREASE_RATING and ACTION_DECREASE_RATING? With this PR such rating changes to the current video will no longer be passed to GUI or seen by the user.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2018

How does that apply to CApp handling of ACTION_INCREASE_RATING and ACTION_DECREASE_RATING

This works as expected. Capp handels those messages, sets rating on fileItem in Capp, and updates guiInfo

// Mirror changes to current GUI item

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2018

With this PR such rating changes to the current video will no longer be passed to GUI or seen by the user

not sure what code you are looking at. Capp updates gui for this. see comment above

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Mar 18, 2018

Rating change for songs is still fine, my comment was about rating change for videos

not sure what code you are looking at. Capp updates gui for this. see comment above

Yes code L2050 calls g_infoManager.SetCurrentItem(*m_itemCurrentFile); , but I am concerned what this now achieves. It will reset the item wiping any extra data it had already from the db, and copy the Capp item thus the new rating, but this method no longer does anything more for video so will not refetch the extra db data it previously had.

I was wrong about rating, it gets that from CApp item, but what of the extra db data?
Also I'm not saying what this code did was right or necessary, frankly setting rating is a mess, only that is was doing something that won't happen with this PR.

guiinfo is just a mirror of Capp. all code that does not consider this is wrong and has to be fixed.

GUI item is potentially a bit more than CApp item (or why do the db read?). Any scope on what is wrong where and what is the fix?

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2018

lets stop talking about concerns. this change is the way to go. if anything breaks, I'll fix it.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

confirmed fix for not working action "playlist" https://github.com/FernetMenta/kodi-agile/pull/208

@FernetMenta FernetMenta merged commit d571386 into xbmc:master Mar 21, 2018

1 check passed

default You're awesome. Have a cookie
Details

@FernetMenta FernetMenta deleted the FernetMenta:videodb branch Mar 21, 2018

@Rechi Rechi added this to the Leia 18.0-alpha2 milestone Mar 21, 2018

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

This was reverted by #13703, so I guess audio does not need to be done the same way.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2018

right, as far as I can tell guiInfoManager only loads additional info for audio tag that is only used by gui.
is this correct?

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

Yes, as far as I can tell too, just additional info (e.g. more info about the artists and album) and art for the GUI, although often the art is already there as well. And render stutter (what @popcornmix was originally trying to fix back in 2016 with the async job) is unlikely to be an issue with music.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

The failure with video was rather worse than just stutter. The Renderer Configure call would time out and fail, resulting in video playing in the background or no video at all.
Not sure if music playback would have similar failures, but database calls can be very slow (e.g. external MySQL database on NAS that has spun down its hard disks) so blocking GUI thread is certainly undesirable.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2018

PAPlayer does not need the main thread like VP does. DB access for music items on the main thread should not harm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.