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

Cleanup resume #1220

Merged
merged 8 commits into from Aug 3, 2012

Conversation

Projects
None yet
5 participants
Member

jmarshallnz commented Jul 26, 2012

This cleans up the resume handling to use m_resumePoint in the videoinfotag if available. It means we don't necessarily have to hit the database if we already have the information.

Further, it means that directory classes (eg plugins + upnp) can supply the details for resuming directly for those items that aren't in the local database (indeed, it would bypass the local database). The intention is that a handler (eg using JSON-RPC) for these directory classes would then update the source of that information (eg update a upnp server) when an item from that directory class is stopped part-way.

There is a question of whether resume points should be stored for such items locally at all - currently there is no way to differentiate those, but now that the resume point structure is being used, this could be changed.

Plugins can also override any resume information during setResolvedUrl() on playback should they wish (request from plugin authors).

Comments welcome.

Contributor

alcoheca commented Jul 26, 2012

Thanks for doing this. In terms of your question, I'll modify SaveFileStateJob so tha resume points are passed back to the upnp server rather than the local db if supported. Not the best place in the long run, but at least the implementation would be hidden for now.

Member

jmarshallnz commented Jul 26, 2012

Sounds good - IIRC there's some other minor cases (eg watched status probably needs the same treatment) but I think getting resume point working might point the way with the rest of it.

Contributor

alcoheca commented Jul 26, 2012

Yep PlayCount is next on the list.. I'll have a look at it this morning, maybe move all playcount increments to the SavefileStateJob too. The trick is how to specify that a playcount is known even if it's zero, hopefully I can avoid something like STARTOFFSET_RESUME ;)

Owner

Montellese commented Jul 26, 2012

Apart from that one change I commented on it looks good.

Member

pieh commented Jul 28, 2012

I wonder if we could nuke checking for resume point in thumbloader now? We do check in for playcounts/resume in CGUIWindowVideoNav::LoadVideoInfo for item list that either doesn't have content set or comes from plugin. And movie/episode/musicvideo views in db now contains resume column (was added with Montellese's db optim pr). It seems like there should be no way we reach thumbloader without checking for resume point already?

Member

jmarshallnz commented Jul 29, 2012

Yeah, I think it can be killed off there now.

@jmarshallnz jmarshallnz was assigned Aug 3, 2012

@jmarshallnz jmarshallnz added a commit that referenced this pull request Aug 3, 2012

@jmarshallnz jmarshallnz Merge pull request #1220 from jmarshallnz/cleanup_resume
Cleanup resume
6791924

@jmarshallnz jmarshallnz merged commit 6791924 into xbmc:master Aug 3, 2012

Jonathan Marshall and others added some commits Jul 8, 2012

Jonathan Marshall [bookmarks] adds IsSet() and IsPartWay() to CBookmark 6ba6095
Jonathan Marshall [resume] use CBookmark::IsSet and CBookmark::IsPartWay for various sp…
…ots we check for whether we've set or partly played an item.

Two places need thought:
1. VideoInfoScanner - currently we test IsPartWay() before setting the bookmark (if the user is importing bookmarks).
   Using IsSet() may be more appropriate if this routine can be used to override whatever is in the database already.

2. ThumbLoader.cpp - this used to also check m_resumePoint.type == CBookmark::RESUME. However, nothing sets this to anything
   other than resume.
ba91333
Jonathan Marshall use IsSet() instead of IsPartWay() when scanning in videos (to allow …
…overriding of existing resume points with none) and when receiving the resume item offset (to allow plugins to override existing resume points with none)
43739d2
Jonathan Marshall [resume] cleanup GetResumeString to reuse GetResumeItemOffset 18e16d8
Jonathan Marshall [resume] if a played item has a resume point set in its videoinfotag,…
… use it over and above the database value when resuming in the player
ce606cd
@hippojay hippojay [resume] Add properties to allow "resumepoint" and "totaltime" to be …
…set via setProperty from python. These tags override any resume information in the video database.
13c0875
@hippojay hippojay [resume] CVideoDatabase::GetPlayCounts no longer overrides informatio…
…n set in the items, which we assume may be more up to date.
10509a3
@hippojay hippojay [resume] if plugins set the resumetime in setResolvedURL(), then assu…
…me they want the item to resume.
d82d1a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment