[pvr] Show resume indicator for recordings + some cleanups

[fetzerch]

This PR contains some smaller changes closely related to the 'set play count' / 'set play position' on the addon features.

First, it adds the resume indicator (arrow) to the recordings view so that you see the partly watched recordings at a glance. Also when a recoding is marked as watched via the context menu, we now clear the resume bookmark on the backend (SetLastPlayedPosition).

In addition, the PR adds some cleanups. Whereas 'set play count' / 'set play position' are quite similar, they're currently called from completely different locations:
When the player stops, the play count is set in the video database and the play position is set in dvdplayer.
I moved both now to SaveFileStateJob.

@Red-F You are mentioning that you had this in dvdplayer on purpose: opdenkamp#536
If I get the whole player concept right, it should go to SaveFileStateJob to have it also working on rpi and other platforms. Or is dvdplayer used everywhere? Maybe you could explain your point and we can get it fixed.

@opdenkamp I'd also like to unify the method calls a bit. Is it ok to move Set/GetRecordingLastPlayedPosition from PVRManager to PVRRecording (where SetPlayCount is)? Or better move SetPlayCount into the manager? I'd then add an additional commit.

Cheers,
Christian

[Red-F]

Ai Christian, that is some time ago already.

Off the top of my head I seem to recall that there is a certain interval (first 5 minutes?) when you start playing a new recording, where SaveFileStateJob will not get valid resume point info. This is probably done on purpose to allow the user to quickly preview a video file without setting the resume point. Thus he will not see the 'resume/start from the beginning dialog' when he really starts viewing the file.

This however thwarts the exact purpose of set last played position on the PVR server. There we want the exact last position that was last viewed. This allows flawless resume cross clients, since all the clients (xbmc or other) retrieve that position from the PVR server. That was the reason to insert it in DVD player.

I fully agree it is more elegant to have the code in SaveFileStateJob. But I was not able to locate where that decision to send the resume point info to SaveFileStateJob was made or I would have suggested to move the decision to actually save the resume point to there too (and update the PVR server always).

Hope this helps. Can you please verify if after playing a new recording for 30 seconds the correct last watched position info is still being sent to the PVR server?

Cheers,
Fred

[opdenkamp]

@fetzerch moving it to CPVRRecording sounds good to me.

@Red-F if we need a different time for pvr recordings, then that should be changed. moving it into SaveFileStateJob looks like the proper way to do it, not add some hook in DVDPlayer for it (that would need to be present in all players then)

[fetzerch]

In my opinion it's correct to apply the same bookmarking rules also for the backend resume point.
One can still modify the advanced settings (ignoresecondsatstart, default 3min) to change the behavior.
It's then kind of the same behavior that you get for normal movies when using a distributed setup with mysql.

However, if there is a reason to treat recordings differently, it might be modified better here: CApplication::UpdateFileState (https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L4663)

[fetzerch]

Done. The bookmark read from the backend was also lacking the duration.

[jmarshallnz]

Other than those minor comments, this looks good.

[fetzerch]

Thanks, the files are updated according to your comments.
Splitting SaveFileStateJob into h/cpp shouldn't be done in this PR, I guess?
I also squashed down the cleanup commits.

[Memphiz]

Looks good to me.

[opdenkamp]

yup, will be merged in next round

[Red-F]

The GetRecordingLastPlayedPosition was originally only called when starting to play a recording. This change causes it to be called for every recording during construction of the recordings view. Every time the recordings view gets focus. With a couple of hundred recordings this causes a delay of more than 20 seconds (on my test system).

I'm a patient person, but I can see this experience decreasing the WAF factor enormously ;)

Just using up/down arrow in the Live-TV left menu will freeze xbmc every time for a long period.

Retrieving this information is not a cheap operation, a round trip to the REST api of the PVR server takes a penalty.

[opdenkamp]

right, so that's what's causing the delay. thanks for checking. noticed it too, but didn't investigate it yet

[opdenkamp]

right, so this data can be cached quite easily, but we do like to show icons, so we do need to call it once per recording when loading.

[Red-F]

I discovered this when I was testing my final (?) catch-up pull request to be.

Caching is tricky when using multiple clients concurrently, which is the whole point of having this on the PVR server initially.

I'll try to look into this tomorrow evening. Workday coming up :)

[opdenkamp]

cache the value in the entries in pvrrecordings, and update it before showing the context menu or continue recording dialog, that should work just fine?

[Red-F]

Yes, that is the scenario that I came up with too, extend pvrrecording with a last played position field that gets set during the initial retrieval of all recordings (capabilities will show that it is valid or not). That will be non-blocking since it runs on a different thread, also many PVR's (certainly FTR) return the last played position already with the rest of the recording details.

This will also refresh the data when the recordings are refreshed from the server.

And then query the server again when a request to play the recording is received.

Will you be picking this up?

[opdenkamp]

i might have time for this some evening this week, but no promises

[Red-F]

Are we going to bump the API version number?

If you want I can create a pull request to get the struct PVR_RECORDING with an added int iLastPlayedPosition; That would allow add-on authors to anticipate the upcoming change. And then I can try to find some time this week to see if I can implement this change, cause I reckon you must be real busy with all this.

[fetzerch]

I'm happy with the proposed solution and will add the required changes to the mythtv addon.
If you're going to read the exact value again when opening the context menu, wouldn't it be better to use a bool bHasLastPlayedPosition for the resume indicators?

For myth, I can get the information if there is a bookmark quite easily when querying the list of recordings.
But the exact values would still require expensive database calls. (Thus it could be speeded up in single query where as currently it uses one query for each recording)

[Red-F]

From a high level point of view every recording has a last played position (if bSupportsLastPlayedPosition is true in PVR_ADDON_CAPABILITIES). It will be 0 for unwatched recordings. I think the last played position is currently used in the recordings view to distinguish between 'not played', 'playing in progress' and 'fully played'. Showing proper icons accordingly.

This is even more important for those PVR servers that do support Last Played Position but don't support Recording Play Count.

[opdenkamp]

right, so let's add both iLastPlayedPosition and iPlayCount to PVR_RECORDING for this and bump if @cptspiff approves.

[Red-F]

iPlayCount is already part of PVR_RECORDING.