Changes for Matrix API changes

emveepee commented 4 years ago

Cache EPG event ID to make system usable after daily update. Lose link to recordings.

emveepee commented 4 years ago

@sub3 This is an interim fix in the hope that we can go back to using the backend event oid in Kodi. It is also WIP but I wanted to test on several platforms and offer something to users who get stuck.

I need to add the following

figure out the best location to put an iterator to erase expired keys from the map
use standard c++ or Kodi addon libraries to get the epoch time for today at midnight utc to use for the the erase. I don't really want to add the CDateTime modules just for this
clear the map on exit

emveepee commented 4 years ago

Looks good, the minor is up to you ;)

It will probably become a feature update before I'm done. I would like to pull the backend database update time if @sub3 is able to persist it between server reboots and use that to trigger EPG updates and help clean ups of most of the stale cache, time changes will linger until they age out.

One other thing will be to change the lookup to use the end date. I can't age out on start time since someone might record a 24 hour rally.

phunkyfish commented 4 years ago

Do you generally just request a review once you are ready?

emveepee commented 4 years ago

Do you generally just request a review once you are ready?

No I prefer a review as I submit but the approval may not be necessary if there are comments that indicate we go ahead. With the monolithic nature of pvr-nextpvr.cpp it is hard to create multiple PR especially when I am touching the same small groups of code for this and for the API changes. If comments come in after I request the tag that could cause extra work.

Normally I would not post WIP but this case is special because effectively the addon is broken, and I can't expect users to delete tables. The state of the world also makes it wiser to be a little more open sharing code.

phunkyfish commented 4 years ago

Cool. I can review as you submit so.

If something is WIP just say so and I can leave it till you are ready.

ksooo commented 4 years ago

This is, like you said, a workaround. If Kodi or the addon gets restarted (due to addon update, disable/enable, uninstall/install), you might run into the "freezing" problem again.

If you look for a far more simple workaround, with does not suffer from the restart problem, just calculate the broadcast uid in the addon based on event's start date and time. Those UIDs only need to be unique within a channel, thus this simple algorithm works quite well. Only drawback is that if the start time of an event changes, its UID will change, but this is a rather rare edge case.

emveepee commented 4 years ago

If you look for a far more simple workaround, with does not suffer from the restart problem, just calculate the broadcast uid in the addon based on event's start date and time. Those UIDs only need to be unique within a channel, thus this simple algorithm works quite well. Only drawback is that if the start time of an event changes, its UID will change, but this is a rather rare edge case.

That is what this does I just send the start time (will be end time) but I need to store the time:channel id as the key to the internal lookup to get the backend event id which I need to pass to the backend for EPG based recordings.

Where this method fails is as noted there is no why to tag a recording to and event since the time is not unique.

This is, like you said, a workaround. If Kodi or the addon gets restarted (due to addon update, disable/enable, uninstall/install), you might run into the "freezing" problem again.

Once the update is in place the date is in the iBroadcastUid column so it will just be the first Matrix to Matrix update that I am worried about and in pre alpha no big deal. Worse case that I see is they corrupt the database not want to use two slow updates and they have to delete epg#.db

phunkyfish commented 4 years ago

I have a question. Why not simply just set the broadcast id to the start or end time and leave the map out?

sub3 commented 4 years ago

I have a question. Why not simply just set the broadcast id to the start or end time and leave the map out?

If he needs to call a NextPVR to schedule a recording etc, he has to pass it the event id. (so he has to have a way to get back from the channel:start-time to event id)

emveepee commented 4 years ago

I have a question. Why not simply just set the broadcast id to the start or end time and leave the map out?

I do, it is an int not string. I still need the real backend UID to do an EPG based recording.

phunkyfish commented 4 years ago

Ok, and maybe a silly question but why not use start or end time on the backend for the EPG IDs since they are generated daily?

(apologies it I have this incorrect)

emveepee commented 4 years ago

Ok, and maybe a silly question but why not use start or end time on the backend for the EPG IDs since they are generated daily?

It would still need to be a key of time:channel and some coding on the backend which is not open source. I also work on the Emby interface and it needs a unique key so it just moves the issue for me. Also I think Kodi wants a unique key for iEpgEventId in PVR_RECORDINGS since iChannelUid is optional

A new API call could help but it would mean two methods in the addon code We support two backend versions and while v5 is much better than v4 it is missing cam support among other things so we can't force an update.

The issue ultimately will be fixing the reason for the workaround, the slow REPLACE INTO. Even with this change a typical daily update is still noticeably slower than zapping the table first

phunkyfish commented 4 years ago

No, I don't think Kodi needs a unique iEPGEventId for recordings (only unique per channel), but I'd say if it has that and the channel it can display the EPG info for the recording which can be more rich.

REPLACE_INTO is just INSERT or UPDATE right? I think if you split that into an insert and an update query it will take the same amount of time surely. I'm not sure this would be the culprit.

emveepee commented 4 years ago

No, I don't think Kodi needs a unique iEPGEventId for recordings (only unique per channel), but I'd say if it has that and the channel it can display the EPG info for the recording which can be more rich.

Your right https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/recordings/PVRRecording.cpp#L480 but it doesn't handle the situation where the same show is being recording twice using different rules. The code seems to already match on date and time anyway.

Thinking about it two recordings from the same show would break whether it was start time or broadcast id, so I can update the PR to use the time just to speed it up.

REPLACE_INTO is just INSERT or UPDATE right? I think if you split that into an insert and an update query it will take the same amount of time surely. I'm not sure this would be the culprit.

That is for the issue thread but everything I have read says REPLACE INTO is slow and not indexed. You could try to see the speed on one channel with sqlite

.open epg13.db
update epgtags
set  iBroadcastUid = random()
where idEpg = 1;

Check the time for Kodi start and exit and check the time for the first channel. Then run the same script go in clear the PVR database and exit and check the same time. If you think it is a cache issue exit Kodi, run delete epgtags and start and exit Kodi.

sub3 commented 4 years ago

Ok, and maybe a silly question but why not use start or end time on the backend for the EPG IDs since they are generated daily?

(apologies it I have this incorrect)

Because NextPVR is a large and complicated application, with large existing api, already built around the existing id system, which has worked perfectly fine for the past 12 or so years. Itdoesn't make sense to change this in the back for just Kodi. It should probably be implemented in the addon.

phunkyfish commented 4 years ago

Ok, and maybe a silly question but why not use start or end time on the backend for the EPG IDs since they are generated daily? (apologies it I have this incorrect)

Because NextPVR is a large and complicated application, with large existing api, already built around the existing id system, which has worked perfectly fine for the past 12 or so years. I don't understand why I would need to change this for just Kodi.

You don't need to change it at all. @emveepee mentioned in a previous post that he was going to request some changes.

phunkyfish commented 4 years ago

Thanks for walking me through the PR ;)

emveepee commented 4 years ago

Thanks for walking me through the PR ;)

NP. Related to the PR, when I update it to make the changes and support PR17515 is it better to push a new commit, amend the previous one, or amend the changes and also push a new commit for PR17515?

phunkyfish commented 4 years ago

As they are not related they should be separate commits. But I don’t yet know when #17515 will get merged as it’s a breaking change so maybe don’t wait for it.

emveepee commented 4 years ago

@sub3 I ran into another slowdown when I enabled EPG artwork for testing since the artwork URLs needs the EPG event oid, and there is no callback to modify the URL before passing it to NextPVR. Users can either have speed or disable EPG art for now

Also if any other authors follow this if you want to use the Recording time to link to the EPG time note that only works in NextPVR for pending and in-progress recordings. Once a recording starts the start time is clock time which could include pre-padding or a late start. When the recording is stopped the duration is actual time so that can also including post padding or an early stop. This is another reason to use end time for any key. For ready recordings the time will almost always be wrong.

AlwinEsch commented 4 years ago

About them it need a rebase related to version increase, this should then become 4.3.5. Was a bit fast on my request, Sorry.

emveepee commented 4 years ago

@sub3 the second commit passed my tests on v5 with Live, New Premiere and Finale you might want to review the conditions for the significance field since I am not sure how you populate non SD data. Also there will be no historical date for significance on recordings but the mapping between the timer and the EPG is working well and New and Live flags do show on pending and in-progress recordings.

@phunkyfish Great job. The first update did slow down the EPG update too because of 100's of changes from IsNew. The only minor comment I have is for me Premiere looks out of place with the choice to go with the accent.

phunkyfish commented 4 years ago

You also have New code in the first commit. Was that intentional?

emveepee commented 4 years ago

How come no check for Live and New in UpdatePvrRecording? Just curious

Several reasons

New is not in the same field and I didn't ask sub to provide it on recording.
I think Live and New look wrong on old recordings. Does it actually make a difference if a recorded show was recorded live or new. In resolving conflicts NextPVR even when it is not new
The timer EPG links provide this information on pending and in progress recordings where it does matter.
Additional the logic is not the same for recordings I show the Premiere and Recording for reruns because @sub3 asked me to do it. That makes sense because there is no S/E order in Kodi and date doesn't always match for shows that are new. Flagging these on the would be confusing.

emveepee commented 4 years ago

You also have New code in the first commit. Was that intentional?

Yes I changed what you suggested, and I mentioned I was going to change to end date and I released I was using the fake iBroadcastUid for a call to the backend

phunkyfish commented 4 years ago

Apologies. I wasn’t clear. I mean the m_showNew check. Is that supposed to be in the first commit or the second one?

emveepee commented 4 years ago

Apologies. I wasn’t clear. I mean the m_showNew check. Is that supposed to be in the first commit or the second one?

Ah right lots of changes recently. Sub3 and I discussed this offline and he explained that when the guide data isn't clear on whether a show is new or not, he will mark it new. There is a server side which is read that controls whether not to show all shows are new or not.

phunkyfish commented 4 years ago

Ah right lots of changes recently. Sub3 and I discussed this offline and he explained that when the guide data isn't clear on whether a show is new or not, he will mark it new. There is a server side which is read that controls whether not to show all shows are new or not.

Ok, cool, makes sense. It still sounds like it belongs in the second commit though right?

emveepee commented 4 years ago

Ok, cool, makes sense. It still sounds like it belongs in the second commit though right?

If you mean from a review perspective yes but in reality I guess there should have been a third commit because I needed it whether or not the commit for PR17515 was released. It's tough when one small function has three enhancements coming in different directions. When I look at the code the changes I made for the second commit were crap.

kodi-pvr / pvr.nextpvr

Changes for Matrix API changes #115