Kodi Community Forum

Full Version: Discussion of Pull Request #5208 [pvr] basic series recording support
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Pages: 1 2
For those who dont know there is a pull request from Glenn-1990 on github to add core support for Series Timers in Kodi 15 I*****.

Wonderful to see!

After working on pvr.wmc to support this new "Advanced Timers" Kodi implementation I had numerous items to discuss and feedback etc with Glenn and other PVR developers, hence creating this thread.

The reason i make all these suggestions is that once this change is merged, all PVR addons need to make changes anyway, and those with active development that actually want to support series recording need to make more than just trivial changes too. So while we are all in the process of doing that, let's get it RIGHT and make it user intuitive etc... eg by having more sensible concepts around Priority and Lifetime, making user's lives easy by being able to specify their preferred defaults for these options and so on

So here are the different comments/suggestions Id like us to discuss

Existing Addons
given that with this change all existing addons now fail to compile, how should we handle it? The 2 reasons are that the DeleteTimer function now takes an extra bool parameter and the AddonCapabilities structure no longer has a SupportsTimers member. One way would be to update all addons to have the new DeleteTimer prototype but not do anything with the extra boolean, or potentially even return the NOT_SUPPORTED return code if bDeleteSchedule is true. And on the capabilities side obviously remove SupportsTimers and then perhaps even add a default state to the new bitmask flagging that they at least support the non series timer types. The intention being to at least hopefully maintain the way addons are currently functioning. Im not sure whether all addons have acive developers on them or not so obviously something has to be done to get builds to compile in the first place, and ideally even preserving however those addons worked in Helix

Timer Priority Field
Recording Priority being a numeric value from 1 to 99 seems a bit odd. Im not sure what other backends do but this doesnt gel that well for our backend (WMC) which has a very large LONG value for priority but when ueed normally actually allows users to see all pending recordings and arrange them up/down in the list. I cant imagine a user wants to actually change a numeric value between 1 and 99 with a spinner control anyway so its not really intuitive. I realise this is how it already was and you're just re-using the edit timer dialog, but I propose we change the Priority concept for recordings to something more sensible like "Low, Normal, High" options, or even "Lowest, Low, Normal, High, Highest" if people preferred that. I think this is much easier for users and also should be easy for most addons to map into whatever priority concept the backend uses (much more so than a numeric 1-99 value)


Weekday/Weekend Timer Type
The recording types all indicate whether they are for AnyChannel or ThisChannel, except the Weekday and Weekend ones. Shouldnt there really be AnyChannelWeekday/ThisChannelWeekDay and AnyChannelWeekends/ThisChannelWeekends? If not what is the expected value for AnyChannel/ThisChannel when using those weekday/weekend options? In our specific backend we actually have a bitflags enum allowing any day/s of the week to be selected. Obviuolsy we can map weekday/weekend back to these but I dont know whether users would like to have individual days able to be selected or not etc. Im ok with the current list that just includes weekday/weekend (not that id use them personally) but I do say as ive just mentioned that for completeness it does seem like we need the AnyChannel/ThisChannel versions of each

Timer Lifetime Field
The lifetime options could be better and more intuitive - instead of just a number of days I know our backend and I assume others also support things like "Until Watched" "Until Space Needed" "Forever" etc. Perhaps this could be handled just like the recording types by having a larger list of supported values in Kodi, and a bitmask for addons to indicate which options they can support and thus which options can be shown in the list. Another option would be an addon callback to provide the items for these fields, meaning addons could have whatever they wanted here. And then it would be the addons responsibility to have hte strings for these options localised and so on. So in other words our backend might have 1 day, 5 days, Until Watched and Forever, whilst another addon might have 1 day/1 month/etc. I assume the latter is harder to do, but i think the first suggestion would be easy (as easy as the recording types this PR implements anyway) ie come up with a more conclusive hardcoded list and a bitmask in the addon capabilities to indicate which ones the given addon can support

Default Values
If a user always likes to have their AdvancedRecordings on AnyTimeThisChannel, NewOnly, Priority 73, Pre/Post padding 5 minutes etc, we should ideally let them indicate that rather than have to adjust all those parameters every time. Our previous custom rolled series timer support allowed users to specify their preferred defaults for type, pre padding/post padding etc. This could be done in Kodi settings of course, but thats potentially a bunch more settings plus for those with multiple clients they have to configure them all individually... so instead i propose a better way would be a new addon callback GetAdvancedTimerDefaults(type, bNewOnly, prePadding, postPadding) that addons could implement if they want. - with these coming from the backend it means for those users who have multiple clients they wouldnt have to set the settings on all of them


I hope to have some good discussion here that hopefully results in modifying the PR a little bit, and then we addon developers can get cracking on implementing the series support (and in a few cases ripping out the custom rolled one). I think I***** will be great for PVR users experience if we can just get this PR as close to perfect as possible!
I agree 100% - these suggestions would make series timers (oops, I mean advanced timers) far better for our users and much more intuitive. This is a big crank to turn since it means all pvr addons and skin developers will need to make changes so lets get it as good as we can in this release.
Hi,

A few comments on your concerns.

1) Existing addons will just have to replace "bSupportsTimers=true" with "iSupportedTimersMask=7" (or use bitflags) to compile and they should be working the same as they do now.
("bDeleteSchedule" can be ignored in this case)

Example, kodi will only show this 2 options when the user adds a new timer: (more bitflags can be used of course)
Code:
pCapabilities->iSupportedTimersMask = PVR_TIMER_TYPE_ONCE_EPG | PVR_TIMER_TYPE_SERIE_EPG_ANYTIME_THIS_CHANNEL;

2) Priority and lifetime fields: I agree but kodi devs will probably say that this has nothing to do with serie recordings and needs to be in a separate PR.

3) Any channel/this channel can easily be added to weekdays or weekends, but are there addons that will use this?

4) Default values are already used (see tv/timer settings).
(2015-01-10, 19:53)glenn 1990 Wrote: [ -> ]Hi,

A few comments on your concerns.
Thanks for coming to discuss Glenn. Firstly let me say thanks for starting this work and I hope my comments come across as simply wanting to achieve the best result we possibly can Smile

(2015-01-10, 19:53)glenn 1990 Wrote: [ -> ]1) Existing addons will just have to replace "bSupportsTimers=true" with "iSupportedTimersMask=7" (or use bitflags) to compile and they should be working the same as they do now.
("bDeleteSchedule" can be ignored in this case)

Example, kodi will only show this 2 options when the user adds a new timer: (more bitflags can be used of course)
Code:
pCapabilities->iSupportedTimersMask = PVR_TIMER_TYPE_ONCE_EPG | PVR_TIMER_TYPE_SERIE_EPG_ANYTIME_THIS_CHANNEL;
Yep I guess that's essentially what I was saying. Although the bDeleteSchedule cant be totally ignored they at least need the new function prototype or else they fail to compile, but yes they dont have to do anything with the boolean to begin with. So in terms of changing all pvr addons with the 2 "tweaks" is that something you plan to do or would you like someone else to take that on (im happy to submit that PR if you want)

(2015-01-10, 19:53)glenn 1990 Wrote: [ -> ]2) Priority and lifetime fields: I agree but kodi devs will probably say that this has nothing to do with serie recordings and needs to be in a separate PR.
Well im not sure how these big API changes work but i would just want to avoid doing all this stuff, then having ANOTHER PR that yet again bumps the API and requires all PVR addons to have work done and another companion PR on the pvr side, addon sync, etc etc. It doesnt seem too far fetched IMO that this PR could be for revamping timers as opposed to only adding series support and thus include these priority/lifetime changes. But if it's a separate PR i dont really care, but what I do care about is getting the recordings/timers as fully featured as we can in Kodi 15 so I definitely think we want all Kodi side changes to go in, then ONE round of changing all addons to support it

(2015-01-10, 19:53)glenn 1990 Wrote: [ -> ]3) Any channel/this channel can easily be added to weekdays or weekends, but are there addons that will use this?
I guess my question is around your intention - the timer type options are essentially trying to indicate 3 things - whether to use only this channel or any channel, whether to use any time of day or only around this exact time of day and what days of the week to do it on. But using a single enumeration to represent these 3 things mean there are gaps in various things. For completeness it seems like all should exist but if not, somebody (you, and/or all PVR devs) would need to reach some sort of consensus over expected behaviour.
EG if we setup a recording in Kodi for an 8:30pm show and use type "weekends (Sat-Sun)" is it meant to be only for this channel or for any channel and is it meant to be for "around 8:30pm" or "any time"? The same situation exists on the "Daily around this time" and "weekly around this time" where those options dont indicate whether it is for Any Channel or This Channel. So I guess Im asking should the enum be fleshed out even further to provide every permutation? Or perhaps the Channel and StartTime fields shouldnt be greyed out and should allow entry of "This/Any" and "around 8:30pm/any". That would be alot harder I imagine, compared to just having more options in the TIMER_TYPE enum

(2015-01-10, 19:53)glenn 1990 Wrote: [ -> ]4) Default values are already used (see tv/timer settings).
Are you referring to "Settings -> Live TV -> Recording" screen?
Even when in expert mode this only shows default priority, lifetime and start/end padding. There is no default for the preferred advanced timer type, nor is there one for the "new episodes only" flag. Things would be more user friendly if we didnt make users select this everytime.
Not a dev, but would love to see all the options offered by myth back end supported. I could make a list with summaries of what each does if that would help.
One other minor suggestion, since we are going to make a distinction between timers and advanced (series) timers, when users view the Timer list it would be nice if there was an indicator there to tell the user what type of timer it is. In other words, show whether an upcoming timer is part of a series timer or just a one-shot timer. With the custom series timers that our addon has been using up until now, we have been prepending 'series:' to those timer names that are part of a series. It would be nice to do something fancier than that. Wmc uses a multiple recording dot icon, instead of a single recording dot icon, for series timers.
(2015-01-11, 01:12)nickr Wrote: [ -> ]Not a dev, but would love to see all the options offered by myth back end supported. I could make a list with summaries of what each does if that would help.
by all means provide whatever input/info you have from your perspective. Although do bear in mind in the PR discussion on github some talk of the "myriad exotic options" that myth etc do provide already occurred where it was agreed that there need not be support for every single one of them etc. That said, the way the implementation in Kodi has ended up, Kodi doesnt really know/do anything with all these different type options, they just get passed to/from the addon and the addon has to indicate which types it can handle (controlling what is available in the Create Advanced Timer window) so in theory there really shouldnt be a problem with Kodi having however many "types" are needed, since each addon can flag if it supports that type or not and kodi doesnt appear to do anything with it other than display it etc

(2015-01-11, 04:44)krustyreturns Wrote: [ -> ]One other minor suggestion, since we are going to make a distinction between timers and advanced (series) timers, when users view the Timer list it would be nice if there was an indicator there to tell the user what type of timer it is. In other words, show whether an upcoming timer is part of a series timer or just a one-shot timer. With the custom series timers that our addon has been using up until now, we have been prepending 'series:' to those timer names that are part of a series. It would be nice to do something fancier than that. Wmc uses a multiple recording dot icon, instead of a single recording dot icon, for series timers.
The implementation of this PR does indicate the recording type in square brackets after the time details although due to screen real-estate you cant actually see it unless you hover over and let the text scroll. Not sure how to better display it though, an icon is obviously nice but that then implies all skins would need to provide icons and we would need confluence changes etc etc. See the screenshot here where you can only just see the opening square bracket on most entries, and the currently highlighted entry has scrolled into view showing the type:

Image

We may still need to do what we currently do in ServerWMC where we can append/prepend some text such as "series:" to the show name so it is clearer. Unless someone has a good idea on how to indicate these better than the square bracket text that is already out of view

Another thing I was thinking was that in the screenshot there you can see that im seeing every INSTANCE of a series recording in the coming days and potentially that makes it difficult to manage your series recordings. WMC provides a "series recordings" screen that shows you one entry per series recording setup, as opposed to one entry per instance. We could do similar in Kodi, perhaps with a sideblade option/filter for "Only Advanced Timers" or similar. Same as how on the Channel list ther eis a Misc options "Show hidden channels" filter option

I also just noticed that on the Timer screen if you change the "Channel Group" view option in the sideblade it doesnt seem to affect the displayed timers - id expect if i choose my "Kids" channel group, the timers ive set on channels not in that group shouldnt show up. Of course if "any channel" type is enabled it should show up but i have timers that are not "any channel" and they are always in the list regardless of that view setting
OK when I go to record in mythtv I get these basic options: All through I will put my comments in []


Cancel this schedule.
Record only this showing. [ie the one you picked in the EPG to set up this rule]
Find and record one showing of this title. [doesn't have to be at this time, scheduling may put it earlier or later]
Record at any time on any channel. [starting point for series recording]

You can also do overrides for a schedule if you are editing it

Record this specific showing.
Do not record this specific showing.

There are then a number of dropdown advanced options, all these fields and their options would be provided by the server, user just has to select one, except the INET ref where a user has to insert an IMDB number for metdata lookup (I think this is only needed if myth can't figure it out itself, ie for 'difficult' or 'obscure' shows)

Recording Profile:
Transcoder:
Recording Group:
Storage Group:
Playback Group:
Recording Priority:
Duplicate Check method: [this one is quite important to myth users, particularly if the epg data is a bit generic]
Check for duplicates in:
Preferred Input:
Internet Reference #

Of those, the only one I use regularly is the duplicate check method, but other people use them regularly and they are quite powerful.

Then there are filters, these are on/off radio buttons:

New episode:
Identifiable episode:
First showing:
Prime time:
Commercial free:
High definition:
This episode:
This series:
This time:
This day and time:
This channel:

And post processing:

Look up Metadata:
Auto-flag commercials:
Auto-transcode:
User Job #1:
User Job #2:
User Job #3:
User Job #4:

User Job ## will be given a more meaningful name if the user has set up specific user jobs on the backend.

Lastly there are some schedule options:


Inactive:[to turn the rule off but keep it there for when you want it again]
Auto-expire recordings:[quite an important option if you don't want your important stuff to disappear]
Record new and expire old: [great in combo with the next one to keep, say, 10 days news]
No. of recordings to keep:
Start Early:
minutes
End Late:
minutes

As you say most of these can be obtained from and passed straight back to the backend server, but they make for a busy dialog. I can do a screeshot of how they are set out in myth's 10 foot interface if you like, but basically I think you need a multi tab dialog in kodi to cope with all those options.
Ah right, I was thinking you meant more permutations of the how/when types that could just correlate to more types in Kodi. Most of your stuff are specific custom fields (rather than more options on current fields) so I can't really see these Kodi changes facilitating those. The myth addon should probably have a custom dialog that comes after the Kodi one, for your profile, transcoding, etc custom options
Maybe Kodi shouldn't attempt to know it's anything more than a one-off recording, or a series recording.

Maybe the addons should just return a collection of strings describing each type of recording it can do, and maybe some flags for any parameters required, and Kodi just displays these option strings? After all, Kodi's not doing the scheduling itself, and doesn't really know what all the recording options mean.

This would allow each addon to have a different list, that suit's it's capabilities, and even add additional types of recordings outside of the Kodi release cycle, without requiring Kodi to add them to the bitmask of options etc,
(2015-01-11, 08:40)scarecrow420 Wrote: [ -> ]We may still need to do what we currently do in ServerWMC where we can append/prepend some text such as "series:" to the show name so it is clearer. Unless someone has a good idea on how to indicate these better than the square bracket text that is already out of view

Another thing I was thinking was that in the screenshot there you can see that im seeing every INSTANCE of a series recording in the coming days and potentially that makes it difficult to manage your series recordings. WMC provides a "series recordings" screen that shows you one entry per series recording setup, as opposed to one entry per instance. We could do similar in Kodi, perhaps with a sideblade option/filter for "Only Advanced Timers" or similar. Same as how on the Channel list ther eis a Misc options "Show hidden channels" filter option

Thanks for the screenshots. The square bracket thing is nice. I agree that we'll probably be keeping the prefix though. Is one little icon that difficult?? The same icon could also be used in the epg display to show that upcoming episode recordings are part of a series recording (like wmc does).

I think for this particular timer display, kodi is doing just what it should: show all the upcoming recordings in order, which means showing the individual recordings for an advanced timer. But yeah, if we could get another display of just the advanced timers - with no individual episodes listed - would really be great. Even better is if you can choose to list them alphabetically or by recording priority. And even better than that, make it possible to shift A-Timers up or down in priority in this view. That's a lot to ask, but to fully support advanced timers, that kind of control is needed. Obviously you could also edit or delete a-timers form this screen too.
@scarecrow420

I'll do a PR when this gets in to let all pvr addons compile again.

I agree that some points can be improved, but I have almost no time to work on this till February.
Feel free to use my code to come up with something which you think is better ;-)

PS: the screenshot posted a few post back is a bit outdated ;-)

This is how it looks at the moment:
Image
Actually, in the most ideal case, the pvr addons should handle the timer dialog filling.
Then the problem with lifetime and priority is also gone ;-)

1 drawback: translations
the pvr addons all have the full language translations updated through transifex as well so i dont know its too much of a problem. For me it's more so the complexity of the code in Kodi that would be required to support addons defining all of their own custom fields etc each with various types/values etc. Systems that are infinitiely extensible/customisable normally have very ugly looking and complex code..
As a rebuttal to @sub3 I think we should strive to keep the interface the same regardless of what backend is used. The idea is to make series recording support an integrated part of Kodi, not just change the way addons can hack it to support the options they need. Obviously we can't support every imaginable use case without making the user interface cluttered, but I think with the current set of options we should be able to cover most use cases.

I'd like to move forward with this so I suggest that we try to get Glenn's PR merged in its current state unless there are some really pressing changes that need to be made. Refinements can be done once al addons have had time to adjust and implement the new API, by then we probably have a more complete picture of what is eventually missing still.
Pages: 1 2