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

series recordings support #38

Merged
merged 1 commit into from Jul 3, 2015

Conversation

Projects
None yet
8 participants
@ksooo
Member

ksooo commented Jun 9, 2015

Full implementation of PVR Addon API 1.9.7 (autorec, timerec, dynamic timer attributes, ...)

For details, refer to xbmc/xbmc#7079

@Jalle19 mind taking a look?

@ksooo ksooo added the Feature label Jun 9, 2015

@ksooo

This comment has been minimized.

Member

ksooo commented Jun 11, 2015

ofc this one is NOT for Isengard, as the new API will be first introduced with J****.

@ksooo ksooo referenced this pull request Jun 11, 2015

Merged

[PVR] Series recordings #7079

@ksooo

This comment has been minimized.

Member

ksooo commented Jun 14, 2015

Rebased.

@r0m30

This comment has been minimized.

r0m30 commented Jun 16, 2015

Are there any other prereqs besides 7079? Applying this and 7079 to master gives me the new UI features but adding a new custom timer and playback seek seem to be broken. The scheduling using the "any time" radio button returns with a start time of 0800. Seeking jumps forward and backwards in (seemingly) unpredictable amounts and will sometimes loop back to a prior point in the recording while seeking forward. The same recording plays fine when played with VLC on a win7 machine.

I forked and pushed my changes to r0m30/OpenELEC.tv@ff3c9ca87ee71b2b2c79950c239382492b39cc93 if you want to verify that I didn't do something foolish.

Testing on a RPi2 if that makes a difference.

@ksooo

This comment has been minimized.

Member

ksooo commented Jun 16, 2015

@r0m30 thanks for the feedback. There are no other prereqs for this PR than 7079 and a recent tvheadend server (4.0.0+ should be fine). What version of tvheadend are you using?

I'm pretty sure the seeking problems you mention are not related to my changes. Please doublecheck that they do not occur without 7079 and this PR.

What exactly do you mean with "adding a new custom timer is broken"? Please give me step-by-step instructions how to reproduce.

@r0m30

This comment has been minimized.

r0m30 commented Jun 16, 2015

@ksooo Thank you for the hard work, This is a huge improvement in the PVR functionality.

The seeking problems do not occur on the 5.95.2 build (and the 5.95.2 rev history shows a merge from master then a config update to "official" as the only difference) but I will build locally to make sure it's not an issue in my build environment.

I'll re-update (had to fall back as recordings were unwatchable) and give you a step-by-step of the timer issue.

@r0m30

This comment has been minimized.

r0m30 commented Jun 16, 2015

PVR.HTS 2.2.0
TVHeadend 6.0.1 (TVHeadend 4.0.3)

Custom Timer issue:
Homescreen->TV->Guide
--context select the show you want to series record
--select "Add custom Timer"
in Dialog box "Timer Settings"
----Active -> SELECTED
----Name -> Show name from EPG
----Search Guide for -> Show name from EPG
----Fulltext Search -> UNSELECTED
----Channel-> channel from EPG
----Days of Week -> Any Day
----Any Time -> SELECTED
----Prevent Duplicate Episodes -> Record all episodes
----Start Padding -> 1min
----Stop Padding -> 1min
----Priority -> Normal
----Lifetime 365Days
----Folder -> BLANK
===> press OK <====
<<<<<<< NO SHOWS ARE SCHEDULED >>>>>>>
GO back to homescreen
TV->Timers
Locate the timer for the show you just created.
The "Scheduled Time" column says:
Any Day from 0800 to any time <<<<<< expecting any time to any time
Context select the timer
Select edit timer
Differences from entries as entered above are:
----Any Time -> NOT SELECTED
----Start Time 8:00
----End Time Any Time
Maneuver to ANY TIME and SELECT IT the Start and end Times fields disappear as expected
Select OK
Locate the timer in the list and it is unchanged. ie The "Scheduled Time" column says:Any Day from 0800 to any time

If I go to the WEB UI for TVH I see this
untitled

Notice the Start Before 22:45 and Start After 23:15 (the actual start time for the show selected was 01:30 and the end time was 02:00.

If I change the Start Before and Start After to any in the WEB UI the show is scheduled and records as expected. and displays in the Kodi TV->Timers as Any Day from any time to any time

I hope this is clear enough, GUI interactions are always difficult to represent in text.

The rebuild will take several hours so I'll have an a/b test on the seeking issue sometime tomorrow.

@ksooo

This comment has been minimized.

Member

ksooo commented Jun 16, 2015

@r0m30 what's your timezone?

@r0m30

This comment has been minimized.

r0m30 commented Jun 16, 2015

America/Los Angeles
-0800


Mike
On Jun 16, 2015 5:52 AM, "Kai Sommerfeld" notifications@github.com wrote:

@r0m30 https://github.com/r0m30 what's your timezone?


Reply to this email directly or view it on GitHub
#38 (comment).

@ksooo

This comment has been minimized.

Member

ksooo commented Jun 16, 2015

@r0m30 Really very strange. Series recordings with "any time" work like a charm here (OpenELEC master, Linux x86_64) - even if I switch my timezone from Germany/Berlin +0100 to America/Los Angeles. I'm afraid we're running in some (Pi specific?) timezone problems here.

Anyway, I found one suspicious piece of code in 7079 which I have fixed now. Doesn't make any difference for me, but could be for you. Would you mind to re-pull (caution, I force-pushed) 7079 and report back?

@r0m30

This comment has been minimized.

r0m30 commented Jun 17, 2015

@ksooo Wow, that's some impressive static code analysis!
Works exactly like I would expect it to.
Many thanks.

The skipping issue may be related to error recovery when streaming recordings of weak signals, I live in a fringe area and during the summer the symbol rate drops because of the hot cloudless days, so I think you are right that this change isn't related. It could also be xmbc/xmbc@04d3ba9f20c97dcf918df3f0cc764b8f5e24406a which was just committed a few hours ago.

@ksooo

This comment has been minimized.

Member

ksooo commented Jun 17, 2015

@r0m30 Thanks for testing. I'm happy that it works now.

ksooo added a commit that referenced this pull request Jul 3, 2015

@ksooo ksooo merged commit 108b15a into kodi-pvr:master Jul 3, 2015

@Memphiz

This comment has been minimized.

Memphiz commented on src/Tvheadend.cpp in 045bd71 Jul 21, 2015

This crashes on osx ... are you sure that a struct has a this pointer and that it is valid at this stage?

This comment has been minimized.

Memphiz replied Jul 21, 2015

http://pastebin.com/sasa44xg

search for thread #47

This comment has been minimized.

Memphiz replied Jul 21, 2015

Another addition - this whole struct or its instantiation causes a crash on osx - even with memset removed. See:

http://pastebin.com/cS8MTCFW

This comment has been minimized.

Member

ksooo replied Jul 21, 2015

which kodi version? latest master?

This comment has been minimized.

Member

ksooo replied Jul 21, 2015

Just removing the memset is not a good idea. The ctor does only init some of the struct members if you remove the memset! Does it work if you remove the memset and initialize all members of the struct manually?

struct decl is here: https://github.com/xbmc/xbmc/blob/master/xbmc/addons/include/xbmc_pvr_types.h#L327

This comment has been minimized.

Member

ksooo replied Jul 21, 2015

Okay. Thanks for looking into this. I will come up with a proper PR tomorrow, then.

This comment has been minimized.

Member

ksooo replied Jul 21, 2015

@Memphiz ... and welcome to Lübeck, then. As I live in Hamburg we should have some beers, then. ;-) 🍻

This comment has been minimized.

Memphiz replied Jul 21, 2015

Not sure why this is a problem but it seems like it is. ulimit tells me a stacksize of

stack size (kbytes, -s) 8192

which means the stacksize per process - not sure if this is in general or each method has 8 MB...

This comment has been minimized.

Memphiz replied Jul 21, 2015

Hamburg -> YAY! sounds good :)

This comment has been minimized.

Contributor

Jalle19 replied Jul 22, 2015

@Memphiz the stack size is per thread

@dreamcat4

This comment has been minimized.

dreamcat4 commented Jul 23, 2015

OK. I have just tried this on windows master nighlies. Needs kodi-pvr-hts addon v2.2.3 or higher. Seems to work as advertised here. Should be the same for mac os x too.

Unfortunately the linux addon binary didn't quite make it, having been updated slightly too early to catch it. And remains on v2.2.2 for the time being. I've asked the build manager to updated the launchpad ppa which build ubuntu .debs. If he can get around to it.

@ksooo ksooo deleted the ksooo:series-recording-support branch Jul 23, 2015

@PhobosK

This comment has been minimized.

PhobosK commented on src/AutoRecordings.cpp in 045bd71 Aug 5, 2015

I do not know why you have decided to do the time periods in this exact way (probably to support HTSP versions < 18), but this totally breaks the TvHeadend logic, usage and API in versions >18...
Now you have separate:
minDuration
maxDuration
start
startWindow (Added in version 18)

Reference: https://tvheadend.org/projects/tvheadend/wiki/Htsp/#addAutorecEntry-Added-in-version-13

So for TvHeadend 4.1.18 (HTSP v22) this isn't working the right way... I'm attaching screenshots showing it...

This is what I set on a new tab with "Add new timer" in Kodi:

hts-pvr

This is what actually is set at the TvHeadend side:

hts-pvr2

And this is what the addon gets back in Kodi when updated from TvHeadend:

hts-pvr3

So the final result is that:

  1. in Kodi the start/stop time gets messed up with minus one hour (this - 1h probably has to do with the GMT conversion)
  2. in TvHeadend you get a start/stop +/- 15 min from the originally used start time in the Kodi settings (- 1h really but I think this is a separate bug having something in common with the GMT conversion) + some min/max duration conditions.... I.e. you get a starting using the approxTime message (a couple of code lines above... which is obsoleted from version 18 btw)
  3. You don't get any recordings scheduled since that is done at TvHeadend's end and the conditions there do not meet what the user expects.....

So I see 3 problems here:

  1. Miscalculation of times with 1 hour behind the reality (probably DTS?)
  2. Improper handling of users expectations about start and stop time (probably shown as behaviour in >18 versions but v 22 is the only one I can test with)
  3. Missing minDuration and maxDuration in GUI that can benefit the user...

So should I file a bug report for all these?
Thanks

This comment has been minimized.

Member

ksooo replied Aug 5, 2015

The one hour off is a bug and needs to be investigated. Maybe there this something wrong with the time conversion the addon does or your system is misconfigured. We'll see.

Min/ Max duration display seems to be broken in tvh 4.1. Works for me in tvh 4.0.5. There, the durations are not shown as plain numbers, like in your screenshot, but as strings like "45 mins". I guess there got something broken in tvh with the localization stuff recently introduced (and not in 4.0.x)

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 5, 2015

@PhobosK Can you please test with tvh 4.0.5 and report back? tvh 4.1 is unstable and is currently not my reference.

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 5, 2015

Regarding support for min max duration in Kodi UI: Yes, as these are not supported by Kodi UI I have to find some mapping from start and end time to the values tvh provides/expects. As with many mappings, this is not perfect, yes, but despite from the 1h off which is a different story I cannot see any real bug (miscalculation) here.

Regarding "users expectations": That's your very personal opinion, not mine. tvh users, knowing the tvh web UI, might be disappointed by the Kodi UI. Average Kodi end user, not even knowing the tvh web UI, might be very happy with the imo simple and clean approach taken by the Kodi UI, which is consistent across different backends btw.

The idea of the Kodi UI is to provide a feature set for average users. For special features, please use the tvh web UI.

But as this is open source, feel free to file a PR containing the implementation for min max duration and whatever else you might be missing at the moment. ;-)

@Jalle19

This comment has been minimized.

Contributor

Jalle19 commented Aug 5, 2015

@metaron-uk has a fix for time conversion in one of his PRs, maybe it fixes this one hour off bug?

@metaron-uk

This comment has been minimized.

Contributor

metaron-uk commented Aug 5, 2015

@PhobosK

This comment has been minimized.

PhobosK commented Aug 5, 2015

Hi,
I am sorry if I sounded a kinda offending... That was not my intention at all.... So I apologize if you felt offended.
You do a great job here and as a long time contributor to open source myself, I know how time consuming and full of efforts this is.... But one of the important contributions is finding and pinpointing bugs in the applications... and this is what I'm trying to do in this case ... To cut the long story short here is what I find erroneous (more like illogical from user's point of view) in this part of the code of pvr.hts:

  1. As far as I followed the code in SendAutorecAdd the following HTSP messages are sent to the server:

    "title"
    "fulltext"
    "startExtra"
    "stopExtra"
    "retention"
    "daysOfWeek"
    "dupDetect"
    "priority"
    "enabled"
    "directory"
    "channelId"
    "approxTime"
    "minDuration"
    "maxDuration"

    • The "approxTime" is already deprecated as of v18 of htsp... In this particular case it tells TVH to seek for a show to record around the time it sends +/- 15 min, so if set, this will record only if the show starts in this specific 30 min interval and autorec will fail otherwise...
    • The calculations of "minDuration" and "maxDuration" driven from timer.endTime and timer.startTime in some cases would be speculative... It is assumed that the user should know the exact start and end time of the show, which is not always possible... If the user schedules the start earlier than 15 minutes - the "approxTime" will fail, and if the user schedules the end later than 15 min after the real end, the "minDuration" will grow above the show length and the autorec will fail...
    • Here you do not ever use the "start" and "startWindow" messages that are the actual ones that are in Kodi's UI (start and stop). Is this because of Kodi's limitation? BTW the UI is perfect - simple and understandable. So what I think is that when the user selects in the UI a start time, it should be mapped to "start" htsp message.... When the user selects an end time, it should be mapped to "startWindow" ... Besides even if the user sets the start time earlier or end time later... the autorec will not fail... Far more flexible

I'll give you а couple of test case examples to be even more clear in what I am suggesting. I hope you do not find them rare...:

  1. Let's say we have a weekday show that starts at 10.00 and ends at 11.00, but Wednesday and Friday it is from 14.00 to 15.00. The user may decide to put all at once (for ease) and the simpliest way to do it would be to set all days of week, start time at 10.00 and end time at 15.00 in Kodi.
    • The pvr.hts version now will fail: at Wednesday/Friday because of the "approxTime" and at all other days because of the large minDuration.
    • The proposed change (using mapped from the UI "start" and "startWindow"" messages) will not fail
  2. Let's say soon an everyday show will start, but the user doesn't know the exact timings, he knows it is somewhere between 10.00 and 12.00. He is lazy to look for it in the EPG, so he will probably set a start time in the UI 10.00 and end time 12.00 - this will fail the autorec if the actual show is starting after 10.15 or if it starts at 10.00 but is only 45 mins long
  3. A show is running everyday 3 times a day(morning, afternoon and evening), 2 of which are repetitions of the first. All 3 have same title, description and length (obviously). But everyday the show is different in length. So the user wants to record everyday the second time (the one in the afternoon), because in the other timings, there are other shows he records. He doesn't have a way to do it right now because start/stop time do not work as described above, but he will have a way if "start" and "startWindow" are used instead.

So the conclusions:

  • the way the autorec now is done in pvr.hts is ok (using the approxTime and minDuration/maxDuration calculations), but it requires the user to know the exact timings + they don't cover all the possible ways the user acts, understands and uses the UI "Start time"/"End time" + approxTime soon will be ditched away

  • IMHO a better way of handling the situation will be to use the same UI "Start time"/"End time" and just map them to "start" and "startWindow" (if Kodi allows that of course)

    As for the one hour difference yes @metaron-uk 's patch works, but only for autorecs set by Kodi... Those by the TVH web UI are mixed up... I guess it is because of the calculations the addon does...

    Sorry for the long answer and thanks for the understanding

@metaron-uk

This comment has been minimized.

Contributor

metaron-uk commented Aug 5, 2015

@PhobosK: In PVR API 3.0.0 (see kodi PR 7626, assuming it gets merged as it currently is) there will be separate start and end 'anytime' controls and you can choose if the start and end clocks / anytime options appear or not individually. My proposal to @ksooo was that he:
a. remove the 'end time' control from the display,
b. use 'Start anytime' to decide if approxTime is set, and
c. not to bother setting max/minDuration at all.

min/maxDuration

  1. If a show repeats several times during a day (say the news) then you can set 'Start anytime' to record them all (I think this is what would happen) or use the approxTime method to only record the 10pm news if you wanted.
  2. If I understand the 'startWindow' function you describe correctly (I've not looked at the code), you could keep the End time clock and use it instead of driving min/maxDuration (which IMO doesn't work properly) to drive the closing value for 'startWindow'.

This needs some thought and a proper prototype creating/testing IMO.

My impression is that Ksooo is open to good ideas, but hasn't got much time at the moment. I'd suggest that once API3.0.0 is merged (hopefully soon) you knock something up and submit a PR which implements your proposal and fixes the remaining time display bugs.

Be patient while he reviews it. Expect to have to get your whitespace correct and use c++ rather than c style initializations before it gets merged ;-)

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 5, 2015

Here you do not ever use the "start" and "startWindow" messages that are the actual ones that are in Kodi's UI (start and stop).

"startWindow" != "stop": "start" + "startWindow" both together form the successor of "approxTime" to overcome the tvh-hardcoded +-15 mins start window. At least this is how I understood it.

EDIT: I have to apologize here. My interpretation from the code was obviously wrong as the htsp spec clearly states

start              s32   required   Exact start time (minutes from midnight) (Added in version 18).
startWindow        s32   required   Exact stop time (minutes from midnight) (Added in version 18).

Thus, your suggestions make perfect sense. Will change this.

@PhobosK

This comment has been minimized.

PhobosK commented Aug 5, 2015

Thanks very much.. I think it will fix the timings problems seen now too

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 5, 2015

@PhobosK maybe you want to give this (https://github.com/ksooo/pvr.hts/commits/fix-autorec-start-stop-handling) a try and report back. Beware, it does compile but I have not tested this myself yet.

@PhobosK

This comment has been minimized.

PhobosK commented Aug 5, 2015

It does compile and it works perfectly well.
Timings now are not messed up at all and are equal in both Kodi and TVH.
No matter where one sets the timings (inside Kodi or inside TVH) they sync perfectly well and the needed shows are added by autorec properly.
I've tested with every possible scenario and they all work as intended by TVH.

I should add that this all is with: Kodi git master + TvHeadend 4.1.18 + the time patch from metaron-uk/pvr.hts@ea8ba20 .... And yes the TVH server and Kodi are in the same time zone... Sadly I cannot test case with a server in another TZ, nor I can afford altering the TZ of any of the PC's

Great job thanks... :)

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 5, 2015

@PhobosK You're welcome. Thanks for testing and flying with pvr.hts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment