[pvr] finally make it possible to use multiple PVR clients #7020

Jalle19 · 2015-04-27T08:46:26Z

I've grown tired of telling users that it's not possible to use more than one addon at a time due to a bug, so this is my attempt at fixing it. We really need a solution for this for Isengard so I'd appreciate it if we can keep the bikeshedding to a minimum and focus on getting something that works done, even though the solution might not be the prettiest.

The problem:

the addons are started by CPVRClients on a background thread. Meanwhile, CPVRManager used a simple sleep loop while waiting for at least one addon to load before continuing. If the second (or third) addon didn't load during the same 50 millisecond sleep duration, the startup would continue with just one addon.

The solution:

use a boolean in CPVRClients to indicate whether all clients have been loaded or not. From CPVRManager we call a method which blocks until this boolean's condition variable is signaled.

Ping @opdenkamp @ksooo @xhaggi @FernetMenta

I'm not convinced this is the ultimate solution, but at least it works and it doesn't introduce arbitrary sleep durations.

xhaggi · 2015-04-27T12:14:10Z

does not runtime test it, but looks good.

ksooo · 2015-04-27T14:40:45Z

Looking good, wrt pragmatic approach to improve things for Isengard. Though I did not runtime test.

Jalle19 · 2015-04-27T16:35:02Z

It's easy to runtime test, just enable pvr.demo and start Kodi a few times, checking that both your normal PVR client and the demo client gets loaded.

xhaggi · 2015-04-27T16:40:18Z

IMO shove it in, even better than it was before.

Jalle19 · 2015-04-27T17:47:02Z

I'd still like to hear @opdenkamp's thoughts

MartijnKaijser · 2015-04-27T21:24:56Z

I don't see this as a fix

Jalle19 · 2015-04-28T07:30:38Z

@MartijnKaijser why not? We have always supported multiple clients, it just hasn't ever really worked that well so we have always adviced people to not do it.

xhaggi · 2015-04-28T07:36:51Z

totally agree with @Jalle19, it's a feature since the beginning but it didn't work in some cases. This will fix it and therefore it's a fix ;)

xhaggi · 2015-04-28T08:24:09Z

@Jalle19 i would say let's shove it in for Beta1 so we have a wide range of testings till our final release.

jenkins build this please.

Jalle19 · 2015-04-28T08:39:53Z

I agree, any breakage should be easily spotted.

ksooo · 2015-04-28T09:35:13Z

👍 for inclusion in beta
👍 for being a fix

xhaggi · 2015-04-29T08:00:23Z

any objections, if not i'll merge it in later today.

FernetMenta · 2015-04-29T11:41:31Z

i am traveling and would like to have a closer look at this.

opdenkamp · 2015-04-29T11:54:01Z

and I've been out of the loop for a couple of days because I was ill and
haven't looked at it yet either.

On 29-04-15 13:41, Rainer Hochecker wrote:

i am traveling and would like to have a closer look at this.

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

xbmc/pvr/addons/PVRClients.cpp

+  // indicate that addons have now been initialised
+  {
+    CSingleLock lock(m_critSection);
+    m_bAddonsInitialised = true;


Jalle19 · 2015-04-30T08:23:39Z

@FernetMenta I changed it to use an event instead and removed the superfluous boolean. Tested by injecting sleeps here and there and it seems to work just fine.

xbmc/pvr/addons/PVRClients.cpp

  return bReturn;
 }

+void CPVRClients::WaitForInitialisation()
+{
+  m_loadedEvent.Wait();


opdenkamp · 2015-04-30T10:37:09Z

it's not.

On 30-04-15 12:32, freddysmith1 wrote:

Any sort of fix is better then no fix at all

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

Jalle19 · 2015-04-30T10:53:18Z

why is this so urgent that it needs a half fix that may or may not be fixed properly at some point? you're removing the "one add-on loaded" code now, which was there on purpose, which used to work a couple of releases back and which was broken while refactoring a while ago

It has been broken for over two years and I don't see anyone else fixing it.

unless there's a very good reason to not fix this properly now (shouldn't be that much work)

Do you have any suggestions on how this should be done properly? Either way I've updated the PR to wait a maximum of 5 seconds instead of forever.

Jalle19 · 2015-05-04T07:18:34Z

What's the verdict on this?

xbmc/pvr/addons/PVRClients.cpp

@@ -47,6 +47,8 @@ using namespace EPG;
 /** sleep time in milliseconds when no auto-configured add-ons were found */
 #define PVR_CLIENT_AVAHI_SLEEP_TIME_MS     (250)

+const int CPVRClients::CLIENT_LOAD_TIMEOUT = 5000;


opdenkamp · 2015-05-04T07:30:31Z

we should not wait for all clients to be connected, as explained earlier. that's why the code was structured like this.

it should just return from the method when 1 add-on connected, and the updater thread for the clients should keep trying to connect to the other add-ons (if any) while they're not all connected.

Jalle19 · 2015-05-04T09:21:42Z

There's much work to be done to implement it the way you're suggesting:

a) m_channelGroups->Load() is only called once (when the manager starts), if all clients are not loaded by then their "content" will not be loaded regardless of whether the client gets loaded eventually.

b) once the channel group container has started to load it will search the database and find all channels from the client that didn't start on time and delete them. This can take a lot of time. Same thing happens with the EPG data.

c) even if we'd change the code so that the channel group container etc. is reloaded when a new client is connected there'd be a ton of pointless database operations in between.

I don't see why we can't go with the current solution for now, the absolutely worst case scenario is that the "Loading channels from clients" dialog is shown for five seconds during startup, alternatively during shutdown if the user quits Kodi as soon as possible after launching it.

edit: typo

Jalle19 · 2015-05-04T10:23:30Z

@FernetMenta I updated the event handling, is this how you meant?

opdenkamp · 2015-05-23T11:43:56Z

The main advantage of having multiple pvr clients imo is to be able to switch between backends easily. All the rest can be consolidated into 1 backend. The code originally did handle this correctly a couple of years ago, but it broke with all the refactoring. Rather than doing it like this, and removing the parts of the code that were structured to handle it properly in the process, the points you mentioned should be fixed. It's one of the main reasons for having the updater threads in pvr...

Jalle19 · 2015-05-23T14:59:26Z

@opdenkamp since you're pretty much the only one who is familiar with how it should work (I didn't use PVR many years ago), any chance you might look into it at some point?

Jalle19 · 2015-07-01T12:55:39Z

@opdenkamp while thinking about this again, I realized that the way you want it to work will make the startup code incredibly complicated. Think of this scenario:

The user normally has two addons enabled so he has channels and guide data from both addons.
Start Kodi, and only one addon gets started "in time".
All the channels and guide data from the "stale" addon is removed. This actually takes quite some time, especially the guide data.
Suddenly the second addon wakes up and starts to add data, all the while the data is still being deleted. This sounds like a recipe for a disaster.

xbmc/pvr/PVRManager.cpp

+  {
+    CLog::Log(LOGINFO, "PVRManager - %s - %d of %d enabled clients connected in time, continue to start", 
+      __FUNCTION__, numConnectedClients, numEnabledClients);
+  }


Jalle19 · 2016-01-25T13:57:33Z

Got some fresh complaints about us never fixing this bug (http://trac.kodi.tv/ticket/14498), any chance we could just show this if I fix the outstanding comments (not the architectural ones)?

least tried to load) all enabled addons. This fixes the ancient bug where it's unreliable to use more than one PVR client simultaneously. Closes xbmc#14498

Jalle19 · 2016-01-25T14:09:17Z

Added some fixups, squashed the last [squash] commit.

jenkins build this please

Jalle19 · 2016-08-25T07:24:26Z

Supposed to be obsolete

zag2me · 2016-08-25T10:13:53Z

Is this possible to do yet? I'm using DVBS-2 and now have IPTV also, I'm happy to write a guide if it does work....

ksooo · 2016-08-25T10:27:46Z

Is this possible to do yet? I'm using DVBS-2 and now have IPTV also, I'm happy to write a guide if it does work....

Not supported, works only by luck if at all.

To realize your use case, I'd route iptv through tvheadend.

Jalle19 · 2016-08-25T10:28:34Z

Just enable more than one PVR client

ksooo · 2016-08-25T10:29:50Z

Just enable more than one PVR client

... which does not work reliably as we know. No?

Jalle19 · 2016-08-25T14:52:23Z

I thought it was fixed now that the manager is started asynchronously?

ksooo · 2016-08-25T14:55:15Z

@FernetMenta ^^^ ?

Jalle19 added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality Component: PVR v15 Isengard labels Apr 27, 2015

xhaggi added this to the Isengard 15.0-beta1 milestone Apr 28, 2015

FernetMenta reviewed Apr 29, 2015
View reviewed changes

FernetMenta reviewed Apr 30, 2015
View reviewed changes

xbmc/pvr/addons/PVRClients.cpp

return bReturn;

}

void CPVRClients::WaitForInitialisation()

{

m_loadedEvent.Wait();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Jalle19 force-pushed the multiple-pvr-clients branch from e00ddd2 to d593e8c Compare April 30, 2015 10:48

xbmc locked and limited conversation to collaborators Apr 30, 2015

MartijnKaijser modified the milestone: Isengard 15.0-beta1 May 1, 2015

opdenkamp reviewed May 4, 2015
View reviewed changes

xbmc/pvr/addons/PVRClients.cpp

@@ -47,6 +47,8 @@ using namespace EPG;

/** sleep time in milliseconds when no auto-configured add-ons were found */

#define PVR_CLIENT_AVAHI_SLEEP_TIME_MS (250)

const int CPVRClients::CLIENT_LOAD_TIMEOUT = 5000;

This comment was marked as spam.

Sign in to view

MartijnKaijser removed the v15 Isengard label Jun 12, 2015

AlwinEsch reviewed Nov 3, 2015
View reviewed changes

xbmc/pvr/PVRManager.cpp

{

CLog::Log(LOGINFO, "PVRManager - %s - %d of %d enabled clients connected in time, continue to start",

__FUNCTION__, numConnectedClients, numEnabledClients);

}

This comment was marked as spam.

Sign in to view

Jalle19 added 4 commits January 25, 2016 16:07

[cosmetics] only check m_addons for NULL once

59e12f8

[pvr] remove bInitialiseAllClients boolean parameter

db3f51d

[pvr] add a way for CPVRClients to signal that it has loaded (or at

22d9139

least tried to load) all enabled addons. This fixes the ancient bug where it's unreliable to use more than one PVR client simultaneously. Closes xbmc#14498

[pvr] improve logging of client load statuses during startup

552e0b5

Jalle19 force-pushed the multiple-pvr-clients branch from 0188f3e to 552e0b5 Compare January 25, 2016 14:08

Jalle19 closed this Aug 25, 2016

MartijnKaijser modified the milestone: Abandoned, obsolete or superseeded Sep 2, 2016

[pvr] finally make it possible to use multiple PVR clients #7020

[pvr] finally make it possible to use multiple PVR clients #7020

Conversation

Jalle19 commented Apr 27, 2015

xhaggi commented Apr 27, 2015

ksooo commented Apr 27, 2015

Jalle19 commented Apr 27, 2015

xhaggi commented Apr 27, 2015

Jalle19 commented Apr 27, 2015

MartijnKaijser commented Apr 27, 2015

Jalle19 commented Apr 28, 2015

xhaggi commented Apr 28, 2015

xhaggi commented Apr 28, 2015

Jalle19 commented Apr 28, 2015

ksooo commented Apr 28, 2015

xhaggi commented Apr 29, 2015

FernetMenta commented Apr 29, 2015

opdenkamp commented Apr 29, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Jalle19 commented Apr 30, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

opdenkamp commented Apr 30, 2015

Jalle19 commented Apr 30, 2015

Jalle19 commented May 4, 2015

This comment was marked as spam.

opdenkamp commented May 4, 2015

Jalle19 commented May 4, 2015

Jalle19 commented May 4, 2015

opdenkamp commented May 23, 2015

Jalle19 commented May 23, 2015

Jalle19 commented Jul 1, 2015

This comment was marked as spam.

Jalle19 commented Jan 25, 2016

Jalle19 commented Jan 25, 2016

Jalle19 commented Aug 25, 2016

zag2me commented Aug 25, 2016

ksooo commented Aug 25, 2016

Jalle19 commented Aug 25, 2016

ksooo commented Aug 25, 2016

Jalle19 commented Aug 25, 2016

ksooo commented Aug 25, 2016