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] Addon API 5.0.0 #8736

Merged
merged 19 commits into from Feb 16, 2016

Conversation

@ksooo
Copy link
Member

ksooo commented Jan 2, 2016

I'd like to propose the following extensions and changes to the PVR Addon API for Krypton:

  1. Introduce "no guilib used" return value for GetGUIAPIVersion and GetMininumGUIAPIVersion. Addons not using guilib now are able to state this, making it unnecessary to recompile these addons on bump of guilib. Discussion on this: kodi-pvr/pvr.hts#153
    Addon Maintainer Todo: If GUI lib is not used by the addon, GetGUIAPIVersion and GetMininumGUIAPIVersion should be changed to return "" (empty string). Additionally, addon.xml.in should be changed to not include
  2. Introduce a callback function that can used by addons to propagate connection state changes. This makes it possible to inform kodi (users) on events like "wrong credentials", "version mismatches", "connection loss", "successful reconnect after connection loss", ... all not possible today. Once an addon is loaded and connected, today there is no standard way to propagate those changes. Now, Kodi can react on changes, and not just by displaying a kai toast, but for example also by refetching addon properties (after reconnect, version numbers etc. may be different - because backend was just updateted, for example), which is done today exactly once after the addon has been loaded. Discussion on this: kodi-pvr/pvr.hts#136 (comment)
    Addon Maintainer Todo: no mandatory addon changes. recompile of the addon is sufficient. but feel free to use the new functionality to enhance your addon
  3. Get rid of another magic number: Introduce EPG_TAG_INVALID_UID
    Addon Maintainer Todo: change all code that denotes an "invalid" epg tag uid to use the new constant for that purpose
  4. Remove unused method GetCurrentClientChannel
    Addon Maintainer Todo: yeah, remove that function
  5. Introduce a callback function for asynchronous announcment of epg data transfer and event state changes. If an addon supports this feature PVR startup time can be improved signifilly. Kodi can be provided with "realtime" epg data (immediate UI chnages, no more operating on tags already deleted in the backend etc.). Reference implementation for pvr.hts gives quite impressive results. ;-)
    Addon Maintainer Todo: no mandatory addon changes. recompile of the addon is sufficient. but feel free to use the new functionality to enhance your addon
  6. Add function SetEpgTimeFrame. This one is needed in conjunction with the new epg data callback. Knowing the epg time frame Kodi is interested in the addon can prevent to send dta to Kodi that are actually not of interest (reduces amount of data transferred from addon to Kodi).
    Addon Maintainer Todo: add this function. refer to https://github.com/xbmc/xbmc/pull/8736/files#diff-e73115cc4e6a02ef7029ee04f8ddd2a8R648 for implementation details
  7. Add (optional) field PVR_RECORDING.iChannelUid. Today, it is very expensive to find an epg tag for a given recording due to organization of Kodi core data. Having the channel uid in addition to the epg event uid greatly improves performanace
    Addon Maintainer Todo: no mandatory changes, but highly recommended to fill this field, because it is a prerequisite for performance improvements on the kodi side of things
  8. Remove unused PVR_SIGNAL_STATUS.d*Bitrate fields.
    Addon Maintainer Todo: yeah, just remove this if used in your addon
  9. Get rid of another magic number: Introduce PVR_CHANNEL_INVALID_UID
    Addon Maintainer Todo: change all code that denotes an "invalid" channel uid to use the new constant for that purpose

@FernetMenta @Jalle19 others... Feedback welcome

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jan 2, 2016

@ksooo thanks, nice additions, I like the idea with the new callback if the state is changed and everything else except the last commit ;)

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 2, 2016

@xhaggi see my comment about the last commit. What you fear (not to see progress at all, because startup takes longer than 30 secs) is not true. ;-)

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jan 2, 2016

okay then it's fine by me :)

@ksooo ksooo force-pushed the ksooo:pvr-api-5-0-0 branch 2 times, most recently from 70a495f to d2472d6 Jan 5, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 5, 2016

Feedback from @xhaggi and @FernetMenta incorporated.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 5, 2016

jenkins build this please
(just for testing latest changes - no merge planned yet)

@ksooo ksooo force-pushed the ksooo:pvr-api-5-0-0 branch from d2472d6 to 3ea7eac Jan 5, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 5, 2016

More feedback from @xhaggi incorporated.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 5, 2016

jenkins build this please

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jan 5, 2016

@ksooo great thanks :) next would be dropping the readytouse member but as you said it is something for a separate PR

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jan 5, 2016

and using more friendly method names like IsConnected instead of ReadyToUse

@ksooo ksooo force-pushed the ksooo:pvr-api-5-0-0 branch from 3ea7eac to 5814df6 Jan 5, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 5, 2016

and using more friendly method names like IsConnected instead of ReadyToUse

yeah, but also in another PR

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 5, 2016

jenkins build this please

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jan 5, 2016

sure :)

@ksooo ksooo force-pushed the ksooo:pvr-api-5-0-0 branch from 5814df6 to dc32ec2 Jan 5, 2016
@Jalle19

This comment has been minimized.

Copy link
Member

Jalle19 commented Jan 8, 2016

@ksooo while you're at it, could you remove the GetCurrentClientChannel() method from the API? As far as I can tell Kodi doesn't use it anywhere.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Jan 8, 2016

@Jalle19 good point. Are there more?

@Jalle19

This comment has been minimized.

Copy link
Member

Jalle19 commented Jan 8, 2016

Not sure, I think that's the only one that's completely unused.

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Jan 9, 2016

Sorry for hijacking but since PVR is really improving and users are really starting to use the full power of it, is there any interested to work on completing the JSON counterpart so that remotes can use that power too ?

Usually I'd talk with Montellese but he have no setup and no real interest in PVR.

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jan 9, 2016

@Tolriq could you please create a forum thread and tell us which parts we neee to expose over json-rpc. that would be highly appreciated.

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Jan 9, 2016

http://forum.kodi.tv/showthread.php?tid=255793, will complete later today or tomorrow wife not happy I'm late :p

@msduketown

This comment has been minimized.

Copy link

msduketown commented Jan 9, 2016

Nice. Some pro-active API compatibility attempts by community members. @opdenkamp is wearn down and now KSOO and Xhaggi are the PVR-managers? Glad to find out..

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Feb 15, 2016

Note: pvr.dvblink and pvr.dvbviewer PRs are intentionally missing, because they require a "real" implementation of API 4.2.0 as they actually support timeshifting. Once 4.2.0 has been implemented for these addons I can submit PRs for 5.0.0

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Feb 15, 2016

If nobody objects I will merge this PR tomorrow.

ksooo added 19 commits Dec 30, 2015
…set from outside. (Needed for new async epg data transfer.)
…xample to speedup and simplify lookup of epg events belonging to recordings.
…ields, including respective PVR_ACTUAL_STREAM_*_BR info labels.
@ksooo ksooo force-pushed the ksooo:pvr-api-5-0-0 branch from 2f31213 to 613539a Feb 15, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Feb 15, 2016

Rebased.

@ksooo ksooo added this to the Krypton 17.0-alpha1 milestone Feb 15, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Feb 16, 2016

jenkins build this please

ksooo added a commit that referenced this pull request Feb 16, 2016
@ksooo ksooo merged commit a0f046c into xbmc:master Feb 16, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
default Merged build finished.
Details
@ksooo ksooo deleted the ksooo:pvr-api-5-0-0 branch Feb 16, 2016
@FernetMenta

This comment has been minimized.

Copy link
Member

FernetMenta commented on xbmc/addons/AddonCallbacksPVR.cpp in aac0d2c Mar 26, 2016

@ksooo just to inform you: I get a hang here because SetConnectionState calls back into the addon. This needs to be handled line the other trigger methods, that schedule a job in PVRManager.
I will change this.

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