[epg] Sync epg update from client / db on startup

[ghost]

On Kodi startup two threads (1: client -> db, 2: db -> EpgContainer) accessing the same database. 1 is checking for new epg entries, 2 is retrieving valid epg's -> crash.

This first approach locks the dbaccess in PersistXXX functions, wich runs but could lead to freezes in the gui when reading Epg's (Timeline window).
So there must be a better solution for this - this PR is just to not forget the issue, it is an major ticket as kodi don't startup anymore after epg db is filled,

Workaround: "Don't use DB for Epg (in TV::EPG settings dialog)

[ghost]

1.) My preferred solution for this is a boost::LockFreeQueue instead StringList insert_sql.
The problem we have here is a problem of non thread safe access to the StringList insert_sql (defined in dataset.h:228). This would affect a lot of files in dbwrapper

2.) Much easier is to implement is a lock on the StringList insert_sql (and the 2 others) for exclusive access:

[code]
m_critSection.lock();
std::map<unsigned int, CEpg*> copy = m_epgs;
m_critSection.unlock();
CSingleLock(database.GetQueueLock())
return m_database.Persist(copy);
[/code]

What do you prefer? 1) Max performance or 2)Less changes?

[Paxxi]

We generally try to avoid boost and minimize the usage of it so that mostly rules out option 1. I leave the rest of the discussion to the pvr people

[ghost]

@Paxxi: Do you know if there is an kodi implementation of a lockfreequeue or similar using atomic operations?
And, other question: when is the expected release date for K15? Maybe this already leads to a simple implementation...???

[popcornmix]

https://github.com/xbmc/xbmc/blob/master/xbmc/threads/LockFree.h

[fritsch]

We should also avoid to even consider using the linked code :-) See the
comments of the last commit to it.

2015-07-01 22:52 GMT+02:00 popcornmix notifications@github.com:

https://github.com/xbmc/xbmc/blob/master/xbmc/threads/LockFree.h

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

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

[Jalle19]

https://github.com/cameron314/concurrentqueue/blob/master/README.md

[ghost]

thx for the link - I'll look over it

[ghost]

I thought about this issue again.

regarding the 2 stacktraces in http://forum.kodi.tv/showthread.php?tid=229401&pid=2029642#pid2029642

we have 2 threads accessing the database:

1.)
#3 0x007b0d7a in EPG::CEpgContainer::PersistAll() ()
#4 0x007b103a in EPG::CEpgContainer::Process() ()
Which backups the EPG Information from time to time

2.)
#7 0x00948952 in PVR::CPVRManager::CreateChannelEpgs (this=0x1880768 PVR::CPVRManager::Get()::pvrManagerInstance) at PVRManager.cpp:1733
#8 0x009459cc in PVR::CPVREpgsCreateJob::DoWork (this=0xa1f12570) at PVRManager.cpp:1007
#9 0x00948500 in PVR::CPVRManager::ExecutePendingJobs (this=0x1880768 PVR::CPVRManager::Get()::pvrManagerInstance) at PVRManager.cpp:1690
#10 0x00943d72 in PVR::CPVRManager::Process (this=0x1880768 PVR::CPVRManager::Get()::pvrManagerInstance) at PVRManager.cpp:545
Wich simply adds new channels to the EPG database.

Question is now: Why do we have to access the database for the 2. Thread?
Why not simply getting epg Id from EpgContainer instead from db (wich is also optional). I think the 2nd thread should never access the db, if the channel is missing then we getting the next free if from container, isn't it like that?
What did I miss?

[Edit] I don't understand the purpose of the 2nd thread at all:
CPVRManager calls EPGContainer::PersistTables() wich inserts only the epg (not the tags) into the database ignoring the settings if db should be used at all. Could anybody explain for what htis 2nd thread is good for?

[ghost]

And one more question: If Epg database is disabled in settings, should the table epg filled for any reason (I don't mean epgtags here)? Maybe to create unique epg ids? Currently the epg table is filled using thread 2 (but not over thread 1)
From theory it could be possible to don't use the db at all (and just introduce a internal idepg for the current kodi session).

Before I change these things it would be great if somebody can tell me the wanted concept, there will be many workflows I don't have any knowledge about....

[ghost]

• PersistTables from PVR will only be triggered and later processed in EpgContainer::Process loop
• PersistTable will only insert epg into DB if EPGDB is disabled ( to don't change the behaviour)
• call EPG::Persist from EpgContainer without commiting the tags after each epg, do it after all epgs are collected in one transaction.

[Jalle19]

@mapfau the only purpose of the EPG database is the ability to scroll back in time on the timeline (since many backends don't supply historic data).

[ghost]

@Jalle19 thanks, this is one big piece of the puzzle! I thought it is because of performance options (what doesn't make sense as you need top bandwidth to view live-tv).

I don't understad at all why the PVRManager calls EPGContainer::PersistTables. I would understad it if PVR needs an unique Epg Id, but this is not archived in EpgContainer::PersistTables (no EpgIds are updated from DB).
Any idea why PVR forces EPGContainer to write the epg table do DB?

[ghost]

I have removed Epg.h / .cpp changes because they are improvements.
What is left is "posting" the PersistTables call to the EpgContainer Process thread instead accessing the db directly from other threads (like PVR)
Discussion point is, whether PersistTables should be removed completely.
It can be deleted IMO if there is no need for having epg-table filled.

[opdenkamp]

remind me of this one in the evening, then I can answer your questions

[ghost]

@opdenkamp What I figured out so far without going too much into detail is:

• PVR receives new channels
• PVR calls CEpgContainer::CreateChannelEpg for each channel and updates epg->channelId if existing or otherwise creates a new Epg using "NextEpgId" for EpgId
• After this step PVR calls PersistTables wich writes only the epg table (but not EpgInfos) to DB)
  This step does NOT adjust the Epg's epgID, it just writes / updates the Epgs to Db.

Thanks for some short background information about this step!

[opdenkamp]

meh forgot to press send yesterday evening :)

First this: the epg db is not only for the ability to scroll back, as @Jalle19 said, but also for some hardware that don't cache epg data and can only get epg data from the channel that they're currently tuned to, like pvr.njoy. Without the epg db, you would not have any epg data for a long time after a restart of Kodi.

PVR code is very much tied to EPG code still. I've separated most of it (but not all) so it can be used independently at some point, even without PVR running, through it's own API (e.g. by one of the add-ons that now do a sort of timeline in python). But since it's not been separated completely (yet), PVR still needs correct epg tables linked to channels and vice versa. Which is not done in a very nice way atm ;-)

Any idea why PVR forces EPGContainer to write the epg table do DB?

There are also some people who insist on using a shared mysql db for all of this, even though I've warned them quite often now that this is not supported by the code properly and will just cause each client to overwrite the other's data. To not cause too many issues for these (not so smart) people and not kill performance by having to check the last insert id on each query, we create all tables in the db at the start, when we can do it with the least amount of delay caused for users.

Importing and persisting all tags takes a lot more time, so we do this later, after the channels import finished.

[Jalle19]

I don't think a single minute of time should be spent making obscure and feature-incomplete addons like pvr.njoy to work, which pretty much leaves the ability to scroll back as the only reason why we have the EPG database.

[opdenkamp]

Then we think differently and the epg db is not going to be removed or gimped to just be used for scrolling back.

It's a hardware limitation for that device, not software.

[Jalle19]

I'm just saying that we have an endless stream of problems related to a feature that likely few people use (scroll back in the timeline) and even fewer people depend on (mythical pvr.njoy users), not the mention the group of people who just don't use it because their devices are too slow.

[opdenkamp]

You're just applying your own use case to the whole of PVR and want to strip everything you don't use yourself? :) I like the scroll back feature in combination with starting playback of something from the timeline that was recorded a few hours ago.

Not sure what "endless stream of problems" you're talking about, because it's fine except for a couple of minor issues. Like the one being addressed here.

There's also a big difference between what people think is a problem or slowness and what's actually happening. Like the epg import. If you remove the status progress thing for importing the epg data (not channels, just epg), then you'll see that most of the complaints will suddenly disappear.

[xhaggi]

First of all I also use the feature to scroll back in the epg timeline and everything else what @opdenkamp said. So -1 for removing the EPG database.

If you remove the status progress thing for importing the epg data (not channels, just epg), then you'll see that most of the complaints will suddenly disappear.

I would say no, because we load the EPG from db in the main/pvr thread which blocks the PVR startup. I've created a PR #6937 which changes the EPG startup to be async but never finished it. IMO we don't need the EPG data to finish the PVR startup and doing it async will speed-up things a lot.

[ghost]

@opdenkamp thanks for the backgrounds! I'll keep them in mind and consider them if I have time to go deeper in it.
The current PR is stripped down to the basics:

• Let the EpgContainer thread write the epg table instead PVR thread directly (epg process timer is checking for changes every minute)
• If database is enabled don't do PersistTables because PersistAll will do this job (and more).

The only thing wich must be evaluated is: Is the minute update time from the EpgContainer Process() loop a problem for PVR?

[ghost]

For me the solution works well - I have not recognised any problems during the last weeks and stop therefore any further development on it..

[ghost]

Any ideas how to continue with this issue? I would love to see it merged in the official kodi repo as my system is not working at all without this patch. The latest PR has only minimal impact.
Thank you!

[xhaggi]

@mapfau imo we can merge it in after you've addressed my comment.

[xhaggi]

btw please add [epg] as prefix to the commit message, thanks

[ghost]

@xhaggi Thanks for your comments. Pls look over the new commit...

[xhaggi]

looks good thanks

[xhaggi]

jenkins build this please

[ghost]

@xhaggi , I have an OT question. I pulled the latest kodi commits from master branch wich requires pvr api version 2.0.1. I'm unable to make the pvr.hts client as it comes always with api version 2.0.0. Is there any trick?

Sorry, my fault - wrong prefix....

[hudokkow]

jenkins build this please

[xhaggi]

iirc we are now pointing to the master branch. i don't know why this doesn't work for you.

[ghost]

I passed the wrong prefix=[] to the make call wich builds the pvr addon.
So version 2.2.5 was installed into the wrong directory (and not found by the kodi app). With correct prefix everything is fine, sorry for asking so early....

[xhaggi]

np. build errors are unrelated.

[pejobo]

Is this issue present in Isengard15.1? After updating from 15.0 it crashes on startup. If I start without PVR (clean userdata) it crashes as soon as I enable PVR (TvHeadend as backend). Could this be related?
[EDIT]
Going back from 15.1 to 15.0 "fixes" my problem. So it seems to be another problem, introduced with 15.1

[ghost]

@pejobo are you sure the merge described above is aready in the 15.1 you took?
Do you compile by yourself? Or do you use something precompiled?

[pejobo]

No, but the effect of the bug should be visible in 15.0 too, shouldn't it? It crashed every time with 15.1 right after upgrading. Even with fresh (empty) settings directory when enabling pvr. The problem disappeared after going back to 15.0.

[pejobo]

Sorry, forgot to answer your questions. Precompiled kodi from arch repository and self compiled tvh plugin. I also recompiled version for the pvr plugin to rule out incompatibilities.

[ghost]

I have 2 devices running kodi, one is 15.0 and it runs without any problems.
The other device is with the latest 15.1, and without the fix here I had the same problems as you describe.

So it seems that the problem is new in the 15.1 (and was not present in 15.0)

[pejobo]

@mapfau So you compiled with this fix here and it fixed your issue with 15.1?
If yes the bug described here was dormant in 15.0 and has been triggered by another change in 15.1...

[ghost]

@pejobo totally right, this fix here was only for 15.1, there was no need to fix anything in 15.0.

[pejobo]

Will the fix then be included in 15.2? It's a severe regression..

[FernetMenta]

I am going to revert this. Triggering Persist on a system that os configured not to use DB makes no sense.

[ghost]

I'm good with everything - the fix here speeds up db in my case (because EPG was pulled in a defined order) and it works without crash (wich was not the case without this fix).

If you find a nother way to sync the both EPG sources together, perfect!

[FernetMenta]

This change makes things worse if you don't use DB. I don't get the sense behind this? There is only a single line that affects DB users. All the rest is makes non DB usage worse. Please elaborate.