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] start epg container asynchronously #6937

Merged
merged 1 commit into from
Aug 3, 2015

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Apr 12, 2015

As the title said this adds asynchronously start of the EPG container which do not block the entire PVR manager startup and results in a really fast resumption of playback for the last played channel if you enable "continue last played channel" in TV settings.

@opdenkamp or @Jalle19 ping

@xhaggi xhaggi added the Type: Improvement non-breaking change which improves existing functionality label Apr 12, 2015
@xhaggi xhaggi force-pushed the pvr-async-epg-container-start branch 2 times, most recently from 6ce5e4b to 34c364f Compare April 12, 2015 18:48
@AlwinEsch
Copy link
Member

Have checked, does not work.

The

bool CPVRManager::CreateChannelEpgs(void)
{
  ...
  m_bEpgsCreated = m_channelGroups->CreateChannelEpgs();
  ...
}

returns false and on

void CEpgContainer::Process(void)
{
  ...
  if (!InterruptUpdate() && bUpdateEpg && g_PVRManager.EpgsCreated() && UpdateEPG())
  ...
}

the "UpdateEPG" becomes never called.

EDIT: Was occurred after database reset and also if database was deleted before.

@FernetMenta
Copy link
Contributor

there are issues when starting playback on a channel which has no valid epg. info shown on OSD seems to get not updated when there is a valid egp.
imo this should be fixed before.

@xhaggi xhaggi force-pushed the pvr-async-epg-container-start branch from 34c364f to d668a8c Compare April 12, 2015 19:42
@xhaggi
Copy link
Member Author

xhaggi commented Apr 12, 2015

@AlwinEsch thanks for pointing this out, it's now fixed.
@FernetMenta i don't have any issue with what you said.

@FernetMenta
Copy link
Contributor

@xhaggi have you ever tried not to store EGP in the db? probably not or you would have observed this issue. Not storing EPG in pvr db is default scenario for vnsi because vdr has its own db for EPG and startup is much faster this way.

@xhaggi
Copy link
Member Author

xhaggi commented Apr 12, 2015

@FernetMenta sure and that's why I want to load the epg data from the database async because IMO this shouldn't block the PVR manager startup.

@AlwinEsch sorry my fix won't work :(

@FernetMenta
Copy link
Contributor

it would cause a regression for vnsi which already starts up very fast. I am not opposed to this but the issue mentioned should be fixed first.

@xhaggi
Copy link
Member Author

xhaggi commented Apr 12, 2015

I don't want to change the way how we get clients epg information into kodi. After a second look we should think about the way we load the data from our database. IMO we should load the client data first and later the data we stored in our database (async). If the related epg data already exists in memory we can skip it from being created from our database data. Currently we load the epg from our database first and later the epg data from the client and decide if we need to update the epg data in our database which is IMO not the right order.

@FernetMenta
Copy link
Contributor

In case of VNSI the db is never used so what is loaded first does not matter. The point is that when a channel is started some data is pulled from EPG container which never gets updated during playback. start a channels with no EPG, then trigger EPG update for the channel playing.
btw: there are a couple of other things not working properly, i.e. EPG tags on channels in pvr window. Sometimes they get updated when moving the curser away and disappear when moving the cursor on the item. This occurs when Kodi is started with no EPG and EPG is updated while running triggered by the backend.

@xhaggi
Copy link
Member Author

xhaggi commented Apr 13, 2015

In case of VNSI the db is never used so what is loaded first does not matter.

Fine for VNSI then. This PR ( if it works ;o) ) addresses all backends which relies on kodi's database.

The point is that when a channel is started some data is pulled from EPG container which never gets updated during playback.

Yep i know this issue. We can look into it in the next few weeks, where we can't push new stuff.

@FernetMenta
Copy link
Contributor

Yep i know this issue. We can look into it in the next few weeks, where we can't push new stuff.

This should be fixed before this PR goes in. Otherwise it makes things worse for vnsi.

@xhaggi xhaggi force-pushed the pvr-async-epg-container-start branch from 7b29f5a to 24e7387 Compare July 31, 2015 15:08
@xhaggi
Copy link
Member Author

xhaggi commented Jul 31, 2015

@FernetMenta with the changes I've done in #7667 now the async startup of the epg container does not introduce any regression. It will speed-up things a lot. I would prefer to merge this after #7667 goes in.

@opdenkamp @ksooo @Jalle19 any objections

@FernetMenta
Copy link
Contributor

very nice and thank you very much! Now epg gets visible right after vnsiserver requests an update.

@xhaggi
Copy link
Member Author

xhaggi commented Jul 31, 2015

ah does it mean you've tested #7667 on top of this?

@FernetMenta
Copy link
Contributor

I merged both for the test.

@xhaggi
Copy link
Member Author

xhaggi commented Jul 31, 2015

great, thanks :)

@ksooo
Copy link
Member

ksooo commented Jul 31, 2015

No objections from my side. Not runtime tested, but code changes are looking straight forward to me.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 2, 2015

jenkins build this please

xhaggi added a commit that referenced this pull request Aug 3, 2015
[epg] start epg container asynchronously
@xhaggi xhaggi merged commit 856414b into xbmc:master Aug 3, 2015
@hudokkow hudokkow added this to the J**** 16.0-alpha2 milestone Aug 3, 2015
@xhaggi xhaggi deleted the pvr-async-epg-container-start branch August 6, 2015 08:47
@ryangribble
Copy link
Contributor

this seems to have broken things for me, my EPG is never loaded (no calls to GetEpgForChannel() are ever made).

image

I have tried with "do not store EPG in database" option enabled and disabled but am getting the problem in both cases.

If i hardcode the bAsync variable in EpgContainer.Start() to false, my EPG loads fine again, which is why I think it is this change. Any advice on how to further troubleshoot?

@xhaggi
Copy link
Member Author

xhaggi commented Aug 6, 2015

please provide some more information:

  • debug log
  • version information (system info)

@ryangribble
Copy link
Contributor

My debugging leads me to
https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/channels/PVRChannelGroupInternal.cpp#L335

CPVRChannelGroupInternal::CreateChannelEpgs()
{
    if (!g_EpgContainer.IsStarted())
        return false;

The function is exiting at this point because EpgContainer is not started, so it never proceeds on to call CreateChannelEpg()

When I set bAsync to false in EpgContainer::Start(), EpgContainer.IsStarted() is false at this same point, and thus this method does not exit here and then goes on to load the Channel Epg's

Debug Log

Version Info: Im running this branch which is the #7626 [PVR] API 3.0.0 Changes PR rebased on top of latest kodi master (commit fb174d6)

@xhaggi
Copy link
Member Author

xhaggi commented Aug 6, 2015

@ryangribble could you please move g_PVRManager.TriggerEpgsCreate(); from

https://github.com/xbmc/xbmc/blob/master/xbmc/epg/EpgContainer.cpp#L174

to

https://github.com/xbmc/xbmc/blob/master/xbmc/epg/EpgContainer.cpp#L187

and try if this will fix the issue. If so I'll create a PR asap.

@ryangribble
Copy link
Contributor

OK so that made the Epg load however it loaded each channel EPG about 11 times back to back before it finally stopped :)

debug log

@xhaggi
Copy link
Member Author

xhaggi commented Aug 6, 2015

I can't reproduce the issue. I've added a debug log to CPVRChannelGroupInternal::CreateChannelEpgs(bool bForce); and it only appears twice in my log, one for TV and one for Radio before everything is loaded.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 6, 2015

please checkout the current master without anything else. we need the same version for testing things.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 6, 2015

@ryangribble see #7734

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants