Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[pvr] make all livetv views available to the user earlier (as soon as channels are fetched from the backend and before epg is loaded from the db) by changing the pvr/epg manager startup logic #1994

Merged
merged 2 commits into from

4 participants

@nemphys

This is quite a big one, but tested to greatly reduce the time needed for the LiveTV views to be available to the user upon startup.

Right now, the logic is a little weird, as PVR manager thread is supposed to first load channel groups, etc, and then ask the EpgContainer to start before it enters its main loop.

The thing is that as soon as all channels are loaded from the db and updated, CreateChannelEpgs is executed (this is done after every internal channel group is loaded), which in turn calls LoadFromDB for the EPG data before it does its work.
Therefore, EPG data is loaded from the database before the epg thread is even asked to start, which causes a huge delay (especially on installations with many channels and slow hardware) before PVR is actually usable (in my secondary RPi setup, this takes more than 5 minutes(!)).

I changed the logic, so that PVR manager loads/updates the channels, but CreateChannelEpgs is only called as a trigger/callback when the main loop has started and therefore EPG data has been loaded from the database. The EPG loading itself has been changed to lock only when needed and not during the whole process.

This means that upon startup, all LiveTV views (and channel viewing, etc) are available as soon as the channels are loaded, even before epg is loaded from the database.
As a side-effect, it also cures the "ugly" overdrawing of the channel and epg loading popups.

In my opinion, it is crucial not to have the user wait for minutes before he/she can start watching TV.

@opdenkamp
Collaborator

i'll review this after frodo is out

@nemphys

Hmm, was hoping that it would make it to Frodo, since the current behavior is slow/ugly.
Have you tried initializing PVR in an installation with 1700 channels and a slow machine? (eg.Ion, or even worse rpi) :-)
Btw, when is Frodo coming out?

PS. At least check PR #1982, it will be a real shame if that one doesn't make it to Frodo, too

@opdenkamp
Collaborator

soon. only critical fixes or minor changes until frodo is out.

@da-anda
Collaborator

this PR causes some issues in my test build. If I try to access one of the liveTV windows but PVR addon is not yet completely loaded (IIRC channels are not yet fully imported) I sometimes only get a black screen and the only possibility to "close" it is by pressing "h" or "home" to get to the home screen.

@opdenkamp opdenkamp was assigned
@MartijnKaijser

@opdenkamp
Any comments on this PR?

@nemphys
please rebase

@nemphys

Just rebased.
FYI I have been running this for the past 3 months or so without any issues...

@opdenkamp
Collaborator

@nemphys could you rebase another time please
@da-anda if you still encounter the black screen after this, please create a ticket and attach a debug log

@nemphys

Rebased, still using it without problems
EDIT: DO NOT MERGE YET, just compiled the rebased branch and it does not work as expected; must look into it further

@nemphys

OK, found the culprit: it was commit bdf8842 (by @opdenkamp ), which I had reverted just after it was merged in January (and forgotten about it since).
This commit breaks the functionality of my commit. Reverting it brings it back to normal; tell me what you want me to do.

@opdenkamp
Collaborator

right indeed, these two conflict, forgot about that one. please add the revert for that one to this pr, and we'll shove it in

@nemphys

Done

@opdenkamp opdenkamp commented on the diff
xbmc/epg/EpgContainer.cpp
@@ -174,6 +195,8 @@ void CEpgContainer::OnSettingChanged(const CSetting *setting)
void CEpgContainer::LoadFromDB(void)
{
+ CSingleLock lock(m_critSection);
@opdenkamp Collaborator

sorry, missed this one before. locking in here isn't very useful like this.
best just create a copy of m_epgs and iterate over that without holding a lock. only thing that can happen here really is that tables are being created, and those will be picked up later

@nemphys
nemphys added a note

If you check the code, ::LoadFromDB is called from ::Start, which (before my changes) held a lock during the call anyway.
The way I did it, the lock is only held when necessary, as the lock is released before calling ::LoadFromDB and created again after it is finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MartijnKaijser

@nemphys can you update and rebase this PR?

nemphys added some commits
@nemphys nemphys [pvr] make all livetv views available to the user earlier (as soon as…
… channels are fetched from the backend and before epg is loaded from the db) by changing the pvr/epg manager startup logic
3244000
@nemphys nemphys Revert "[epg] fixed - epg load/update was interrupted when the pvr ma…
…nager imports channels, leading to delays and channel ids gone missing in certain situations"

This reverts commit bdf8842.

Conflicts:

	xbmc/pvr/PVRManager.h
75c8a08
@nemphys

Rebased.

@opdenkamp opdenkamp merged commit b0f94c7 into from
@CharlieMarshall CharlieMarshall referenced this pull request from a commit in CharlieMarshall/xbmc
@CharlieMarshall CharlieMarshall Revert "Merge pull request #1994 from nemphys/pvr-epg-startup"
This reverts commit b0f94c7, reversing
changes made to dcd897b.
f76419f
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 6, 2013
  1. @nemphys

    [pvr] make all livetv views available to the user earlier (as soon as…

    nemphys authored
    … channels are fetched from the backend and before epg is loaded from the db) by changing the pvr/epg manager startup logic
  2. @nemphys

    Revert "[epg] fixed - epg load/update was interrupted when the pvr ma…

    nemphys authored
    …nager imports channels, leading to delays and channel ids gone missing in certain situations"
    
    This reverts commit bdf8842.
    
    Conflicts:
    
    	xbmc/pvr/PVRManager.h
This page is out of date. Refresh to see the latest.
View
70 xbmc/epg/EpgContainer.cpp
@@ -53,6 +53,7 @@ CEpgContainer::CEpgContainer(void) :
m_iNextEpgId = 0;
m_bPreventUpdates = false;
m_updateEvent.Reset();
+ m_bStarted = false;
m_bLoaded = false;
m_bHasPendingUpdates = false;
}
@@ -74,6 +75,12 @@ void CEpgContainer::Unload(void)
Clear(false);
}
+bool CEpgContainer::IsStarted(void) const
+{
+ CSingleLock lock(m_critSection);
+ return m_bStarted;
+}
+
unsigned int CEpgContainer::NextEpgId(void)
{
CSingleLock lock(m_critSection);
@@ -100,6 +107,7 @@ void CEpgContainer::Clear(bool bClearDb /* = false */)
}
m_epgs.clear();
m_iNextEpgUpdate = 0;
+ m_bStarted = false;
m_bIsInitialising = true;
m_iNextEpgId = 0;
}
@@ -125,24 +133,34 @@ void CEpgContainer::Start(void)
{
Stop();
- CSingleLock lock(m_critSection);
+ {
+ CSingleLock lock(m_critSection);
- if (!m_database.IsOpen())
- m_database.Open();
+ if (!m_database.IsOpen())
+ m_database.Open();
- m_bIsInitialising = true;
- m_bStop = false;
- LoadSettings();
+ m_bIsInitialising = true;
+ m_bStop = false;
+ LoadSettings();
- m_iNextEpgUpdate = 0;
- m_iNextEpgActiveTagCheck = 0;
+ m_iNextEpgUpdate = 0;
+ m_iNextEpgActiveTagCheck = 0;
+ }
LoadFromDB();
- CheckPlayingEvents();
- Create();
- SetPriority(-1);
- CLog::Log(LOGNOTICE, "%s - EPG thread started", __FUNCTION__);
+ CSingleLock lock(m_critSection);
+ if (!m_bStop)
+ {
+ CheckPlayingEvents();
+
+ Create();
+ SetPriority(-1);
+
+ m_bStarted = true;
+
+ CLog::Log(LOGNOTICE, "%s - EPG thread started", __FUNCTION__);
+ }
}
bool CEpgContainer::Stop(void)
@@ -152,6 +170,9 @@ bool CEpgContainer::Stop(void)
if (m_database.IsOpen())
m_database.Close();
+ CSingleLock lock(m_critSection);
+ m_bStarted = false;
+
return true;
}
@@ -174,6 +195,8 @@ void CEpgContainer::OnSettingChanged(const CSetting *setting)
void CEpgContainer::LoadFromDB(void)
{
+ CSingleLock lock(m_critSection);
@opdenkamp Collaborator

sorry, missed this one before. locking in here isn't very useful like this.
best just create a copy of m_epgs and iterate over that without holding a lock. only thing that can happen here really is that tables are being created, and those will be picked up later

@nemphys
nemphys added a note

If you check the code, ::LoadFromDB is called from ::Start, which (before my changes) held a lock during the call anyway.
The way I did it, the lock is only held when necessary, as the lock is released before calling ::LoadFromDB and created again after it is finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
if (m_bLoaded || m_bIgnoreDbForClient)
return;
@@ -193,14 +216,17 @@ void CEpgContainer::LoadFromDB(void)
for (map<unsigned int, CEpg *>::iterator it = m_epgs.begin(); it != m_epgs.end(); it++)
{
+ if (m_bStop)
+ break;
UpdateProgressDialog(++iCounter, m_epgs.size(), it->second->Name());
+ lock.Leave();
it->second->Load();
+ lock.Enter();
}
CloseProgressDialog();
}
- CSingleLock lock(m_critSection);
m_bLoaded = bLoaded;
}
@@ -233,12 +259,6 @@ void CEpgContainer::Process(void)
bool bUpdateEpg(true);
bool bHasPendingUpdates(false);
- if (!CPVRManager::Get().WaitUntilInitialised())
- {
- CLog::Log(LOGDEBUG, "EPG - %s - pvr manager failed to load - exiting", __FUNCTION__);
- return;
- }
-
while (!m_bStop && !g_application.m_bStop)
{
CDateTime::GetCurrentDateTime().GetAsUTCDateTime().GetAsTime(iNow);
@@ -248,7 +268,7 @@ void CEpgContainer::Process(void)
}
/* update the EPG */
- if (!InterruptUpdate() && bUpdateEpg && UpdateEPG())
+ if (!InterruptUpdate() && bUpdateEpg && g_PVRManager.EpgsCreated() && UpdateEPG())
m_bIsInitialising = false;
/* clean up old entries */
@@ -334,7 +354,6 @@ CEpg *CEpgContainer::CreateChannelEpg(CPVRChannelPtr channel)
return NULL;
WaitForUpdateFinish(true);
- CSingleLock lock(m_critSection);
LoadFromDB();
CEpg *epg(NULL);
@@ -345,6 +364,8 @@ CEpg *CEpgContainer::CreateChannelEpg(CPVRChannelPtr channel)
{
channel->SetEpgID(NextEpgId());
epg = new CEpg(channel, false);
+
+ CSingleLock lock(m_critSection);
m_epgs.insert(make_pair((unsigned int)epg->EpgID(), epg));
SetChanged();
epg->RegisterObserver(this);
@@ -352,8 +373,11 @@ CEpg *CEpgContainer::CreateChannelEpg(CPVRChannelPtr channel)
epg->SetChannel(channel);
- m_bPreventUpdates = false;
- CDateTime::GetCurrentDateTime().GetAsUTCDateTime().GetAsTime(m_iNextEpgUpdate);
+ {
+ CSingleLock lock(m_critSection);
+ m_bPreventUpdates = false;
+ CDateTime::GetCurrentDateTime().GetAsUTCDateTime().GetAsTime(m_iNextEpgUpdate);
+ }
NotifyObservers(ObservableMessageEpgContainer);
View
7 xbmc/epg/EpgContainer.h
@@ -95,6 +95,12 @@ namespace EPG
virtual void Reset(void) { Clear(true); }
/*!
+ * @brief Check whether the EpgContainer has fully started.
+ * @return True if started, false otherwise.
+ */
+ bool IsStarted(void) const;
+
+ /*!
* @brief Delete an EPG table from this container.
* @param epg The table to delete.
* @param bDeleteFromDatabase Delete this table from the database too if true.
@@ -271,6 +277,7 @@ namespace EPG
//@{
bool m_bIsUpdating; /*!< true while an update is running */
bool m_bIsInitialising; /*!< true while the epg manager hasn't loaded all tables */
+ bool m_bStarted; /*!< true if EpgContainer has fully started */
bool m_bLoaded; /*!< true after epg data is initially loaded from the database */
bool m_bPreventUpdates; /*!< true to prevent EPG updates */
bool m_bHasPendingUpdates; /*!< true if there are manual updates pending */
View
49 xbmc/pvr/PVRManager.cpp
@@ -74,6 +74,7 @@ CPVRManager::CPVRManager(void) :
m_currentFile(NULL),
m_database(NULL),
m_bFirstStart(true),
+ m_bEpgsCreated(false),
m_progressHandle(NULL),
m_managerState(ManagerStateStopped),
m_bOpenPVRWindow(false)
@@ -261,12 +262,6 @@ bool CPVRManager::UpgradeOutdatedAddons(void)
return false;
}
-bool CPVRManager::WaitUntilInitialised(void)
-{
- return m_initialisedEvent.Wait() &&
- IsStarted();
-}
-
void CPVRManager::Cleanup(void)
{
CSingleLock lock(m_critSection);
@@ -284,12 +279,12 @@ void CPVRManager::Cleanup(void)
m_bIsSwitchingChannels = false;
m_outdatedAddons.clear();
m_bOpenPVRWindow = false;
+ m_bEpgsCreated = false;
for (unsigned int iJobPtr = 0; iJobPtr < m_pendingUpdates.size(); iJobPtr++)
delete m_pendingUpdates.at(iJobPtr);
m_pendingUpdates.clear();
- m_initialisedEvent.Reset();
SetState(ManagerStateStopped);
}
@@ -352,8 +347,6 @@ void CPVRManager::Start(bool bAsync /* = false */, bool bOpenPVRWindow /* = fals
m_database = new CPVRDatabase;
m_database->Open();
- g_EpgContainer.Start();
-
/* create the supervisor thread to do all background activities */
StartUpdateThreads();
}
@@ -368,7 +361,6 @@ void CPVRManager::Stop(void)
SetState(ManagerStateStopping);
/* stop the EPG updater, since it might be using the pvr add-ons */
- m_initialisedEvent.Set();
g_EpgContainer.Stop();
CLog::Log(LOGNOTICE, "PVRManager - stopping");
@@ -408,6 +400,8 @@ void CPVRManager::SetState(ManagerState state)
void CPVRManager::Process(void)
{
+ g_EpgContainer.Stop();
+
/* load the pvr data from the db and clients if it's not already loaded */
while (!Load() && GetState() == ManagerStateStarting)
{
@@ -425,7 +419,7 @@ void CPVRManager::Process(void)
/* main loop */
CLog::Log(LOGDEBUG, "PVRManager - %s - entering main loop", __FUNCTION__);
- m_initialisedEvent.Set();
+ g_EpgContainer.Start();
if (m_bOpenPVRWindow)
{
@@ -914,6 +908,11 @@ CPVRChannelGroupPtr CPVRManager::GetPlayingGroup(bool bRadio /* = false */)
return CPVRChannelGroupPtr();
}
+bool CPVREpgsCreateJob::DoWork(void)
+{
+ return g_PVRManager.CreateChannelEpgs();
+}
+
bool CPVRRecordingsUpdateJob::DoWork(void)
{
g_PVRRecordings->Update();
@@ -1349,6 +1348,12 @@ bool CPVRManager::IsStarted(void) const
return GetState() == ManagerStateStarted;
}
+bool CPVRManager::EpgsCreated(void) const
+{
+ CSingleLock lock(m_critSection);
+ return m_bEpgsCreated;
+}
+
bool CPVRManager::IsPlayingTV(void) const
{
return IsStarted() && m_addons && m_addons->IsPlayingTV();
@@ -1397,6 +1402,18 @@ bool CPVRManager::IsJobPending(const char *strJobName) const
return bReturn;
}
+void CPVRManager::TriggerEpgsCreate(void)
+{
+ CSingleLock lock(m_critSectionTriggers);
+ if (IsJobPending("pvr-create-epgs"))
+ return;
+
+ m_pendingUpdates.push_back(new CPVREpgsCreateJob());
+
+ lock.Leave();
+ m_triggerEvent.Set();
+}
+
void CPVRManager::TriggerRecordingsUpdate(void)
{
CSingleLock lock(m_critSectionTriggers);
@@ -1540,3 +1557,13 @@ bool CPVRChannelSwitchJob::DoWork(void)
return true;
}
+
+bool CPVRManager::CreateChannelEpgs(void)
+{
+ if (EpgsCreated())
+ return true;
+
+ CSingleLock lock(m_critSection);
+ m_bEpgsCreated = m_channelGroups->CreateChannelEpgs();
+ return m_bEpgsCreated;
+}
View
29 xbmc/pvr/PVRManager.h
@@ -250,6 +250,12 @@ namespace PVR
bool IsStarted(void) const;
/*!
+ * @brief Check whether EPG tags for channels have been created.
+ * @return True if EPG tags have been created, false otherwise.
+ */
+ bool EpgsCreated(void) const;
+
+ /*!
* @brief Reset the playing EPG tag.
*/
void ResetPlayingTag(void);
@@ -333,6 +339,11 @@ namespace PVR
CPVRChannelGroupPtr GetPlayingGroup(bool bRadio = false);
/*!
+ * @brief Let the background thread create epg tags for all channels.
+ */
+ void TriggerEpgsCreate(void);
+
+ /*!
* @brief Let the background thread update the recordings list.
*/
void TriggerRecordingsUpdate(void);
@@ -510,6 +521,12 @@ namespace PVR
static void SettingOptionsPvrStartLastChannelFiller(const CSetting *setting, std::vector< std::pair<std::string, int> > &list, int &current);
+ /*!
+ * @brief Create EPG tags for all channels in internal channel groups
+ * @return True if EPG tags where created successfully, false otherwise
+ */
+ bool CreateChannelEpgs(void);
+
protected:
/*!
* @brief PVR update and control thread.
@@ -611,6 +628,7 @@ namespace PVR
CCriticalSection m_critSection; /*!< critical section for all changes to this class, except for changes to triggers */
bool m_bFirstStart; /*!< true when the PVR manager was started first, false otherwise */
bool m_bIsSwitchingChannels; /*!< true while switching channels */
+ bool m_bEpgsCreated; /*!< true if epg data for channels has been created */
CGUIDialogProgressBarHandle * m_progressHandle; /*!< progress dialog that is displayed while the pvrmanager is loading */
CCriticalSection m_managerStateMutex;
@@ -618,7 +636,16 @@ namespace PVR
CStopWatch *m_parentalTimer;
bool m_bOpenPVRWindow;
std::map<std::string, std::string> m_outdatedAddons;
- CEvent m_initialisedEvent; /*!< triggered when the pvr manager initialised */
+ };
+
+ class CPVREpgsCreateJob : public CJob
+ {
+ public:
+ CPVREpgsCreateJob(void) {}
+ virtual ~CPVREpgsCreateJob() {}
+ virtual const char *GetType() const { return "pvr-create-epgs"; }
+
+ virtual bool DoWork();
};
class CPVRRecordingsUpdateJob : public CJob
View
6 xbmc/pvr/channels/PVRChannelGroup.cpp
@@ -1226,3 +1226,9 @@ bool CPVRChannelGroup::IsSelectedGroup(void) const
CSingleLock lock(m_critSection);
return m_bSelectedGroup;
}
+
+bool CPVRChannelGroup::CreateChannelEpgs(bool bForce /* = false */)
+{
+ /* used only by internal channel groups */
+ return true;
+}
View
7 xbmc/pvr/channels/PVRChannelGroup.h
@@ -415,6 +415,13 @@ namespace PVR
bool RemoveDeletedChannels(const CPVRChannelGroup &channels);
/*!
+ * @brief Create an EPG table for each channel.
+ * @brief bForce Create the tables, even if they already have been created before.
+ * @return True if all tables were created successfully, false otherwise.
+ */
+ virtual bool CreateChannelEpgs(bool bForce = false);
+
+ /*!
* @brief Remove invalid channels from this container.
*/
void RemoveInvalidChannels(void);
View
4 xbmc/pvr/channels/PVRChannelGroupInternal.cpp
@@ -60,7 +60,7 @@ bool CPVRChannelGroupInternal::Load(void)
if (CPVRChannelGroup::Load())
{
UpdateChannelPaths();
- CreateChannelEpgs();
+ g_PVRManager.TriggerEpgsCreate();
return true;
}
@@ -369,6 +369,8 @@ void CPVRChannelGroupInternal::CreateChannelEpg(CPVRChannelPtr channel, bool bFo
bool CPVRChannelGroupInternal::CreateChannelEpgs(bool bForce /* = false */)
{
+ if (!g_EpgContainer.IsStarted())
+ return false;
{
CSingleLock lock(m_critSection);
for (unsigned int iChannelPtr = 0; iChannelPtr < m_members.size(); iChannelPtr++)
View
13 xbmc/pvr/channels/PVRChannelGroups.cpp
@@ -519,3 +519,16 @@ void CPVRChannelGroups::FillGroupsGUI(int iWindowId, int iControlId) const
CGUIMessage msgSel(GUI_MSG_ITEM_SELECT, iWindowId, iControlId, iSelectedGroupPtr);
g_windowManager.SendMessage(msgSel);
}
+
+bool CPVRChannelGroups::CreateChannelEpgs(void)
+{
+ bool bReturn(false);
+ CSingleLock lock(m_critSection);
+ for (std::vector<CPVRChannelGroupPtr>::iterator it = m_groups.begin(); it != m_groups.end(); it++)
+ {
+ /* Only create EPGs for the internatl groups */
+ if ((*it)->IsInternalGroup())
+ bReturn = (*it)->CreateChannelEpgs();
+ }
+ return bReturn;
+}
View
6 xbmc/pvr/channels/PVRChannelGroups.h
@@ -163,6 +163,12 @@ namespace PVR
bool DeleteGroup(const CPVRChannelGroup &group);
/*!
+ * @brief Create EPG tags for all channels of the internal group.
+ * @return True if EPG tags where created successfully, false if not.
+ */
+ bool CreateChannelEpgs(void);
+
+ /*!
* @brief Remove a channel from all non-system groups.
* @param channel The channel to remove.
*/
View
7 xbmc/pvr/channels/PVRChannelGroupsContainer.cpp
@@ -282,3 +282,10 @@ bool CPVRChannelGroupsContainer::CreateChannel(const CPVRChannel &channel)
{
return GetGroupAll(channel.IsRadio())->AddNewChannel(channel);
}
+
+
+bool CPVRChannelGroupsContainer::CreateChannelEpgs(void)
+{
+ return m_groupsRadio->CreateChannelEpgs() &&
+ m_groupsTV->CreateChannelEpgs();
+}
View
6 xbmc/pvr/channels/PVRChannelGroupsContainer.h
@@ -182,6 +182,12 @@ namespace PVR
bool CreateChannel(const CPVRChannel &channel);
+ /*!
+ * @brief Create EPG tags for channels in all internal channel groups.
+ * @return True if EPG tags were created succesfully.
+ */
+ bool CreateChannelEpgs(void);
+
protected:
/*!
* @brief Update the contents of all the groups in this container.
Something went wrong with that request. Please try again.