It appears that the root cause is that playlist GUI uses copies of the FileItem objects that are in the playlist. The result of this that the resumepoint and startoffset are set in the wrong place (the "gui" copy item) and then the playlistplayer starts the playback of the actual playlist (library) item, which does not have these values set...
Next to the problem described in the ticket (cannot resume in playlist), once fixed, this has the additional effect that when playback of a movie via the library, the resume point in the playlist GUI "playing now..." is out of date, because it does not get refreshed.
@Montellese could you double check. I looked for a more elegant way but there does not seem to be any due to the multiple copies of fileitems.
You reset it so that it gets fetched by the background loader?
If I understood correctly the problem is that the CFileItem objects used by g_playlistPlayer and the ones in CGUIWindowVideoPlaylist are not the same right? In that case I fear that resume point is not the only property that might cause problems. Actually any property that is changed by the user in the GUI after an item has been added to a playlist will probably not be synced with the CFileItem in the playlist.
@Montellese that is correct. CPlaylist members in the g_playlistPlayer hold different CFileItem objects which point to the same physical File. Now that's not a problem since at the point of wanting to play (OnPlayMedia) the only thing that is important is whether or not the user decided to resume or not, so the resumepoint and m_startOffset value. The reset is used to force fetching the actual offset from the database (the m_resumePoint->IsSet() call would then return false, so that eventually the db is consulted again).
Conversely, the playlistPlayer should not really make changes to any CFileItem properties, because it's just playing the file - with the exception again the resumepoint. That's why in the OnMessage INIT event the same thing is done to the resumepoints of the CFileItems in the CGUIWindowVideoPlaylist are also reset (yes, to be picked up either by the background loader, or directly when trying to fetch the resumepoint e.g. when displaying the context menu).
Right, I forgot about the resume point check in PlayMedia(). I'm OK with this. Probably best ping @jmarshallnz as well.
@jmarshallnz I recall a comment that the IsSet() was introduced to deal with the plugin situation you describe. Are there any plugins that use the Video Playlist Window?
If that's the case then the solution is relatively easy, i.e. not to reset() but to actually 'set' the playlist item's resumepoint (and offset) so that existing behaviour for plugins is maintained.
Alternatively, with a test for IsVideoDb() I could determine whether or not a reset() call on the resumepoint is justified. This would actually be my preference...
UPDATE: IsVideoDb() is actually incorrect - should be something more like " ! IsPlugin()"
@jmarshallnz - I checked the plugin situation with "TED Talks" but this one stores bookmarks in our videodb. Looking further, according to SaveFileStateJob.h anything that is not LiveTV (even PVR...) saves their bookmarks in the videodb, however PVRChannel seems to be skipped at the Application.cpp level... so I guess that this (also) needs to be excluded.
So I'm wondering what the good approach really is. IMO we should find out where the bookmark is stored. Is it in the db? in the PVR backend? in the plugin (server)? etc. And then if we determine it's in our db, we can do the reset... 'A' way to do it is keep a member variable in the FileItem that knows where the bookmark is coming from. Thoughts?
An alternate fix would be to play the gui item not the playlist item?
@jmarshallnz , yes that would be the solution, but probably too much change.
I have just pushed an updated commit. I believe it's a clean solution, that doesn't take into account types nor does it blindly reset resumepoints.
The approach is
a) when playing from the playlist window, we copy the resumepoint from the GUI item down to the playlist item. We know the GUI item is correct, so that's good.
b) when entering the playlist window, some resumepoints there may be invalid, after playback either through the playlist or the library. That's a bit more tricky but if we only "reset" the resumepoints that are set AND also exist in the videodb, we don't risk of resetting any plugin-set resumepoints (which wouldn't exist in the db.
Fix resume playback from playlist does not work (trac 13929)
commit updated. Now using GUI_MSG_NOTIFY_ALL for correct update of playlist items after saving (better fix for (b) mentioned above).
Looks much better - nice work