[vdr] Re: vdr-1.3.17: thread issues with vdr-xine and vdr-osdteletext

[Reinhard Nissl <rnissl@gmx.de>]

Klaus Schmidinger wrote:
> Reinhard Nissl wrote:
>
> > Hi,
> >
> > Reinhard Nissl wrote:
> >
> > > > > after applying all osdteletext patches and poisening VDR there 
> > > > > still seems to remain a race condtion when havily zapping between 
> > > > > channels.
> > > > >
> > > > >  From /var/log/messages:
> >
> >
> > ...
> >
> > > > > At this point VDR crashed. Strange is that there are two receivers 
> > > > > (1417 , 1419) and two TS buffers (1420, 1421) but only a single 
> > > > > transfer thread (1416).
> > > >
> > > >
> > > > My guess would be that the osdteletext uses the other receiver.
> > >
> > >
> > > Would you expect that there are two receivers at that time?
> >
> >
> > Damn words! The Thread description you choose was leading me into the 
> > wrong direction. Actually it's a "cDevice" that is started twice!
> >
> >  From device.c: --------------------------------------------
> >
> > cDevice::cDevice(void)
> > {
> >   cardIndex = nextCardIndex++;
> >
> >   SetDescription("receiver on device %d", CardIndex() + 1);
> >
> > -----------------------------------------------------------
> >
> > > I don't think that two receivers are necessary as I've added debug 
> > > output to osdteletext's Receive() function and it gets also called 
> > > when there is only one receiver that typically feeds the transfer 
> > > thread.
> >
> >
> > You'll definitely need to cReceiver instances: one for transfer thread 
> > and one for teletext.
> >
> > > BTW: I've meanwhile realized that each time VDR crashes there were 
> > > two receivers created.
> >
> >
> > The race condition is in cDevice::AttachReceiver() and or 
> > cThread::Start():
> >
> > bool cDevice::AttachReceiver(cReceiver *Receiver)
> > {
> >   if (!Receiver)
> >      return false;
> >   if (Receiver->device == this)
> >      return true;
> >   if (!HasLock(TUNER_LOCK_TIMEOUT)) {
> >      esyslog("ERROR: device %d has no lock, can't attach receiver!", 
> > CardIndex() + 1);
> >      return false;
> >      }
> >   for (int i = 0; i < MAXRECEIVERS; i++) {
> >       if (!receiver[i]) {
> >          for (int n = 0; n < MAXRECEIVEPIDS; n++) {
> >              if (!AddPid(Receiver->pids[n])) {
> >                 for ( ; n-- > 0; )
> >                     DelPid(Receiver->pids[n]);
> >                 return false;
> >                 }
> >              }
> >          Receiver->Activate(true);
> >          Lock();
> >          Receiver->device = this;
> >          receiver[i] = Receiver;
> >          Unlock();
> >          Start();    // <================
> >          return true;
> >          }
> >       }
> >   esyslog("ERROR: no free receiver slot!");
> >   return false;
> > }
> >
> > The above marked Start() is called for adding each receiver. It now 
> > depends on the time between two of these calls, whether one or two 
> > threads are created. From thread.c:
> >
> > bool cThread::Start(void)
> > {
> >   if (!childTid) {        // <================// <================
> >      parentTid = pthread_self();
> >      pthread_t Tid;
> >      pthread_create(&Tid, NULL, (void *(*) (void *))&StartThread, 
> > (void *)this);
> >      pthread_detach(Tid); // auto-reap
> >      pthread_setschedparam(Tid, SCHED_RR, 0);
> >      }
> >   return true; //XXX return value of pthread_create()???
> > }
> >
> > If the time is too short then childTid will be 0 for both calls 
> > resulting in two threads where one is expected.
> Well, looks like I should never have changed anything in thread.c (1.3.16),
> because back then everything actually did work... :-(
Attached you'll find a suggested fix. It works for me here.

Bye.
--
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
--- /data/download/thread.c	2004-12-02 22:30:16.000000000 +0100
+++ thread.c	2004-12-05 20:38:12.000000000 +0100
@@ -223,6 +223,7 @@ void *cThread::StartThread(cThread *Thre
 {
   Thread->childTidMutex.Lock();
   Thread->childTid = pthread_self();
+  Thread->childTidChanged.Broadcast();
   Thread->childTidMutex.Unlock();
   if (Thread->description)
      dsyslog("%s thread started (pid=%d, tid=%ld)", Thread->description, getpid(), Thread->childTid);
@@ -231,18 +232,23 @@ void *cThread::StartThread(cThread *Thre
      dsyslog("%s thread ended (pid=%d, tid=%ld)", Thread->description, getpid(), Thread->childTid);
   Thread->childTidMutex.Lock();
   Thread->childTid = 0;
+  Thread->childTidChanged.Broadcast();
   Thread->childTidMutex.Unlock();
   return NULL;
 }
 
 bool cThread::Start(void)
 {
+  cMutexLock startLock(&startMutex);
   if (!childTid) {
      parentTid = pthread_self();
      pthread_t Tid;
      pthread_create(&Tid, NULL, (void *(*) (void *))&StartThread, (void *)this);
      pthread_detach(Tid); // auto-reap
      pthread_setschedparam(Tid, SCHED_RR, 0);
+     cMutexLock childTidLock(&childTidMutex);
+     if (!childTid)
+       childTidChanged.Wait(childTidMutex);
      }
   return true; //XXX return value of pthread_create()???
 }
--- /data/download/thread.h	2004-12-02 22:30:27.000000000 +0100
+++ thread.h	2004-12-05 20:36:56.208615091 +0100
@@ -75,7 +75,8 @@ class cThread {
 private:
   pthread_t parentTid, childTid;
   cMutex childTidMutex;
-  cMutex mutex;
+  cCondVar childTidChanged;
+  cMutex mutex, startMutex;
   char *description;
   static bool emergencyExitRequested;
   static void *StartThread(cThread *Thread);