[PVR] Add IsRecordable, IsPlayable, GetEpgTagUrl to PVRClient #10363

rbuehlma · 2016-08-29T23:31:42Z

My IPTV provider allows recordings back in the past. As Kodi currently does not allow that, I would like to give the PVR addon the possibility to decide if a EPG entry can be recorded or not. If the addon does not implement the functions (returns PVR_ERROR_NOT_IMPLEMENTED), the old behaviour will be used.

Please let me know if there is a preferred way for this to be implemented. And lets hope I didn't touch a deferred feature again ;)

xbmc/addons/PVRClient.cpp

@@ -730,6 +730,35 @@ PVR_ERROR CPVRClient::RenameChannel(const CPVRChannelPtr &channel)
  return retVal;
 }

+PVR_ERROR CPVRClient::IsRecordable(const EPG::CEpgInfoTag *tag, bool *isRecordable) {


AlwinEsch · 2016-08-30T05:02:38Z

The idea to have something for this is nice and the basic way looks good for me.

Maybe we can add a new Error like PVR_ERROR_NOT_RECORDABLE

     if(client->IsRecordable(tag, &isRecordable) == PVR_ERROR_NOT_RECORDABLE)
       return false;
     isRecordable = tag->EndAsLocalTime() > CDateTime::GetCurrentDateTime();

AlwinEsch · 2016-08-30T05:08:27Z

Also we can use maybe direct a true/false of the return on IsRecordable. So if a normal add-on support recordings it always returns true as default otherwise false and always the same as on PVR_ADDON_CAPABILITIES.

And the if needed individual set.

ksooo · 2016-08-30T05:52:01Z

Thanks for the contribution, looking good in general.

Some comments from first quick review:

I agree to @alwinus on the signature for the new api function, returning a bool is fine here, no error code needed => EDIT: I agree not. ;-) See my next comment.
A pvr add-on api bump is needed, but missing in the PR
have you checked that the feature also works if you are for instance in the channels window or pvr guide window? Just asking because you only modified the guide dialog, but there are other places where a decision whether a broadcast can be recorded is done.
your change can be earliest merged for Kodi v18, because for Krypton we're in besta now and now api changes are allowed at this stage

ksooo · 2016-08-30T06:30:24Z

Let's start with getting the api defined before commenting on code that might be obsoleted once we have a decision on that (mainly a note to myself). ;-)

My take (after thinking about this some more minutes):

The signature as it currently is, is fine
Return ERROR_NONE: Kodi takes the bool value from the api function to decide on recordability
Return any other error: Kodi decides on recordabiliy according to start and end time of the epg tag, like it does today
no need for an additional error code, especially as "cannot be recorded" is not an error!
if we would change return value to bool, Kodi no longer can decide whether to use start / end time to decide, it had to take the value as returned by the add-on. The latter implies that every (!) add-on must properly support the new api function. This is problematic as not all add-ons have maintainers any more and the proper implementation for most of the add-ons is more complicated than just "return true/false;"

ksooo · 2016-08-30T08:12:06Z

@AlwinEsch ^^^ is my proposal to let the new api function like it is designed now, okay?

rbuehlma · 2016-08-30T08:42:53Z

Wow, thanks for the fast feedback. Its perfectly fine to have this PR only in v18.
I will work on feedback the next days.

Do you prefer incremental commits or shall I amend the current one?

have you checked that the feature also works if you are for instance in the channels window or pvr guide window? Just asking because you only modified the guide dialog, but there are other places where a decision whether a broadcast can be recorded is done.

You are right, my first thought was that its not possible to select past events elsewhere, but this call should also be used in the other cases. I will pick this up.

rbuehlma · 2016-08-30T08:55:54Z

BTW: I did write down some ideas of what I would like to implement for IPTV in the board:
http://forum.kodi.tv/showthread.php?tid=287864
This PR is one of them.
My next attempt would be to also allow "Switch" to an EPG entry (e.g. select a past (or even future) show and play it instead of recording it).
May be you could comment on these ideas in the thread if you have some input.

FernetMenta · 2016-08-30T11:10:03Z

Why do you want to overcomplicate things? Just drop the entire check and have the timer creation fail if recording is not supported on a show.

rbuehlma · 2016-08-30T11:21:13Z

Why do you want to overcomplicate things? Just drop the entire check and have the timer creation fail if recording is not supported on a show.

The check is also used to decide whether the "Record" button is shown. In my opinion, we could drop the check in PVRTimerInfoTag.cpp, but the remaining of the code would still be required for the button.

ksooo · 2016-08-30T11:22:01Z

@FernetMenta I don't want to handle all the bug reports saying "kodi recording does not work. There is a 'record' menu item and sometimes when I use it I get an error message. Please make that there is no 'record' item if the event can't be recorded." ;-)

xbmc/addons/PVRClient.cpp

+    pvrTag.startTime = t;
+    tag->EndAsUTC().GetAsTime(t);
+    pvrTag.endTime = t;
+    pvrTag.iUniqueBroadcastId = tag->UniqueBroadcastID();


rbuehlma · 2016-08-30T17:36:18Z

If I did not miss one, I have implemented all your feedback.

I hope I did also find all the other checks which should be replaced with this new function.

xbmc/addons/PVRClient.h

@@ -707,6 +708,13 @@ namespace PVR
    static void WriteClientChannelInfo(const CPVRChannelPtr &xbmcChannel, PVR_CHANNEL &addonChannel);

    /*!
+     * @brief Copy over epg info from CEpgInfoTag to EPG_TAG.
+     * @param tag The epg tag on XBMC's side.
+     * @param addonChannel The epg tag on the addon's side.


rbuehlma · 2017-02-20T18:19:12Z

If a path is prefixed with "pvr", the pvr addon must handle the stream.

Doesn't that contradict the separation of PVR-API and InputStream? As far as I unterstood the PVR addon should only be responsible to provide the URL which is then handled by one of the inputstream addons. Is this wrong?

FernetMenta · 2017-02-20T19:13:40Z

Nope, this is not wrong but it has to apply to the rules already in place. A resource prefixed with "http://" won't get picked up by some ftp handler and a resource prefixed with protocol "pvr" won't get picked up by an mpeg-dash addon. We must not abuse what has been designed to be a protocol. pvr is actually a protocol implemented by addons like vnsi or tvh. The problem is that it got abused and we need to take corrective action.

One of the first things that need be corrected is prefixing channel items with "pvr://".

rbuehlma · 2017-02-20T21:34:02Z

Ok, if I understand this correctly, an EPG item (and also other PVR related items) should always produce a FileItem containing a "pvr://".
This URL will then be handled by the corresponding PVR-addon.
Is that correct so far?
If yes, how should the PVR-addon then pass the actual URL (e.g DASH or what ever) to the inputstream addon?

FernetMenta · 2017-02-20T21:53:34Z

Ok, if I understand this correctly, an EPG item (and also other PVR related items) should always produce a FileItem containing a "pvr://".

This is how it's currently done. This is a big issue. Again: "pvr://" is a protocol that is not applicable to all pvr addons. You know what a protocol is, don't you?

rbuehlma · 2017-02-20T22:05:40Z

A protocol defines how something is transferred but not what it transfers. If there is no further specification of the pvr://, it can be perfectly fine to use it to return the actual stream URL to the player (similar to http 302).

Is there some specification of that protocol available?

Its still completely unclear to me on how the desired solution will look like. What happens after the user has selected a show (epg tag), channel or recording to de played? Maybe its easier for you to describe the approach than for me trying to guess it.

FernetMenta · 2017-02-21T06:38:02Z

Maybe its easier for you to describe the approach than for me trying to guess it.

pvr architecture is exhausted and I know many weaknesses but this does not mean that I have a solution for everything.

pvr://, it can be perfectly fine to use it to return the actual stream URL to the player (similar to http 302)

Sure but this mechanism is not implemented in Kodi. FileItem is a generic element and you would need to touch many other components than only pvr. Every component that works with FileItems would be required to resolve this kind of redirect url before knowing how to deal with the item.

Allow the PVR Addon to decide wether a given EPG entry is recordable or not.

rbuehlma · 2017-07-09T20:33:51Z

Rebase to current master.

Add "Replay program" Use IsRecordable for "Record" Make "Smart select" replay program if IsPlayable

rbuehlma · 2017-07-10T19:20:50Z

I have added the new functionality to the context menu of the guide. Also "Smart select" does now replay a program if possible.

ksooo · 2017-07-10T20:09:38Z

As I explained in the forum, I think that using recordings to achieve replay functionality is the right approach. Almost everything we need for "replay" is already in place and possible today with recordings. Recordings conceptional fit really nice here, because a replayable item for me feels exactly like a recording - with the only difference that somebody else did the recording, not me. Why should we introduce new API functions for something we already have?

The only unsolved problem with recordings as they work today is memory usage, as for your use case there is large amount of recordings involved. Currently the list of all available recordings is fetched from the client during Kodi pvr startup. For your use case this could get a killer memory-wise. I suggest to concentrate on solving this general conceptional problem instead of introducing more and more of functionality for something we basically already have.

xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_dll.h

+   * @remarks Optional, return PVR_ERROR_NOT_IMPLEMENTED to let kodi decide
+   *
+   */
+  PVR_ERROR IsRecordable(const EPG_TAG &tag, bool *isRecordable);


xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_dll.h

+   * @remarks Required, always return false if not supported by the addon
+   *
+   */
+  bool IsPlayable(const EPG_TAG &tag);


xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_dll.h

+   * @remarks Required, always return -1 if not supported by the addon
+   *
+   */
+  int GetEpgTagUrl(const EPG_TAG &tag, char *url, int urlLen, const CStringPropertyMapPtr&);


xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_types.h

@@ -103,6 +105,10 @@ extern "C" {
    xbmc_codec_id_t   codec_id;
  } xbmc_codec_t;

+  typedef std::map<std::string, std::string> CStringPropertyMap;
+  typedef std::shared_ptr<CStringPropertyMap> CStringPropertyMapPtr;


rbuehlma · 2017-07-10T20:18:20Z

I agree that you could probably implement replay as recordings but there are some recording feature you do not want to have in case of replay. Some I am thinking of:

Should not need to load half of the guide (the past) again as recordings
Should not show up in the recordings list
Should not generate a popups if a recording is planned/completed
Should not be possible to edit/delete in any way
Should not show the "recorded" icon in the guide
and probably more...

I'm therefore not sure if the better way is to "duplicate" some of the feature or implement exception for quite some feature used for recordings. What do you think?

FernetMenta · 2017-07-10T20:30:04Z

there are indeed considerable differences. treating those cases differently, learn, then generalize and factor out common code is imo much better than do generalization up front. the latter always results in a mess.

ksooo · 2017-07-10T21:11:59Z

Should not need to load half of the guide (the past) again as recordings

That's what I said. we should concentrate on solving this.

Should not show up in the recordings list

Why? What would be the disadvantage of having them there?

Should not generate a popups if a recording is planned/completed

Will not happen as the popups are for timers, not recordings.

Should not be possible to edit/delete in any way

Could easily added to the API (just one read-only flag in PVR_RECORDING or the like).

Should not show the "recorded" icon in the guide

There is no such icon. There is a "play" icon if a recording is available for an epg event. This fits perfect, imo.

and probably more...

Name it, then we can discuss it.

rbuehlma · 2017-07-11T07:19:34Z

Should not show up in the recordings list

Why? What would be the disadvantage of having them there?

I as a user would be confused if I find multiple 10k of recordings which I have never recorded. I would need some sort of filter to find my own recordings. Forcing the user to enable a filter to be able to use the recordings is probably not the way to go.

Should not generate a popups if a recording is planned/completed

Will not happen as the popups are for timers, not recordings.

If I remember it correctly, I get a popup if a new recording is inserted after the initial load, but not sure about that.

Should not show the "recorded" icon in the guide

There is no such icon. There is a "play" icon if a recording is available for an epg event. This fits perfect, imo.

I agree that it would technically fit, but do you want to have this icon on every past show in the guide? I expect that it would look a bit ugly, and you would not find out which show has actually been recorded by you (and will persist after the replay period).

Considering that the transfer of the Replay-recordings needs to somehow be prevented (for example by delivering the "recorded" information together with the EPG item), I do only see the "Play"-action in the guide as a feature I would want to reuse from the recordings.
Did I miss some there?

To minimize the API changes from my approach, we could add "isPlayable" and "isRecordable" to the EPG item and removing these two functions from the API. But retrieving the URL still needs to somehow be done after the user has selected the program to replay.

ksooo · 2017-07-11T08:24:49Z

Sorry, I'm not with you. You need to convince somebody else from the Team if you want to get this PR merged some day or we should agree on the recordings approach. For the latter you have my full backing, including me providing the necessary core and API changes. I don't want to discuss this to death, tbh.

ksooo · 2017-07-11T08:51:23Z

I would need some sort of filter to find my own recordings.

The addon can define virtual folders to organize recordings. For instance, there could be two toplevel folders, one labeled "my recordings", the other "TV program replay". You can even add subfolders for the different channels or whatever you like.

If I remember it correctly, I get a popup if a new recording is inserted after the initial load, but not sure about that.

You don't remember this correctly. Should be no problem at all.

but do you want to have this icon on every past show in the guide?

Yes, I want a "play" icon for every epg item which can be played, no matter who "recorded" this.

I do only see the "Play"-action in the guide as a feature I would want to reuse from the recordings.

Then, a PVR addon is the wrong container for what you want to implement. Write a video addon, then.

Wait .. didn't you say that one can make personal recordings from theEPG as well. I this case, you possibly want full recordings functionality... Or what is "IsRecordable" for?

Considering that the transfer of the Replay-recordings needs to somehow be prevented (for example by delivering the "recorded" information together with the EPG item)

To minimize the API changes from my approach, we could add "isPlayable" and "isRecordable" to ...

Please don't get me wrong, but I have the bigger picture. Even adding small things like isplayable and isrecordable in whatever way (function, struct field - does not matter) means, that every (!) pvr addon implementation needs to be adapted to support this properly or ugly chunks of compatibility code need to be injected into the Kodi pvr code ("if functionality supported do it the new way if not do it like in the past" - no go for this!). Given that some of the addons don't have active maintainers anymore changes like you suggest will lead to the immediate death of those addons - which I want to prevent if possible.

rbuehlma · 2017-07-11T10:30:34Z

Well, I'm fine with also implementing your proposed approach. I'm not strictly against it, I'm just not sure which one will cause less trouble :)

Probably the best is, to try both since I don't expect a lot implementation effort.

For me, the most unclear point is this:

Should not need to load half of the guide (the past) again as recordings

That's what I said. we should concentrate on solving this.

Do you have something in mind how we could solve that? First try to simply load them as recordings an see how it behaves?

ksooo · 2017-07-11T10:35:18Z

First try to simply load them as recordings an see how it behaves?

This would be a good first step, yes. If it turns out then that there will be no problems, we would have saved much time for a concept and implementation nobody needs. ;-)

Can you do me a favor and close this PR and open another one for the new approach. This PR here has so many comments and stuff that it is really hard to keep track what's still of interest...

rbuehlma · 2017-07-11T12:43:38Z

Ok, I'll implement the second approach in the next days and will create a new PR for that one. Until we have one of the solutions accepted, I will keep this one open. Is that ok?

ksooo · 2017-07-11T13:20:23Z

Ok, I will just stop watching this PR.

rbuehlma · 2017-08-22T11:31:49Z

Superseeded by #12689

AlwinEsch reviewed Aug 30, 2016
View reviewed changes

AlwinEsch added Type: Feature non-breaking change which adds functionality Component: PVR API change: PVR labels Aug 30, 2016

ksooo added the v18 Leia label Aug 30, 2016

ksooo changed the title ~~Add IsRecordable to PVRClient~~ [PVR] Add IsRecordable to PVRClient Aug 30, 2016

ksooo reviewed Aug 30, 2016
View reviewed changes

xbmc/addons/PVRClient.cpp Outdated

pvrTag.startTime = t;

tag->EndAsUTC().GetAsTime(t);

pvrTag.endTime = t;

pvrTag.iUniqueBroadcastId = tag->UniqueBroadcastID();

This comment was marked as spam.

Sign in to view

ksooo reviewed Aug 30, 2016
View reviewed changes

rbuehlma added 3 commits July 9, 2017 20:20

Add IsRecordable to PVRClient

1c4b96a

Allow the PVR Addon to decide wether a given EPG entry is recordable or not.

Implement IsPlayable and GetEpgTagUrl to support Replay-TV

aab5bb8

Allow providing file item properties from the addon

9820f4e

rbuehlma force-pushed the pvr_client_is_recordable branch from 8ce9193 to 9820f4e Compare July 9, 2017 20:32

Extend guide context menu

181cdec

Add "Replay program" Use IsRecordable for "Record" Make "Smart select" replay program if IsPlayable

rbuehlma force-pushed the pvr_client_is_recordable branch from 0215627 to 181cdec Compare July 10, 2017 19:11

ksooo requested changes Jul 10, 2017

View reviewed changes

ksooo mentioned this pull request Aug 18, 2017

[PVR][addons][guiinfo][Estuary] PVR Addon API 5.7.0: New functions for EPG tags. #12689

Merged

rbuehlma closed this Aug 22, 2017

rbuehlma deleted the pvr_client_is_recordable branch August 22, 2017 11:32

[PVR] Add IsRecordable, IsPlayable, GetEpgTagUrl to PVRClient #10363

[PVR] Add IsRecordable, IsPlayable, GetEpgTagUrl to PVRClient #10363

Conversation

rbuehlma commented Aug 29, 2016

This comment was marked as spam.

This comment was marked as spam.

AlwinEsch commented Aug 30, 2016 • edited

AlwinEsch commented Aug 30, 2016 • edited

ksooo commented Aug 30, 2016 • edited

ksooo commented Aug 30, 2016 • edited

ksooo commented Aug 30, 2016

rbuehlma commented Aug 30, 2016

rbuehlma commented Aug 30, 2016

FernetMenta commented Aug 30, 2016

rbuehlma commented Aug 30, 2016

ksooo commented Aug 30, 2016

This comment was marked as spam.

rbuehlma commented Aug 30, 2016

This comment was marked as spam.

This comment was marked as spam.

rbuehlma commented Feb 20, 2017 • edited

FernetMenta commented Feb 20, 2017

rbuehlma commented Feb 20, 2017

FernetMenta commented Feb 20, 2017

rbuehlma commented Feb 20, 2017

FernetMenta commented Feb 21, 2017

rbuehlma commented Jul 9, 2017

rbuehlma commented Jul 10, 2017

ksooo commented Jul 10, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

rbuehlma commented Jul 10, 2017

FernetMenta commented Jul 10, 2017

ksooo commented Jul 10, 2017 • edited

rbuehlma commented Jul 11, 2017

ksooo commented Jul 11, 2017

ksooo commented Jul 11, 2017 • edited

rbuehlma commented Jul 11, 2017

ksooo commented Jul 11, 2017 • edited

rbuehlma commented Jul 11, 2017

ksooo commented Jul 11, 2017

rbuehlma commented Aug 22, 2017

AlwinEsch commented Aug 30, 2016 •

edited

AlwinEsch commented Aug 30, 2016 •

edited

ksooo commented Aug 30, 2016 •

edited

ksooo commented Aug 30, 2016 •

edited

rbuehlma commented Feb 20, 2017 •

edited

ksooo commented Jul 10, 2017 •

edited

ksooo commented Jul 11, 2017 •

edited

ksooo commented Jul 11, 2017 •

edited