Skip to content
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

[epg][fix] trigger epgs create after we started the epg container #7734

Merged
merged 1 commit into from
Aug 8, 2015

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Aug 6, 2015

@ryangribble let's further discuss you issue here.

@xhaggi xhaggi added the Type: Fix non-breaking change which fixes an issue label Aug 6, 2015
@ryangribble
Copy link
Contributor

thanks @xhaggi, im in GMT +10 and finished for the night so I'll do some more testing etc tomorrow night, including rolling back to upstream master

@ryangribble
Copy link
Contributor

Btw I run with "don't store Epg in the database" incase that's a factor

@xhaggi
Copy link
Member Author

xhaggi commented Aug 6, 2015

tested both (with and without local caching in our db) and everything works fine. I've also cleared the database etc. and there is no issue.

@Jalle19
Copy link
Member

Jalle19 commented Aug 7, 2015

@opdenkamp do you see any potential problems with this?

@xhaggi xhaggi force-pushed the epg-fix-trigger-epgs-create branch from a91ca69 to 78fd924 Compare August 7, 2015 05:54
@xhaggi xhaggi changed the title [epg][fix] trigger eps create after we started the epg container [epg][fix] trigger epgs create after we started the epg container Aug 7, 2015
@xhaggi
Copy link
Member Author

xhaggi commented Aug 7, 2015

fixed a typo in the commit message

@opdenkamp
Copy link
Member

I'll free some time to review all PRs in which I got pinged some evening
later this week. I've had a couple of delays in the house building work,
some of which I must get finished before Monday.

On 07-08-15 07:52, Sam Stenvall wrote:

@opdenkamp https://github.com/opdenkamp do you see any potential
problems with this?


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

@xhaggi
Copy link
Member Author

xhaggi commented Aug 7, 2015

@ryangribble any news on this?

@ryangribble
Copy link
Contributor

Sorry didn't get a chance to look yet. This weekend I will for sure

@ryangribble
Copy link
Contributor

OK so just to summarise, i found that after #6937 the EPG would no longer load for me at all. This PR #7734 was the fix suggested by @xhaggi but it seemed I had behaviour where my EPG loaded each channel 11 times with this fix, but I needed time to do more testing.

Ive now reset my workspace back to fully clean kodi master + this PR branch only and done some testing.

The issue is not the "importing guide from clients" but actually the "loading guide from database" phase that happens first. (so it only happens when "Don't cache in local database" is set OFF).

It seems that every time I restart Kodi, the "load from database" phase does an extra run through of the full channel list. I had a look in the PVR and EPG database files with SQLLiteBrowser and the PVR database is fine, the number of channels is not changing. However in the Epg database, the entries in epg table are increasing on each restart (I have 22 channels so they are going 22 44 66 88 etc), so somehow Kodi is storing each channel in the EPG database again on each run rather than using the existing one?

Debug Log
Screenshot of EPG database query for a particular channel, showing it has 6 duplicates:

image

To determine if this is caused by #6937 and/or #7734, I reverted this PR, and then set bAsync to false (effectively backing out the #6937 async change as well). The problem still happens. So therefore it isnt related to either of these 2 PR specifically and is some other EPG Database problem. I just havent noticed previously since I dont generally run with EPG database enabled.

@ryangribble
Copy link
Contributor

OK so the problem is caused by the channel groups + channels loaded from the PVR database, the channels have an iEpgId of 0 in the PVR database. Therefore at this point in CEpgContainer::CreateChannelEpg() it always creates a newentry in the EPG database (epg table) for that channel, since it can't find the existing entry (since epgID in PVR database was 0).

This is an edge case scenario where if you originally stored channels in the PVR database while "Dont cache in local database" for EPG is set, then the channels all have iEpgId=0 (possibly only if you have done a clear data on Epg database as well, im not sure on the exact steps to reproduce a zero value idEpg in the channels PVR database). Then at any point in the future, if you turn off "Dont cache in local database" for EPG, it will constantly create duplicates since all the channels in the PVR database are stored with 0 epgID still and they never get updated. Resetting the PVR database and loading everything again with the EPG database enabled, fixes the problem (channel items in PVR database now have EpgID associated).

So im guessing there must be a bug somewhere, when a "new" Epg database "epg" item is created for an existing channel, that channel's iEpgId is either not set or that object is not flagged as changed and not persisted to PVR database. Ill see if i can track it down and submit a fix. A cursory run through seems to show that there are several safeguards to try and set the EpgId on a channel object and flag it as changed, so im not sure why it isnt being updated in the database... perhaps PVR database is not persisted except on initial channel load or something like that

Suffice to say this PR should be merged since it fixes the Epg loading after the previous EpgContainer Async change stopped it

@xhaggi
Copy link
Member Author

xhaggi commented Aug 8, 2015

ok nice find I'll merge it then.

jenkins build and merge

@ryangribble
Copy link
Contributor

Cool. In some quick poking around Ive done, this change seems to fix things. It looks like the channel record isnt correctly flagged as Changed when the EpgId is set. I thought SetChanged() was doing it, but that seems to be setting a Observable changed flag rather than a DB/record changed flag.

Ill do some more testing over the weekend and submit a PR if it looks OK

@xhaggi
Copy link
Member Author

xhaggi commented Aug 8, 2015

thanks for doing it :)

@xhaggi
Copy link
Member Author

xhaggi commented Aug 8, 2015

build errors are unrelated

xhaggi added a commit that referenced this pull request Aug 8, 2015
[epg][fix] trigger epgs create after we started the epg container
@xhaggi xhaggi merged commit d42e33c into xbmc:master Aug 8, 2015
@xhaggi xhaggi deleted the epg-fix-trigger-epgs-create branch February 5, 2016 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants