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][guilib][json][skin.estuary][skin.estouchy] Add new API HasArchive for ListItem #14736

Closed

Conversation

@arthur-liberman
Copy link
Contributor

commented Oct 29, 2018

Description

This PR adds a new API for ListItem. The skin can now query the channel or the program in the EPG whether it can be "Played from EPG tag" or not (introduced in 21900fd).
I'm not sure about the naming used as "archive", and if you can think of a better naming scheme for this I'm all for it.

Motivation and Context

In some cases this functionality is not supported on all available channels, and it is very useful to have a visual indication of whether a channel/program supports it or not.

Screenshots (if appropriate):

image
image

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed
@MartijnKaijser MartijnKaijser requested a review from ksooo Oct 29, 2018
@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Thanks for this nice contribution.

First, as this PR includes PVR add-on API, JSON API and database version bumps it can't make it into v18, sorry. We're nearing RC phase and incompatible API changes are not possible at this stage. So, this one is for v19.

Code looks pretty much okay, I will do a review later.

I'm also uncertain about the wording - we can figure this out later.

Questions:

  • can channels without EPG have "archive" support?
  • which pvr add-on did you extend to test your charges / make the screen shots? Show me the code, please.
  • the green dots on the epg grid - this is "ListItem.IsPlayable ", right?
@@ -1016,7 +1016,8 @@
"icon": { "type": "string" },
"channelnumber": { "type": "integer" },
"subchannelnumber": { "type": "integer" },
"isrecording": { "type": "boolean" }
"isrecording": { "type": "boolean" },
"hasarchive": { "type": "boolean" }

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 29, 2018

Member

Thanks for updating the API as part of this feature, and for correctly adding the field at the end of the type.

However addition of fields to the JSON schema requires a bump to the minor version number (the middle one) in https://github.com/xbmc/xbmc/blob/master/xbmc/interfaces/json-rpc/schema/version.txt as a non-breaking change, communicating to API customers that things have changed. Adding [JSON] to the commit comment would also make it easier to spot.

This comment has been minimized.

Copy link
@arthur-liberman

arthur-liberman Oct 29, 2018

Author Contributor

Thanks. I'll do that once I'm home.
Should I make another commit, or amend the previous one and force push? I'm not yet familiar with how PRs to Kodi work.

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Oct 29, 2018

Member

You can amend and force push changes. You can just do Fixup commits for now and in the end squash them into clear separated changes.

This comment has been minimized.

Copy link
@arthur-liberman

arthur-liberman Oct 29, 2018

Author Contributor

Done.

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@ksooo
Some services refer to this as "catchup", but I think Archive is still a very common term for it.

  • Channels without EPG can have archive support. This will depend on the PVR add-on implementation of course, but as long as the channel has support for it, you can timeshift "blind", without knowing exactly on what show you land.
  • I extended (and renamed) pvr.iptvsimple, but it includes more functionality than just support for this. It's currently in patch form, and I have a PR open for it here: CoreELEC/CoreELEC#138 (oops, I'll need to update that PR with the changes for the HasArchive flag)
    I did test Kodi with just the changes from this PR (HasArchive) with it, and it worked as expected.
  • I tried ListItem.IsPlayable, that did nothing, I don't think there is an implementation for it as I did a quick search for it. The green dots is the new "ListItem.HasArchive", I simply added a new png to the Confluence skin and a new image element, it only shows the green dot on shows that are "IsPlayable" in their EPGInfoTag + not recording, same for channels.
@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

I extended (and renamed) pvr.iptvsimple, but it includes more functionality than just support for this. It's currently in patch form, and I have a PR open for it here: CoreELEC/CoreELEC#138 (oops, I'll need to update that PR with the changes for the HasArchive flag)

I'm STRONGLY against extending random distros with heavily patched versions of official Kodi addons and Kodi itself! If you want to do this kind of stuff you have to rebrand Kodi and the addon.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

I just noticed that you renamed the addon. Doesn't realy make it overall better, imo.

Why are you doing PRs for CoreELEC instead for xbmc / kodi-pvr ? Don't you want your features in the upstream products?

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

I would personally love to see support for this in Kodi. I couldn't find a relatively simple and non-intrusive way to do that without resorting to the changes I made. It's possible I'm missing something, but I believe a proper implementation is going to require much more in depth changes, that I'm not sure I have the knowledge to do right now.
There is currently no way to play a tag from EPG in "Live TV" mode. So that very ugly "hack" in PVRManager is not something I made the decision on lightly. And it's definitely something that's going to have to go sooner rather than later.
But really, the user experience with this particular change, where the program simply "timeshifts" back, instead of playing as a "video" is really great. So it's something that perhaps should be considered as an option for v19.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

I tried ListItem.IsPlayable, that did nothing, I don't think there is an implementation for it as I did a quick search for it.

Kodi core implementation is in place, but your addon needs also to implement it. There is an PVR Addon API function for this: https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_dll.h#L107

@vpeter4

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

Addon is already using IsEPGTagPlayable.

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

Ah yes, that works. I meant that it's not implemented for ListItem, for the skin to use as an indication of whether the item is playable or not.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Ah yes, that works. I meant that it's not implemented for ListItem, for the skin to use as an indication of whether the item is playable or not.

Ah, right. I thought we have "ListItem.IsPlayable". I was wrong on that.

@Ray-future

This comment has been minimized.

Copy link

commented Oct 29, 2018

I extended (and renamed) pvr.iptvsimple, but it includes more functionality than just support for this. It's currently in patch form, and I have a PR open for it here: CoreELEC/CoreELEC#138 (oops, I'll need to update that PR with the changes for the HasArchive flag)

I'm STRONGLY against extending random distros with heavily patched versions of official Kodi addons and Kodi itself! If you want to do this kind of stuff you have to rebrand Kodi and the addon.

So pvr.iptvsimple is your trademark as well?

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

There is currently no way to play a tag from EPG in "Live TV" mode. [...] But really, the user experience with this particular change, where the program simply "timeshifts" back, instead of playing as a "video" is really great.

Have you tried skip+/skip- while playing Live TV using a PVR addon that properly implements timeshifting? => #14387

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

I just found out about it a couple of days ago, actually.
I tried it with the patched iptvsimple and it works perfect, but I haven't tried other add-ons with it.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

So pvr.iptvsimple is your trademark as well?

IANAL, but this is not about trademarks. ID change is about working versioning and updates for the official addon (and the forked addon). Name change is about avoiding confusion for users and people trying to give support for the official addon (and the "forked" addon).

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Just a heads up: Any further non constructive comments will be removed without questioning. Keep comments about the code implementation and potential issues.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Yes, I just found out about it a couple of days ago, actually. It works perfect.

So, I guess, for best user experience you should implement proper timeshifting for your addon - no special hacks needed. There are "templates" for this (local ts buffer if no serverside ts) - for example pvr.vuplus has it.

But this is ofc not scope of this PR and should not be discussed further here, it just came to my mind.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

So, I guess, for best user experience you should implement proper timeshifting for your addon - no special hacks needed.

If I understand the coreelec patch halfway right, then implementing timeshifting support for official pvr.iptvsimple would be the way to go as it will give the users what you want to achieve with pvr.iptvarchive + Kodi patches, but in a clean and acceptable way. What do you think? You could for instance ask @phunkyfish if you need help with that. He just did it for pvr.vuplus. Users will love you if you extend pvr.iptvsimple with proper timeshifting, btw. ;-)

So, back to this PR, which is generally fine as I said before... I will take a closer look at the code changes the next days.

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@ksooo Is there a place where we can discus this further? One obstacle I hit was with DVDFactoryInputStream.
m3u8 based playlists are relegated to CDVDInputStreamFFmpeg which doesn't even support the PVR methods. I think with a little bit of cooperative work, on both Kodi and iptvsimple side, we can get this working better and more seamless than it is working right now with the patches.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

I'm not very familiar with videoplayer internals. @FernetMenta what's your take on this?

@phunkyfish

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

@ksooo @arthur-liberman Happy to help with timeshifting for IPTV simple.

@arthur-liberman arthur-liberman force-pushed the arthur-liberman:add-listitem-hasarchive branch from e433dbf to 766bacb Oct 29, 2018
@arthur-liberman arthur-liberman changed the title [pvr][guilib] Add new API HasArchive for ListItem [pvr][guilib][json] Add new API HasArchive for ListItem Oct 29, 2018
@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@phunkyfish The Archive feature allows us to have 'pseudo-timeshift'. Basically it works like this:
IPTV providers have an API for accessing the archive. Some build it right into the playlist, using the catchup-source #EXTINF tag. There is a separate URL to access the channel at a specific time stamp using a placeholder for the start time of the program in UTC.
There is more information in those tags, such as how many days back you can access on said channel. Others require you to append parameters to the channel URL, such as ?utc={start-time-utc-timestamp}&lutc={current-utc-timestamp}. This format is supported by many players, such as OTT Player.
Now, to timeshift, all we need is to know the time we want to start watching, and restart the stream with a new timestamp in the URL.
This also allows you to have a timeshift buffer that is as long as the whole archive duration, because nothing is physically stored on your device. With #14387 you can skip whole programs back/forward with ease.

Supporting archive/timeshift in playlists that have the catchup-source url built in is easy, because it's provided to us. The question then becomes what to do with the services that require URL parameters, because these formats can differ from one service to the next. I think that for starters, adding support for catchup should be the first step, and we can figure out what to do with the services that require parameters later.

@phunkyfish

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

Just let me know when/if I can help and happy to do so.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Can you guys please move the discussion not related to this PR elsewhere. Otherwise I'm loosing track here. Thanks.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

I suggest to open a thread in the kodi forum.

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

I agree. I opened a thread about where I got stuck with this here:
https://forum.kodi.tv/showthread.php?tid=337062
@phunkyfish your help would be appreciated.

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

Edit: See #14736 (comment)

@ksooo ksooo added the v19 Matrix label Nov 1, 2018
Copy link
Member

left a comment

some minors only.

xbmc/pvr/PVRGUIInfo.cpp Show resolved Hide resolved
xbmc/GUIInfoManager.cpp Show resolved Hide resolved
xbmc/pvr/channels/PVRChannel.h Show resolved Hide resolved
@arthur-liberman arthur-liberman changed the title [pvr][guilib][json] Add new API HasArchive for ListItem [pvr][guilib][json][skin.estuary] Add new API HasArchive for ListItem Nov 3, 2018
@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

It just occurred to me that Estuary is a part of the XBMC repo. I added HasArchive and IsPlayable support to it. Now this PR can be tested more easily.
I have setup a branch with an updated pvr.iptvsimple to enable testing this PR.
https://github.com/arthur-liberman/pvr.iptvsimple/commits/archive

@arthur-liberman arthur-liberman force-pushed the arthur-liberman:add-listitem-hasarchive branch from c4860b0 to d7c8841 Nov 5, 2018
@arthur-liberman arthur-liberman changed the title [pvr][guilib][json][skin.estuary] Add new API HasArchive for ListItem [pvr][guilib][json][skin.estuary][skin.estouchy] Add new API HasArchive for ListItem Nov 5, 2018
@arthur-liberman arthur-liberman force-pushed the arthur-liberman:add-listitem-hasarchive branch from d7c8841 to 67a302b Jan 31, 2019
This separates the EPG tag from HasArchive.
@arthur-liberman arthur-liberman force-pushed the arthur-liberman:add-listitem-hasarchive branch from 67a302b to 07c05e5 Apr 19, 2019
@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

Can I have a status update on this PR? I saw that Kodi has went through some restructuring of the PVR, but after fixing conflicts, this PR still works as expected.

@ksooo

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

I most probably will cherry pick this PR for the next PVR Addon API change PR I'm currently preparing for v19.

@arthur-liberman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Superseded by #15953

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