kodi-pvr / pvr.nextpvr

Kodi's NextPVR client addon
GNU General Public License v2.0
22 stars 33 forks source link

EPG dangling pointers #114

Closed phunkyfish closed 4 years ago

phunkyfish commented 4 years ago

Discovered an issue while doing the PVR API update. Certain addons don't store their EPG. They simply read it from the backend temporarily to pass it to Kodi.

HDHomerun, WMC, Enigam2/vuplus and nextpvr all have this issue, The instance the data is passed to Kodi, the pointers are invalid and Kodi will try to persist all the data on every exit.

Which I think is the problem nextpvr has currently. I wonder can we implement a single solution and use the same (wish) solution for each of the addons. I believe WMC/hdhomerun don't have maintainers so something tried and trusted would be good for them.

phunkyfish commented 4 years ago

@ksooo @emveepee ^^^

emveepee commented 4 years ago

@phunkyfish is this related to this issue https://github.com/xbmc/xbmc/issues/17390 On that I found that is not really each call, it is each NextPVR backend EPG update invalidates the iBroadcastUid. After each nightly update it is impossibly slow because it is the REPLACE INTO which is slow likely because it is not an indexed call. Once the daily iBroadcastUid are updated new calls are not slow on subsequent runs because there are no updates. As pointed out in that thread even a regular update ignoring the iBroadcastUid 20 replaces can be slow.

On that issue I did note there are never deletes. Perhaps that leads to a dangling pointer issue.

For development I have started to simply "delete from epgtags" after each update which gets around this problem I am pretty sure that one of the reasons @sub3 choose to zap the backend EPG table was performance related.

I was also working on two performance changes . The first PR and optional API change I was working on prior to this was for the PVR to request the backend database update time so that channels would only be updated if sLastScan < update time. Related to this would be trusting no data from the backend perhaps with a new return. It is not clear if deleting all the tags could be linked in with this.

This could still be problem with NextPVR users who do a lot of OTA incremental updates over the day, I don't know how @sub3 handles these changes for the entire database.

Without this API another performance enhancement I was planning was to fake EPG entries for no data channels to avoid the unnecessary calls to the backend. I find getting users to be aware or change the value in isn't easy and with IPTV we are seeing 1000's of channels with no EPG data. It gives the advantage of being able to schedule a manual recording from the guide with some additional tweak. @ksooo told me not to work on that one

phunkyfish commented 4 years ago

So when you create an EPG tag you set all the c_str() from the strings on the EPG_TAG. I think how it works is the EPG keeps this EPG_TAG and checks for changes. But as they are not backed by real strings the next time the addon pushes the data kodi thinks it needs to persist all the data due to the new pointers.

I'm going to add a backing store to vuplus and see does it solve this problem.

emveepee commented 4 years ago

For testing I removed this line https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/epg/EpgInfoTag.cpp#L496 for the delta and speed was improved of course any epg links are broken.

phunkyfish commented 4 years ago

So do you generate a broadcast ID each time or is it from the backend?

emveepee commented 4 years ago

Backend after an EPG update

phunkyfish commented 4 years ago

So then why is it always different?

emveepee commented 4 years ago

The main difference is the daily update when the ID changes but while monitoring this a few shows change details. After that initial update changes are because of the sliding window into the EPG. I have 3 days of 15 available but those aren't really days, they are 72 hours. So for each period of time you have Kodi running new shows will get added to the tail.

sub3 commented 4 years ago

So then why is it always different?

The EPG data in NextPVR can come from several sources, and I will usually not have delta information available to tell me what has changed since last time the EPG data was read. Also any attributes of existing listings may have changed (like start/end time, descriptions etc). Given a user can have hundreds of thousands of listings, and it's time consuming to compare the existing listings with the new listings...NextPVR instead takes the strategy of blowing away the existing listings for a channel, and inserting the new ones, then updating the recording schedule. Its much quicker and more efficient. This does mean IDs for shows etc change each time the EPG is updated though, but this is the fine since it's the way NextPVR is designed to work.

phunkyfish commented 4 years ago

But this means that logic would then need to exist in the addon instead right?

ksooo commented 4 years ago

This does mean IDs for shows etc change each time the EPG is updated though, but this is the fine since it's the way NextPVR is designed to work.

And this causes the "incredible long Kodi exit times". If the broadcast UID changes, the tag is detected by Kodi as changed (technically this is equal to for example changed start/end time of the tag) and will be rewritten to database. If all EPG tags are detected as changed, this can take very long, yes. If the addon would report only actual tag changes, only the very first storage of tags (fresh Kodi install with no database or after an epg database reset) would be slow. This is, how it works for example for pvr.hts (tvheadend).

The dangling pointer problem is caused by something completely different (char* pointing to random already freed memory content, thus producing always different "strings" for the same tag), but has exactly the same effect as a changed broadcast UID. Tags which are actually not changed are detected as changed by Kodi and will needlessly be written to disk.

BTW: The slow exit problem is not new for Kodi 19. The difference is that usage of the EPG database was optional in prior Kodi versions. With EPG database being mandatory for 19, the problem becomes more visible. Actually it was even worse with former Kodi versions, because there all EPG tags were persisted to database again and again on each epf update, regardless the tags were actually changed or not.

I'm pretty sure that some database optimization guru (which I'm unfortunately not) can fix the performance problems we currently see in kodi master.

emveepee commented 4 years ago

Thanks @ksooo I will close the discussion on the UID here since there is a Matrix issue thread for the slowdown. It is not just slow it is unusable without doing a database reset. I am surprised that tvheadend isn't slow with fixed time daily updates.

@phunkyfish can you describe the dangling pointer issue and if there is something that we need to do to needs to do to solve it. Managing internal tables to use one UID won't work since those tables would not persist a restart.

ksooo commented 4 years ago

Tvheadend sends EPG tags containing only changes when something really has changed. Uid does never change for example.

And I'm using SSDs.

For me, with an SSD and tvheadend, persisting 150 channels with 15 to 30 days of EPG on exit does not last longer than a minute!

Finally, Kodi 19 is pre alpha. So, everybody is invited to help nailing down the issue and to help fixing it.

emveepee commented 4 years ago

I definitely will continue to work on it. You might actually find deleting your epgtags table would cut that minute too.

phunkyfish commented 4 years ago

Let me create a PR for vuplus so you can see what I will do there.

The main difference will be the vuplus backend will give the same UID whereas it won’t for you. But you could still use the same method as it will be based on start time. It will force an update on restarts.

emveepee commented 4 years ago

Thinking about it investigating the slowdown issues a couple of days ago when I removed the UID check I stepped through https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/epg/EpgInfoTag.cpp#L475 and was able to confirm that every mismatch was valid. If the problem is what I suspect I think only shows with an empty title might fail

phunkyfish commented 4 years ago

Have a look here: https://github.com/kodi-pvr/pvr.vuplus/pull/276/commits/3350ca8a70c0b81b235034435ca34311bcad16bb

If you do something similar for next pvr (but omit the epgId in the equality test) you would only send events that have changed). Yes, you would need to send again on a restart but it should be way more performant.

Vuplus performance issues have disappeared after this change.

emveepee commented 4 years ago

Are your suggesting storing the whole EPG in memory? Wasn't the reason @ksooo started using the database was to avoid excess memory use? There are people with hundreds of thousands of rows and that is with cable before IPTV If I don't pass the UID to Kodi, I'd have to keep a lookup table of the UID deltas and that will be be tough and mean we use more memory then we did before.

Besides my performance issues already disappear when I don't update the UID so I am not sure I have pointer any issue. EPG13.db looks clean to me, what should I look for to see what the pointer issue is.

If the issue isn't fixed on restart then it won't be acceptable because in the other post I indicated that I have no UI control for the small incremental updates that take place and this is minutes not seconds

Finally there sounds like there might be something else at play here. If the EPG call is only looking at deltas why isn't that documented. I think the delete is maybe missing if this working.

phunkyfish commented 4 years ago

Memory is cheap these days. I wouldn't have that be a driving consideration.

But bear with this as I think we have a nice clean solution here with a few mods. Your main issue is you need a broadcastId, in this implementation everything is indexed by start time and it is unique per channel. Therefore it should fix all your issues if I'm not mistaken. You just have to make sure you populate it when reading the new entry.

You now have no issues on restart either.

Let me try a clean impl on HDHomerun and see if this works.

phunkyfish commented 4 years ago

Here is a cleaner impl on hdhomerun: https://github.com/kodi-pvr/pvr.hdhomerun/pull/92

Bear in mind it has not be run time tested yet.

emveepee commented 4 years ago

Ok I will wait much longer for that until I better understand why this is a requirement. I might be causing more problems for myself by killing the task and not letting the updates finish as there are rows going missing, and the iBroadcastUid blanked out but not 0 so

I will start to update the UID via a script and test real deltas because having the database locked for an hour is not helping testing.

One thing I have noticed is prior data which is not provided by the backend is corrupt too and it really not clear how this change would fix that

phunkyfish commented 4 years ago

Why not try it from it from a clean DB and see if it's any better? It may save you a lot of headaches and it's not much effort to implement.

emveepee commented 4 years ago

With a clean database I have zero issues and I am not seeing any issue other than slowdown with the uid. Until the requirement changes to force a persistent uid I'd rather understand what the issue is.
This is pre alpha no need to rush.

ksooo commented 4 years ago

@phunkyfish are you sure pvr.nextpvr actually has the dangling pointer issue? I don't think so.

Looking at https://github.com/kodi-pvr/pvr.nextpvr/blob/Matrix/src/pvrclient-nextpvr.cpp#L594 ff., I see that an EPG_TAG named 'broadcast' is filled with char coming from local stack variables, yes. But before those locals are destroyed (which would lead to dangling pointers), PVR->TransferEpgEntry(handle, &broadcast); gets called which hands over the EPG_TAG synchronously* to Kodi. All fine, if you ask me.

phunkyfish commented 4 years ago

Understood. But by happenstance this does solve the UID problem albeit by requiring storage of all the EPG data. This could be changed to just store a hash of the data and would still work.

ksooo commented 4 years ago

Understood. But by happenstance this does solve the UID problem albeit by requiring storage of the all EPG data.

That might well be the case. But this issue is "blaming" pvr.nextpvr to suffer from a dangling pointer bug which is not the case. I suggest to close this issue and to open a new issue to discuss the changing UID problem.

phunkyfish commented 4 years ago

Fair

emveepee commented 4 years ago

I think there is one issue with the title because I don't currently clear it and I need to fix that, although guide data without a title is rare, it was worth a review.

ksooo commented 4 years ago

That's also not a dangling pointer issue. 😉