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

pvr: Read recording's last played position from field in PVR_RECORDING #1823

Merged
merged 3 commits into from
Mar 7, 2013

Conversation

fetzerch
Copy link
Member

@opdenkamp @Red-F: This is the follow-up on the discussion in #1452

I've added the field iLastPlayedPosition to PVR_RECORDING.
The value in this field is used for showing the resume indicators in the recordings view.
Instead of calling GetLastPlayedPosition for every single recording.

This fixes the high load times of the recording view. Now it's usable again even with a high number of recordings.

When working on this, I recognized that I forgot to add playCount to WriteClientRecordingInfo. e97116b takes care of this.

Cheers,
Christian

@ghost ghost assigned opdenkamp Nov 21, 2012
@fetzerch
Copy link
Member Author

@opdenkamp: sorry, you're right. now the database is used if it's not supported to read the play position from the addon.

@jdembski
Copy link
Contributor

Just to make sure I get this correctly: When setting pCapabilities->bSupportsLastPlayedPosition = false then the last played position will be stored into / fetched from the database - so there is no need to store this information within the add-on if the backend itself does not support it?

@Red-F
Copy link
Contributor

Red-F commented Nov 27, 2012

@jdembski yes that was the idea originally, and as far as I know that is how it is implemented and working now.

If your PVR backend does not support last played position, or you don't want to implement it in the add-on (at this moment) set the capability to false and XBMC uses its database.

@Red-F
Copy link
Contributor

Red-F commented Nov 27, 2012

@fetzerch @opdenkamp : will this be merged before Frodo's GA?

@opdenkamp
Copy link
Member

no more interface changes until after frodo, sorry.

@Red-F
Copy link
Contributor

Red-F commented Nov 30, 2012

No problem, I just wanted to know if I should find time in my agenda this year to implement this.

@opdenkamp
Copy link
Member

please rebase

@fetzerch
Copy link
Member Author

I'm not convinced anymore that this is a good solution.

  1. Do we still improve performance after the recent changes? The values seem to be cached in the db now.
  2. Having 2 different methods for the same thing is not really a nice API.

Imo, we could also remove GetRecordingLastPlayedPosition and only use the field in PVR_RECORDING?
I don't know if this works for all addons. For MythTV, we get notifications as soon as a recording has been changed (for example another frontend plays the recording). We cache all recordings in the addon and don't have to query the backend any more. We would then call TriggerRecordingsUpdate so that the value in xbmc gets updated.
That way, the API would be more consistent as it would match the API for the play count.

Any thoughts? I will try to rebase it at the weekend so that we can see if it makes a difference performance wise.

@Red-F
Copy link
Contributor

Red-F commented Feb 13, 2013

ARGUS TV does not send events. Clients that play a recording will update the position on the server, but the server does not push out events. This mainly due to the fact that the ARGUS TV api is a pure RESTfull api, patiently waiting for requests.

There are some plans to extend the api via the use of SignalR in the future for easy real-time event pushing by the server. But this is just under consideration and not available any time soon (and integrating that with XBMC will probably a challenge by itself when it is available 🍭 :).

@fetzerch
Copy link
Member Author

Ok. As long as there are addons that don't get notified about such changes, it is needed.
I've rebased it and tested it with MythTV.

@opdenkamp
Copy link
Member

goes in with #2250

opdenkamp added a commit that referenced this pull request Mar 7, 2013
@opdenkamp opdenkamp merged commit 2a5b865 into xbmc:master Mar 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants