In order to fix a possible race condition in cTransfer I have centralized the 'active' handling of cThread. Since this is a rather sensitive area of the program, I'd like to invite you all to take a look at the attached patch against VDR version 1.3.28 and let me know if you're running into any trouble with it.
Here's the HISTORY entry I made for this change:
Fixed a race condition in cTransfer (thanks to Klaus ??? for reporting this one). In doing so, the 'active' variables used by the actual derived cThread classes have been replaced by the cThread::Active() function. The previous functionality of cThread::Active() has been moved into the new cThread::Running(). Plugin authors should check their derived cThread classes and replace any 'active' variables the same way as, for instance, done in transfer.c.
Klaus
=================================================================== RCS file: ./RCS/cutter.c retrieving revision 1.8 diff -u -r1.8 ./cutter.c --- ./cutter.c 2005/05/15 14:21:08 1.8 +++ ./cutter.c 2005/08/13 11:49:02 @@ -18,7 +18,6 @@ class cCuttingThread : public cThread { private: const char *error; - bool active; int fromFile, toFile; cFileName *fromFileName, *toFileName; cIndexFile *fromIndex, *toIndex; @@ -35,7 +34,6 @@ :cThread("video cutting") { error = NULL; - active = false; fromFile = toFile = -1; fromFileName = toFileName = NULL; fromIndex = toIndex = NULL; @@ -53,7 +51,6 @@
cCuttingThread::~cCuttingThread() { - active = false; Cancel(3); delete fromFileName; delete toFileName; @@ -67,7 +64,8 @@ if (Mark) { fromFile = fromFileName->Open(); toFile = toFileName->Open(); - active = fromFile >= 0 && toFile >= 0; + if (fromFile < 0 || toFile < 0) + return; int Index = Mark->position; Mark = fromMarks.Next(Mark); int FileSize = 0; @@ -78,7 +76,7 @@ uchar buffer[MAXFRAMESIZE]; bool LastMark = false; bool cutIn = true; - while (active) { + while (Active()) { uchar FileNumber; int FileOffset, Length; uchar PictureType; @@ -215,7 +213,7 @@
void cCutter::Stop(void) { - bool Interrupted = cuttingThread && cuttingThread->Active(); + bool Interrupted = cuttingThread && cuttingThread->Running(); const char *Error = cuttingThread ? cuttingThread->Error() : NULL; delete cuttingThread; cuttingThread = NULL; @@ -232,7 +230,7 @@ bool cCutter::Active(void) { if (cuttingThread) { - if (cuttingThread->Active()) + if (cuttingThread->Running()) return true; error = cuttingThread->Error(); Stop(); =================================================================== RCS file: ./RCS/cutter.h retrieving revision 1.1 diff -u -r1.1 ./cutter.h =================================================================== RCS file: ./RCS/device.c retrieving revision 1.103 diff -u -r1.103 ./device.c --- ./device.c 2005/06/12 13:39:11 1.103 +++ ./device.c 2005/08/13 11:44:06 @@ -156,8 +156,6 @@
SetVideoFormat(Setup.VideoFormat);
- active = false; - mute = false; volume = Setup.CurrentVolume;
@@ -1126,25 +1124,25 @@
void cDevice::Action(void) { - if (active && OpenDvr()) { - for (; active;) { - // Read data from the DVR device: - uchar *b = NULL; - if (GetTSPacket(b)) { - if (b) { - int Pid = (((uint16_t)b[1] & PID_MASK_HI) << 8) | b[2]; - // Distribute the packet to all attached receivers: - Lock(); - for (int i = 0; i < MAXRECEIVERS; i++) { - if (receiver[i] && receiver[i]->WantsPid(Pid)) - receiver[i]->Receive(b, TS_SIZE); - } - Unlock(); - } - } - else - break; - } + if (Active() && OpenDvr()) { + while (Active()) { + // Read data from the DVR device: + uchar *b = NULL; + if (GetTSPacket(b)) { + if (b) { + int Pid = (((uint16_t)b[1] & PID_MASK_HI) << 8) | b[2]; + // Distribute the packet to all attached receivers: + Lock(); + for (int i = 0; i < MAXRECEIVERS; i++) { + if (receiver[i] && receiver[i]->WantsPid(Pid)) + receiver[i]->Receive(b, TS_SIZE); + } + Unlock(); + } + } + else + break; + } CloseDvr(); } } @@ -1188,10 +1186,8 @@ Receiver->device = this; receiver[i] = Receiver; Unlock(); - if (!active) { - active = true; + if (!Active()) Start(); - } return true; } } @@ -1218,10 +1214,8 @@ else if (receiver[i]) receiversLeft = true; } - if (!receiversLeft) { - active = false; + if (!receiversLeft) Cancel(3); - } }
void cDevice::DetachAll(int Pid) @@ -1246,13 +1240,11 @@ delivered = false; ringBuffer = new cRingBufferLinear(Size, TS_SIZE, true, "TS"); ringBuffer->SetTimeouts(100, 100); - active = true; Start(); }
cTSBuffer::~cTSBuffer() { - active = false; Cancel(3); delete ringBuffer; } @@ -1262,20 +1254,20 @@ if (ringBuffer) { bool firstRead = true; cPoller Poller(f); - for (; active;) { - if (firstRead || Poller.Poll(100)) { - firstRead = false; - int r = ringBuffer->Read(f); - if (r < 0 && FATALERRNO) { - if (errno == EOVERFLOW) - esyslog("ERROR: driver buffer overflow on device %d", cardIndex); - else { - LOG_ERROR; - break; - } - } - } - } + while (Active()) { + if (firstRead || Poller.Poll(100)) { + firstRead = false; + int r = ringBuffer->Read(f); + if (r < 0 && FATALERRNO) { + if (errno == EOVERFLOW) + esyslog("ERROR: driver buffer overflow on device %d", cardIndex); + else { + LOG_ERROR; + break; + } + } + } + } } }
=================================================================== RCS file: ./RCS/device.h retrieving revision 1.60 diff -u -r1.60 ./device.h --- ./device.h 2005/07/30 09:31:53 1.60 +++ ./device.h 2005/08/13 11:44:13 @@ -233,7 +233,6 @@ // PID handle facilities
private: - bool active; virtual void Action(void); protected: enum ePidType { ptAudio, ptVideo, ptPcr, ptTeletext, ptDolby, ptOther }; @@ -518,7 +517,6 @@ private: int f; int cardIndex; - bool active; bool delivered; cRingBufferLinear *ringBuffer; virtual void Action(void); =================================================================== RCS file: ./RCS/dvbdevice.c retrieving revision 1.131 diff -u -r1.131 ./dvbdevice.c --- ./dvbdevice.c 2005/06/19 11:00:43 1.131 +++ ./dvbdevice.c 2005/08/13 11:40:46 @@ -76,7 +76,6 @@ cCiHandler *ciHandler; cChannel channel; const char *diseqcCommands; - bool active; bool useCa; time_t startTime; eTunerStatus tunerStatus; @@ -101,7 +100,6 @@ frontendType = FrontendType; ciHandler = CiHandler; diseqcCommands = NULL; - active = false; useCa = false; tunerStatus = tsIdle; startTime = time(NULL); @@ -113,7 +111,6 @@
cDvbTuner::~cDvbTuner() { - active = false; tunerStatus = tsIdle; newSet.Signal(); Cancel(3); @@ -294,8 +291,7 @@ void cDvbTuner::Action(void) { dvb_frontend_event event; - active = true; - while (active) { + while (Active()) { Lock(); if (tunerStatus == tsSet) { while (GetFrontendEvent(event)) =================================================================== RCS file: ./RCS/dvbplayer.c retrieving revision 1.36 diff -u -r1.36 ./dvbplayer.c --- ./dvbplayer.c 2005/07/30 10:00:24 1.36 +++ ./dvbplayer.c 2005/08/13 12:27:17 @@ -79,7 +79,6 @@ int wanted; int length; bool hasData; - bool active; cCondWait newSet; protected: void Action(void); @@ -98,13 +97,11 @@ buffer = NULL; wanted = length = 0; hasData = false; - active = false; Start(); }
cNonBlockingFileReader::~cNonBlockingFileReader() { - active = false; newSet.Signal(); Cancel(3); free(buffer); @@ -147,8 +144,7 @@
void cNonBlockingFileReader::Action(void) { - active = true; - while (active) { + while (Active()) { Lock(); if (!hasData && f >= 0 && buffer) { int r = safe_read(f, buffer + length, wanted - length); @@ -187,8 +183,6 @@ cIndexFile *index; int replayFile; bool eof; - bool active; - bool running; bool firstPacket; ePlayModes playMode; ePlayDirs playDir; @@ -207,7 +201,7 @@ public: cDvbPlayer(const char *FileName); virtual ~cDvbPlayer(); - bool Active(void) { return active; } + bool Active(void) { return cThread::Active(); } void Pause(void); void Play(void); void Forward(void); @@ -233,8 +227,6 @@ backTrace = NULL; index = NULL; eof = false; - active = true; - running = false; firstPacket = true; playMode = pmPlay; playDir = pdForward; @@ -353,11 +345,8 @@ if (replayFile >= 0) Start(); } - else if (active) { - running = false; + else Cancel(9); - active = false; - } }
void cDvbPlayer::Action(void) @@ -374,8 +363,7 @@ int Length = 0; bool Sleep = false;
- running = true; - while (running && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { + while (Active() && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { if (Sleep) { cCondWait::SleepMs(3); // this keeps the CPU load low Sleep = false; @@ -501,7 +489,6 @@ Sleep = true; } } - active = running = false;
cNonBlockingFileReader *nbfr = nonBlockingFileReader; nonBlockingFileReader = NULL; =================================================================== RCS file: ./RCS/dvbplayer.h retrieving revision 1.2 diff -u -r1.2 ./dvbplayer.h =================================================================== RCS file: ./RCS/recorder.c retrieving revision 1.13 diff -u -r1.13 ./recorder.c --- ./recorder.c 2005/01/16 12:53:17 1.13 +++ ./recorder.c 2005/08/13 11:33:35 @@ -29,7 +29,6 @@ uchar pictureType; int fileSize; int recordFile; - bool active; time_t lastDiskSpaceCheck; bool RunningLowOnDiskSpace(void); bool NextFile(void); @@ -43,7 +42,6 @@ cFileWriter::cFileWriter(const char *FileName, cRemux *Remux) :cThread("file writer") { - active = false; fileName = NULL; remux = Remux; index = NULL; @@ -63,7 +61,6 @@
cFileWriter::~cFileWriter() { - active = false; Cancel(3); delete index; delete fileName; @@ -96,13 +93,11 @@ void cFileWriter::Action(void) { time_t t = time(NULL); - active = true; - while (active) { + while (Active()) { int Count; uchar *p = remux->Get(Count, &pictureType); if (p) { - //XXX+ active??? see old version (Busy) - if (!active && pictureType == I_FRAME) // finish the recording before the next 'I' frame + if (!Active() && pictureType == I_FRAME) // finish the recording before the next 'I' frame break; if (NextFile()) { if (index && pictureType != NO_PICTURE) @@ -124,15 +119,12 @@ t = time(NULL); } } - active = false; }
cRecorder::cRecorder(const char *FileName, int Ca, int Priority, int VPid, const int *APids, const int *DPids, const int *SPids) :cReceiver(Ca, Priority, VPid, APids, Setup.UseDolbyDigital ? DPids : NULL, SPids) ,cThread("recording") { - active = false; - // Make sure the disk is up and running:
SpinUpDisk(FileName); @@ -157,25 +149,22 @@ writer->Start(); Start(); } - else if (active) { - active = false; + else Cancel(3); - } }
void cRecorder::Receive(uchar *Data, int Length) { - if (active) { + if (Active()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && active) + if (p != Length && Active()) ringBuffer->ReportOverflow(Length - p); } }
void cRecorder::Action(void) { - active = true; - while (active) { + while (Active()) { int r; uchar *b = ringBuffer->Get(r); if (b) { =================================================================== RCS file: ./RCS/recorder.h retrieving revision 1.3 diff -u -r1.3 ./recorder.h --- ./recorder.h 2005/01/15 16:35:53 1.3 +++ ./recorder.h 2005/08/13 11:31:18 @@ -23,7 +23,6 @@ cRingBufferLinear *ringBuffer; cRemux *remux; cFileWriter *writer; - bool active; protected: virtual void Activate(bool On); virtual void Receive(uchar *Data, int Length); =================================================================== RCS file: ./RCS/remote.c retrieving revision 1.42 diff -u -r1.42 ./remote.c --- ./remote.c 2005/03/20 13:25:31 1.42 +++ ./remote.c 2005/08/13 11:28:35 @@ -213,7 +213,6 @@ :cRemote("KBD") ,cThread("KBD remote control") { - active = false; tcgetattr(STDIN_FILENO, &savedTm); struct termios tm; if (tcgetattr(STDIN_FILENO, &tm) == 0) { @@ -230,7 +229,6 @@ cKbdRemote::~cKbdRemote() { kbdAvailable = false; - active = false; Cancel(3); tcsetattr(STDIN_FILENO, TCSANOW, &savedTm); } @@ -261,12 +259,11 @@ void cKbdRemote::Action(void) { cPoller Poller(STDIN_FILENO); - active = true; - while (active) { + while (Active()) { if (Poller.Poll(100)) { uint64 Command = 0; uint i = 0; - while (active && i < sizeof(Command)) { + while (Active() && i < sizeof(Command)) { uchar ch; int r = read(STDIN_FILENO, &ch, 1); if (r == 1) { =================================================================== RCS file: ./RCS/remote.h retrieving revision 1.29 diff -u -r1.29 ./remote.h --- ./remote.h 2004/05/28 14:14:02 1.29 +++ ./remote.h 2005/08/13 11:28:10 @@ -81,7 +81,6 @@
class cKbdRemote : public cRemote, private cThread { private: - bool active; static bool kbdAvailable; static bool rawMode; struct termios savedTm; =================================================================== RCS file: ./RCS/sections.c retrieving revision 1.11 diff -u -r1.11 ./sections.c --- ./sections.c 2005/05/29 11:43:17 1.11 +++ ./sections.c 2005/08/13 11:25:04 @@ -44,7 +44,6 @@ { shp = new cSectionHandlerPrivate; device = Device; - active = false; statusCount = 0; on = false; waitForLock = false; @@ -54,7 +53,6 @@
cSectionHandler::~cSectionHandler() { - active = false; Cancel(3); cFilter *fi; while ((fi = filters.First()) != NULL) @@ -166,9 +164,8 @@
void cSectionHandler::Action(void) { - active = true; SetPriority(19); - while (active) { + while (Active()) {
Lock(); if (waitForLock) =================================================================== RCS file: ./RCS/sections.h retrieving revision 1.4 diff -u -r1.4 ./sections.h --- ./sections.h 2004/08/08 13:44:17 1.4 +++ ./sections.h 2005/08/13 11:23:55 @@ -25,7 +25,6 @@ private: cSectionHandlerPrivate *shp; cDevice *device; - bool active; int statusCount; bool on, waitForLock; time_t lastIncompleteSection; =================================================================== RCS file: ./RCS/thread.c retrieving revision 1.43 diff -u -r1.43 ./thread.c --- ./thread.c 2005/05/29 11:40:30 1.43 +++ ./thread.c 2005/08/13 11:22:37 @@ -197,7 +197,7 @@
cThread::cThread(const char *Description) { - running = false; + running = active = false; childTid = 0; description = NULL; SetDescription(Description); @@ -205,6 +205,7 @@
cThread::~cThread() { + Cancel(); // just in case the derived class didn't call it free(description); }
@@ -233,6 +234,7 @@ Thread->Action(); if (Thread->description) dsyslog("%s thread ended (pid=%d, tid=%ld)", Thread->description, getpid(), pthread_self()); + Thread->active = false; Thread->running = false; return NULL; } @@ -240,21 +242,21 @@ bool cThread::Start(void) { if (!running) { - running = true; + running = active = true; if (pthread_create(&childTid, NULL, (void *(*) (void *))&StartThread, (void *)this) == 0) { pthread_detach(childTid); // auto-reap pthread_setschedparam(childTid, SCHED_RR, 0); } else { LOG_ERROR; - running = false; + running = active = false; return false; } } return true; }
-bool cThread::Active(void) +bool cThread::Running(void) { if (running) { // @@ -271,7 +273,7 @@ if (err != ESRCH) LOG_ERROR; childTid = 0; - running = false; + running = active = false; } else return true; @@ -281,10 +283,11 @@
void cThread::Cancel(int WaitSeconds) { + active = false; if (running) { if (WaitSeconds > 0) { for (time_t t0 = time(NULL) + WaitSeconds; time(NULL) < t0; ) { - if (!Active()) + if (!Running()) return; cCondWait::SleepMs(10); } =================================================================== RCS file: ./RCS/thread.h retrieving revision 1.28 diff -u -r1.28 ./thread.h --- ./thread.h 2005/05/29 11:31:24 1.28 +++ ./thread.h 2005/08/13 13:01:33 @@ -76,6 +76,7 @@ friend class cThreadLock; private: bool running; + bool active; pthread_t childTid; cMutex mutex; char *description; @@ -86,13 +87,30 @@ void Lock(void) { mutex.Lock(); } void Unlock(void) { mutex.Unlock(); } virtual void Action(void) = 0; + ///< A derived cThread class must implement the code it wants to + ///< execute as a separate thread in this function. If this is + ///< a loop, it must check Active() repeatedly to see whether + ///< it's time to stop. + bool Active(void) { return active; } + ///< Returns false if a derived cThread object shall leave its Action() + ///< function. void Cancel(int WaitSeconds = 0); + ///< Cancels the thread by first setting 'active' to false, so that + ///< the Action() loop can finish in an orderly fashion and then waiting + ///< up to WaitSeconds seconds for the thread to actually end. If the + ///< thread doesn't end by itself, it is killed. public: cThread(const char *Description = NULL); + ///< Creates a new thread. + ///< If Description is present, a log file entry will be made when + ///< the thread starts and stops. The Start() function must be called + ///< to actually start the thread. virtual ~cThread(); void SetDescription(const char *Description, ...); bool Start(void); - bool Active(void); + ///< Actually starts the thread. + bool Running(void); + ///< Checks whether the thread is actually running. static bool EmergencyExit(bool Request = false); };
=================================================================== RCS file: ./RCS/transfer.c retrieving revision 1.28 diff -u -r1.28 ./transfer.c --- ./transfer.c 2005/02/19 14:38:55 1.28 +++ ./transfer.c 2005/08/13 11:19:46 @@ -21,7 +21,6 @@ ringBuffer = new cRingBufferLinear(TRANSFERBUFSIZE, TS_SIZE * 2, true, "Transfer"); remux = new cRemux(VPid, APids, Setup.UseDolbyDigital ? DPids : NULL, SPids); needsBufferReserve = Setup.UseDolbyDigital && VPid != 0 && DPids && DPids[0] != 0; - active = false; }
cTransfer::~cTransfer() @@ -34,21 +33,17 @@
void cTransfer::Activate(bool On) { - if (On) { - if (!active) - Start(); - } - else if (active) { - active = false; + if (On) + Start(); + else Cancel(3); - } }
void cTransfer::Receive(uchar *Data, int Length) { - if (IsAttached() && active) { + if (IsAttached() && Active()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && active) + if (p != Length && Active()) ringBuffer->ReportOverflow(Length - p); return; } @@ -70,8 +65,7 @@ bool GotBufferReserve = false; int RequiredBufferReserve = KILOBYTE(DvbCardWith4MBofSDRAM ? 288 : 576); #endif - active = true; - while (active) { + while (Active()) { #ifdef FW_NEEDS_BUFFER_RESERVE_FOR_AC3 if (needsBufferReserve && !GotBufferReserve) { //XXX For dolby we've to fill the buffer because the firmware does @@ -145,7 +139,6 @@ } } } - active = false; }
// --- cTransferControl ------------------------------------------------------ =================================================================== RCS file: ./RCS/transfer.h retrieving revision 1.9 diff -u -r1.9 ./transfer.h --- ./transfer.h 2005/01/15 16:39:39 1.9 +++ ./transfer.h 2005/08/13 10:16:02 @@ -21,7 +21,6 @@ cRingBufferLinear *ringBuffer; cRemux *remux; bool needsBufferReserve; - bool active; protected: virtual void Activate(bool On); virtual void Receive(uchar *Data, int Length);
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Hi,
Fixed a race condition in cTransfer (thanks to Klaus ??? for reporting this one). In doing so, the 'active' variables used by the actual derived cThread classes have been replaced by the cThread::Active() function. The previous functionality of cThread::Active() has been moved into the new cThread::Running(). Plugin authors should check their derived cThread classes and replace any 'active' variables the same way as, for instance, done in transfer.c.
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
This would limit the impact this change has to plugins.
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Regards.
Stefan Huelswitt wrote:
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
In that case I'd suggest Terminated()
while(!Terminated())
it would somewhat reduce my confusion when switching from the elegance of delphi/lazarus to the awkwardness of C++ ;-)
Bye
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Hi,
Fixed a race condition in cTransfer (thanks to Klaus ??? for reporting this one). In doing so, the 'active' variables used by the actual derived cThread classes have been replaced by the cThread::Active() function. The previous functionality of cThread::Active() has been moved into the new cThread::Running(). Plugin authors should check their derived cThread classes and replace any 'active' variables the same way as, for instance, done in transfer.c.
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
Well, Active() reflects the internal 'active' variable, so Continue() would have to correspond to 'continue', which is a reserved keyword...
This would limit the impact this change has to plugins.
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Is there any plugin that actually uses cThread::Active()?
Klaus
Luca Olivetti wrote:
Stefan Huelswitt wrote:
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
In that case I'd suggest Terminated()
while(!Terminated())
it would somewhat reduce my confusion when switching from the elegance of delphi/lazarus to the awkwardness of C++ ;-)
Why use an extra negation here? I think a positive check ('Active()') is more straightforward than a negative one ('!Terminated()').
Just wondering: what does this have to do with "elegance" vs. "awkwardness"?
Klaus
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Stefan Huelswitt wrote:
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Is there any plugin that actually uses cThread::Active()?
Both mp3 & mplayer plugin do.
To make it worse, they don't access the old "active" var, so I guess your changes won't make them fail compile.
Regards.
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Stefan Huelswitt wrote:
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
Well, Active() reflects the internal 'active' variable, so Continue() would have to correspond to 'continue', which is a reserved keyword...
This is after your change, but before Active() didn't reflected any internal variable. I don't know if one must build such a strict rule ...
Just my two cents.
Regards.
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Stefan Huelswitt wrote:
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Is there any plugin that actually uses cThread::Active()?
Both mp3 & mplayer plugin do.
And are you sure that this is _really_ necessary? Do they _really_ want to determine whether the thread is still running, or would they just like to know whether it's time to return from the Action() function? I don't know the internals of these plugins, therefore my question.
To make it worse, they don't access the old "active" var, so I guess your changes won't make them fail compile.
That variable was private to some derived cThread classes in VDR, so they couldn't have accessed them, anyway. I was referring to cases where plugins might have used an 'active' variable (or whatever they may have called it) in the same way VDR did.
Klaus
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Stefan Huelswitt wrote:
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
Well, Active() reflects the internal 'active' variable, so Continue() would have to correspond to 'continue', which is a reserved keyword...
This is after your change, but before Active() didn't reflected any internal variable. I don't know if one must build such a strict rule ...
Of course you don't have to, but it's nice ;-)
Klaus
Klaus Schmidinger wrote:
it would somewhat reduce my confusion when switching from the elegance of delphi/lazarus to the awkwardness of C++ ;-)
Why use an extra negation here? I think a positive check ('Active()') is more straightforward than a negative one ('!Terminated()').
It mostly depends on habit. The native delphi class for threads (and the corresponding one in freepascal), TThread, just uses Terminate instead of Cancel, and the corresponding property is Terminated, so I'm just used to wrap a thread in a "while not terminated".
Just wondering: what does this have to do with "elegance" vs. "awkwardness"?
Nothing, it was a tongue-in-cheek remark.
Bye
Klaus Schmidinger wrote:
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Hi,
Fixed a race condition in cTransfer (thanks to Klaus ??? for reporting this one). In doing so, the 'active' variables used by the actual derived cThread classes have been replaced by the cThread::Active() function. The previous functionality of cThread::Active() has been moved into the new cThread::Running(). Plugin authors should check their derived cThread classes and replace any 'active' variables the same way as, for instance, done in transfer.c.
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
Well, Active() reflects the internal 'active' variable, so Continue() would have to correspond to 'continue', which is a reserved keyword...
What has the name of a method to do with te name of a internal variable?
This would limit the impact this change has to plugins.
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Is there any plugin that actually uses cThread::Active()?
/In the vbox plugin i use some code from mp3 plugin and there i have a Active() call which simply returns active member of cTrhead.
IMO what stfean huelswitt says , is right.
regards, gunnar
/
Gunnar Roth wrote:
Klaus Schmidinger wrote:
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Hi,
Fixed a race condition in cTransfer (thanks to Klaus ??? for reporting this one). In doing so, the 'active' variables used by the actual derived cThread classes have been replaced by the cThread::Active() function. The previous functionality of cThread::Active() has been moved into the new cThread::Running(). Plugin authors should check their derived cThread classes and replace any 'active' variables the same way as, for instance, done in transfer.c.
Wouldn't it be better to leave Active() untouched (instead of renaming to Running()) and create a new (differently named) function for the active var replacement? (e.g. Continue(), which would give good readability with while(Continue()))
Well, Active() reflects the internal 'active' variable, so Continue() would have to correspond to 'continue', which is a reserved keyword...
What has the name of a method to do with te name of a internal variable?
Well, nothing, technically - it's just that I like to do it that way. Now, before you start dissecting VDR and showing me places where I did it differently, let's just end this here ;-)
This would limit the impact this change has to plugins.
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Is there any plugin that actually uses cThread::Active()?
/In the vbox plugin i use some code from mp3 plugin and there i have a Active() call which simply returns active member of cTrhead.
IMO what stfean huelswitt says , is right.
Ok, I give up. I'll leave the old Active() function as it was and call the new one something like Continue() or so. I won't call it Terminate() or anyting that requires an extra negation, though.
Now, with the naming problem settled, are there any actual, technical issues with this modification?
Klaus
Now, with the naming problem settled, are there any actual, technical issues with this modification?
A quick compile test (using the patch in this thread) and all my plugins build except for autotimeredit-0.1.6:
g++ -g -O2 -Wall -Woverloaded-virtual -c -DPLUGIN_NAME_I18N='"autotimeredit"' -D_GNU_SOURCE -I../../../include -I/usr/src/DVB/linux/include autotimeredit.c autotimeredit.c: In member function `virtual void cPluginAutoTimer::Housekeeping()': ../../../include/vdr/thread.h:94: error: `bool cThread::Active()' is protected autotimeredit.c:948: error: within this context make[1]: *** [autotimeredit.o] Error 1
Regards.
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Stefan Huelswitt wrote:
Otherwise a plugins using Active() now, could compile fine with the next VDR version, but certainly wouldn't work correctly.
Is there any plugin that actually uses cThread::Active()?
Both mp3 & mplayer plugin do.
And are you sure that this is _really_ necessary? Do they _really_ want to determine whether the thread is still running, or would they just like to know whether it's time to return from the Action() function? I don't know the internals of these plugins, therefore my question.
I dunno if it's necessary, but for me it's common practice in a player plugin. The derived cControl class calls player->Active() so see if the derived cPlayer class (which in most cases is a cThread as well) is still running. Like in:
eOSState cMP3Control::ProcessKey(eKeys Key) { if(!player->Active()) return osEnd; [...]
But there are other cases where the call is usefull.
And yes, if Active() is called, I want to know if the thread is still running, as this is the only purpose of this function (up to now).
You may argue, that I'm using (the old) Active() in an incorrect way, but nevertheless I think that any API change should be made in a way that existing plugins either continue to work properly or fail to compile. Anything else will lead to massive confusion for the average user.
To make it worse, they don't access the old "active" var, so I guess your changes won't make them fail compile.
That variable was private to some derived cThread classes in VDR, so they couldn't have accessed them, anyway. I was referring to cases where plugins might have used an 'active' variable (or whatever they may have called it) in the same way VDR did.
You're right. My plugins use an own variable but in a similiar way as the internal one.
Regards.
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Now, with the naming problem settled, are there any actual, technical issues with this modification?
*g*
If you could provide an updated patch, I'll go and test it.
Regards.
Stefan Huelswitt wrote:
You're right. My plugins use an own variable but in a similiar way as the internal one.
In fact if this is done right it will avoid a lot of duplicated code in derived classes.
Bye
C.Y.M wrote:
Now, with the naming problem settled, are there any actual, technical issues with this modification?
A quick compile test (using the patch in this thread) and all my plugins build except for autotimeredit-0.1.6:
g++ -g -O2 -Wall -Woverloaded-virtual -c -DPLUGIN_NAME_I18N='"autotimeredit"' -D_GNU_SOURCE -I../../../include -I/usr/src/DVB/linux/include autotimeredit.c autotimeredit.c: In member function `virtual void cPluginAutoTimer::Housekeeping()': ../../../include/vdr/thread.h:94: error: `bool cThread::Active()' is protected autotimeredit.c:948: error: within this context make[1]: *** [autotimeredit.o] Error 1
The attached patch is compile tested.
Regards,
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Now, with the naming problem settled, are there any actual, technical issues with this modification?
*g*
If you could provide an updated patch, I'll go and test it.
I'll provide one tomorrow. Have to think about the name some more. With Continue() we get some funny things like
if (!Continue()) Start();
while
if (!Active()) Start();
would have made sense ;-)
Klaus
Klaus Schmidinger wrote:
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Now, with the naming problem settled, are there any actual, technical issues with this modification?
*g*
If you could provide an updated patch, I'll go and test it.
I'll provide one tomorrow. Have to think about the name some more.
I dont seem to have any trouble with the first one (except that one patch I had to make for the autotimeredit plugin).
Best Regards,
Klaus Schmidinger wrote:
I'll provide one tomorrow. Have to think about the name some more. With Continue() we get some funny things like
if (!Continue()) Start();
while
if (!Active()) Start();
would have made sense ;-)
if (Terminated()) Start();
also does. Catch you ;-)
Anyway, Looping(), Running() (since it wasn't used before), Activated() (though easily confused with Active()), Execute()......
Bye
Stefan Huelswitt wrote:
On 13 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Now, with the naming problem settled, are there any actual, technical issues with this modification?
*g*
If you could provide an updated patch, I'll go and test it.
Ok, here it is. cThread::Active() is as it was before, and the new function is called cThread::Running() as suggested by Luca Olivetti.
Klaus
=================================================================== RCS file: ./RCS/cutter.c retrieving revision 1.8 diff -u -r1.8 ./cutter.c --- ./cutter.c 2005/05/15 14:21:08 1.8 +++ ./cutter.c 2005/08/14 10:51:54 @@ -18,7 +18,6 @@ class cCuttingThread : public cThread { private: const char *error; - bool active; int fromFile, toFile; cFileName *fromFileName, *toFileName; cIndexFile *fromIndex, *toIndex; @@ -35,7 +34,6 @@ :cThread("video cutting") { error = NULL; - active = false; fromFile = toFile = -1; fromFileName = toFileName = NULL; fromIndex = toIndex = NULL; @@ -53,7 +51,6 @@
cCuttingThread::~cCuttingThread() { - active = false; Cancel(3); delete fromFileName; delete toFileName; @@ -67,7 +64,8 @@ if (Mark) { fromFile = fromFileName->Open(); toFile = toFileName->Open(); - active = fromFile >= 0 && toFile >= 0; + if (fromFile < 0 || toFile < 0) + return; int Index = Mark->position; Mark = fromMarks.Next(Mark); int FileSize = 0; @@ -78,7 +76,7 @@ uchar buffer[MAXFRAMESIZE]; bool LastMark = false; bool cutIn = true; - while (active) { + while (Running()) { uchar FileNumber; int FileOffset, Length; uchar PictureType; =================================================================== RCS file: ./RCS/device.c retrieving revision 1.103 diff -u -r1.103 ./device.c --- ./device.c 2005/06/12 13:39:11 1.103 +++ ./device.c 2005/08/14 10:52:08 @@ -156,8 +156,6 @@
SetVideoFormat(Setup.VideoFormat);
- active = false; - mute = false; volume = Setup.CurrentVolume;
@@ -1126,25 +1124,25 @@
void cDevice::Action(void) { - if (active && OpenDvr()) { - for (; active;) { - // Read data from the DVR device: - uchar *b = NULL; - if (GetTSPacket(b)) { - if (b) { - int Pid = (((uint16_t)b[1] & PID_MASK_HI) << 8) | b[2]; - // Distribute the packet to all attached receivers: - Lock(); - for (int i = 0; i < MAXRECEIVERS; i++) { - if (receiver[i] && receiver[i]->WantsPid(Pid)) - receiver[i]->Receive(b, TS_SIZE); - } - Unlock(); - } - } - else - break; - } + if (Running() && OpenDvr()) { + while (Running()) { + // Read data from the DVR device: + uchar *b = NULL; + if (GetTSPacket(b)) { + if (b) { + int Pid = (((uint16_t)b[1] & PID_MASK_HI) << 8) | b[2]; + // Distribute the packet to all attached receivers: + Lock(); + for (int i = 0; i < MAXRECEIVERS; i++) { + if (receiver[i] && receiver[i]->WantsPid(Pid)) + receiver[i]->Receive(b, TS_SIZE); + } + Unlock(); + } + } + else + break; + } CloseDvr(); } } @@ -1188,10 +1186,8 @@ Receiver->device = this; receiver[i] = Receiver; Unlock(); - if (!active) { - active = true; + if (!Running()) Start(); - } return true; } } @@ -1218,10 +1214,8 @@ else if (receiver[i]) receiversLeft = true; } - if (!receiversLeft) { - active = false; + if (!receiversLeft) Cancel(3); - } }
void cDevice::DetachAll(int Pid) @@ -1246,13 +1240,11 @@ delivered = false; ringBuffer = new cRingBufferLinear(Size, TS_SIZE, true, "TS"); ringBuffer->SetTimeouts(100, 100); - active = true; Start(); }
cTSBuffer::~cTSBuffer() { - active = false; Cancel(3); delete ringBuffer; } @@ -1262,20 +1254,20 @@ if (ringBuffer) { bool firstRead = true; cPoller Poller(f); - for (; active;) { - if (firstRead || Poller.Poll(100)) { - firstRead = false; - int r = ringBuffer->Read(f); - if (r < 0 && FATALERRNO) { - if (errno == EOVERFLOW) - esyslog("ERROR: driver buffer overflow on device %d", cardIndex); - else { - LOG_ERROR; - break; - } - } - } - } + while (Running()) { + if (firstRead || Poller.Poll(100)) { + firstRead = false; + int r = ringBuffer->Read(f); + if (r < 0 && FATALERRNO) { + if (errno == EOVERFLOW) + esyslog("ERROR: driver buffer overflow on device %d", cardIndex); + else { + LOG_ERROR; + break; + } + } + } + } } }
=================================================================== RCS file: ./RCS/device.h retrieving revision 1.60 diff -u -r1.60 ./device.h --- ./device.h 2005/07/30 09:31:53 1.60 +++ ./device.h 2005/08/13 11:44:13 @@ -233,7 +233,6 @@ // PID handle facilities
private: - bool active; virtual void Action(void); protected: enum ePidType { ptAudio, ptVideo, ptPcr, ptTeletext, ptDolby, ptOther }; @@ -518,7 +517,6 @@ private: int f; int cardIndex; - bool active; bool delivered; cRingBufferLinear *ringBuffer; virtual void Action(void); =================================================================== RCS file: ./RCS/dvbdevice.c retrieving revision 1.131 diff -u -r1.131 ./dvbdevice.c --- ./dvbdevice.c 2005/06/19 11:00:43 1.131 +++ ./dvbdevice.c 2005/08/14 10:52:26 @@ -76,7 +76,6 @@ cCiHandler *ciHandler; cChannel channel; const char *diseqcCommands; - bool active; bool useCa; time_t startTime; eTunerStatus tunerStatus; @@ -101,7 +100,6 @@ frontendType = FrontendType; ciHandler = CiHandler; diseqcCommands = NULL; - active = false; useCa = false; tunerStatus = tsIdle; startTime = time(NULL); @@ -113,7 +111,6 @@
cDvbTuner::~cDvbTuner() { - active = false; tunerStatus = tsIdle; newSet.Signal(); Cancel(3); @@ -294,8 +291,7 @@ void cDvbTuner::Action(void) { dvb_frontend_event event; - active = true; - while (active) { + while (Running()) { Lock(); if (tunerStatus == tsSet) { while (GetFrontendEvent(event)) =================================================================== RCS file: ./RCS/dvbplayer.c retrieving revision 1.36 diff -u -r1.36 ./dvbplayer.c --- ./dvbplayer.c 2005/07/30 10:00:24 1.36 +++ ./dvbplayer.c 2005/08/14 10:52:45 @@ -79,7 +79,6 @@ int wanted; int length; bool hasData; - bool active; cCondWait newSet; protected: void Action(void); @@ -98,13 +97,11 @@ buffer = NULL; wanted = length = 0; hasData = false; - active = false; Start(); }
cNonBlockingFileReader::~cNonBlockingFileReader() { - active = false; newSet.Signal(); Cancel(3); free(buffer); @@ -147,8 +144,7 @@
void cNonBlockingFileReader::Action(void) { - active = true; - while (active) { + while (Running()) { Lock(); if (!hasData && f >= 0 && buffer) { int r = safe_read(f, buffer + length, wanted - length); @@ -187,8 +183,6 @@ cIndexFile *index; int replayFile; bool eof; - bool active; - bool running; bool firstPacket; ePlayModes playMode; ePlayDirs playDir; @@ -207,7 +201,7 @@ public: cDvbPlayer(const char *FileName); virtual ~cDvbPlayer(); - bool Active(void) { return active; } + bool Active(void) { return cThread::Running(); } void Pause(void); void Play(void); void Forward(void); @@ -233,8 +227,6 @@ backTrace = NULL; index = NULL; eof = false; - active = true; - running = false; firstPacket = true; playMode = pmPlay; playDir = pdForward; @@ -353,11 +345,8 @@ if (replayFile >= 0) Start(); } - else if (active) { - running = false; + else Cancel(9); - active = false; - } }
void cDvbPlayer::Action(void) @@ -374,8 +363,7 @@ int Length = 0; bool Sleep = false;
- running = true; - while (running && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { + while (Running() && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { if (Sleep) { cCondWait::SleepMs(3); // this keeps the CPU load low Sleep = false; @@ -501,7 +489,6 @@ Sleep = true; } } - active = running = false;
cNonBlockingFileReader *nbfr = nonBlockingFileReader; nonBlockingFileReader = NULL; =================================================================== RCS file: ./RCS/recorder.c retrieving revision 1.13 diff -u -r1.13 ./recorder.c --- ./recorder.c 2005/01/16 12:53:17 1.13 +++ ./recorder.c 2005/08/14 10:53:28 @@ -29,7 +29,6 @@ uchar pictureType; int fileSize; int recordFile; - bool active; time_t lastDiskSpaceCheck; bool RunningLowOnDiskSpace(void); bool NextFile(void); @@ -43,7 +42,6 @@ cFileWriter::cFileWriter(const char *FileName, cRemux *Remux) :cThread("file writer") { - active = false; fileName = NULL; remux = Remux; index = NULL; @@ -63,7 +61,6 @@
cFileWriter::~cFileWriter() { - active = false; Cancel(3); delete index; delete fileName; @@ -96,13 +93,11 @@ void cFileWriter::Action(void) { time_t t = time(NULL); - active = true; - while (active) { + while (Running()) { int Count; uchar *p = remux->Get(Count, &pictureType); if (p) { - //XXX+ active??? see old version (Busy) - if (!active && pictureType == I_FRAME) // finish the recording before the next 'I' frame + if (!Running() && pictureType == I_FRAME) // finish the recording before the next 'I' frame break; if (NextFile()) { if (index && pictureType != NO_PICTURE) @@ -124,15 +119,12 @@ t = time(NULL); } } - active = false; }
cRecorder::cRecorder(const char *FileName, int Ca, int Priority, int VPid, const int *APids, const int *DPids, const int *SPids) :cReceiver(Ca, Priority, VPid, APids, Setup.UseDolbyDigital ? DPids : NULL, SPids) ,cThread("recording") { - active = false; - // Make sure the disk is up and running:
SpinUpDisk(FileName); @@ -157,25 +149,22 @@ writer->Start(); Start(); } - else if (active) { - active = false; + else Cancel(3); - } }
void cRecorder::Receive(uchar *Data, int Length) { - if (active) { + if (Running()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && active) + if (p != Length && Running()) ringBuffer->ReportOverflow(Length - p); } }
void cRecorder::Action(void) { - active = true; - while (active) { + while (Running()) { int r; uchar *b = ringBuffer->Get(r); if (b) { =================================================================== RCS file: ./RCS/recorder.h retrieving revision 1.3 diff -u -r1.3 ./recorder.h --- ./recorder.h 2005/01/15 16:35:53 1.3 +++ ./recorder.h 2005/08/13 11:31:18 @@ -23,7 +23,6 @@ cRingBufferLinear *ringBuffer; cRemux *remux; cFileWriter *writer; - bool active; protected: virtual void Activate(bool On); virtual void Receive(uchar *Data, int Length); =================================================================== RCS file: ./RCS/remote.c retrieving revision 1.42 diff -u -r1.42 ./remote.c --- ./remote.c 2005/03/20 13:25:31 1.42 +++ ./remote.c 2005/08/14 10:53:55 @@ -213,7 +213,6 @@ :cRemote("KBD") ,cThread("KBD remote control") { - active = false; tcgetattr(STDIN_FILENO, &savedTm); struct termios tm; if (tcgetattr(STDIN_FILENO, &tm) == 0) { @@ -230,7 +229,6 @@ cKbdRemote::~cKbdRemote() { kbdAvailable = false; - active = false; Cancel(3); tcsetattr(STDIN_FILENO, TCSANOW, &savedTm); } @@ -261,12 +259,11 @@ void cKbdRemote::Action(void) { cPoller Poller(STDIN_FILENO); - active = true; - while (active) { + while (Running()) { if (Poller.Poll(100)) { uint64 Command = 0; uint i = 0; - while (active && i < sizeof(Command)) { + while (Running() && i < sizeof(Command)) { uchar ch; int r = read(STDIN_FILENO, &ch, 1); if (r == 1) { =================================================================== RCS file: ./RCS/remote.h retrieving revision 1.29 diff -u -r1.29 ./remote.h --- ./remote.h 2004/05/28 14:14:02 1.29 +++ ./remote.h 2005/08/13 11:28:10 @@ -81,7 +81,6 @@
class cKbdRemote : public cRemote, private cThread { private: - bool active; static bool kbdAvailable; static bool rawMode; struct termios savedTm; =================================================================== RCS file: ./RCS/sections.c retrieving revision 1.11 diff -u -r1.11 ./sections.c --- ./sections.c 2005/05/29 11:43:17 1.11 +++ ./sections.c 2005/08/14 10:54:39 @@ -44,7 +44,6 @@ { shp = new cSectionHandlerPrivate; device = Device; - active = false; statusCount = 0; on = false; waitForLock = false; @@ -54,7 +53,6 @@
cSectionHandler::~cSectionHandler() { - active = false; Cancel(3); cFilter *fi; while ((fi = filters.First()) != NULL) @@ -166,9 +164,8 @@
void cSectionHandler::Action(void) { - active = true; SetPriority(19); - while (active) { + while (Running()) {
Lock(); if (waitForLock) =================================================================== RCS file: ./RCS/sections.h retrieving revision 1.4 diff -u -r1.4 ./sections.h --- ./sections.h 2004/08/08 13:44:17 1.4 +++ ./sections.h 2005/08/13 11:23:55 @@ -25,7 +25,6 @@ private: cSectionHandlerPrivate *shp; cDevice *device; - bool active; int statusCount; bool on, waitForLock; time_t lastIncompleteSection; =================================================================== RCS file: ./RCS/thread.c retrieving revision 1.43 diff -u -r1.43 ./thread.c --- ./thread.c 2005/05/29 11:40:30 1.43 +++ ./thread.c 2005/08/14 11:15:42 @@ -197,7 +197,7 @@
cThread::cThread(const char *Description) { - running = false; + active = running = false; childTid = 0; description = NULL; SetDescription(Description); @@ -205,6 +205,7 @@
cThread::~cThread() { + Cancel(); // just in case the derived class didn't call it free(description); }
@@ -234,20 +235,21 @@ if (Thread->description) dsyslog("%s thread ended (pid=%d, tid=%ld)", Thread->description, getpid(), pthread_self()); Thread->running = false; + Thread->active = false; return NULL; }
bool cThread::Start(void) { - if (!running) { - running = true; + if (!active) { + active = running = true; if (pthread_create(&childTid, NULL, (void *(*) (void *))&StartThread, (void *)this) == 0) { pthread_detach(childTid); // auto-reap pthread_setschedparam(childTid, SCHED_RR, 0); } else { LOG_ERROR; - running = false; + active = running = false; return false; } } @@ -256,7 +258,7 @@
bool cThread::Active(void) { - if (running) { + if (active) { // // Single UNIX Spec v2 says: // @@ -271,7 +273,7 @@ if (err != ESRCH) LOG_ERROR; childTid = 0; - running = false; + active = running = false; } else return true; @@ -281,7 +283,8 @@
void cThread::Cancel(int WaitSeconds) { - if (running) { + running = false; + if (active) { if (WaitSeconds > 0) { for (time_t t0 = time(NULL) + WaitSeconds; time(NULL) < t0; ) { if (!Active()) @@ -292,7 +295,7 @@ } pthread_cancel(childTid); childTid = 0; - running = false; + active = false; } }
=================================================================== RCS file: ./RCS/thread.h retrieving revision 1.28 diff -u -r1.28 ./thread.h --- ./thread.h 2005/05/29 11:31:24 1.28 +++ ./thread.h 2005/08/14 11:14:34 @@ -75,6 +75,7 @@ class cThread { friend class cThreadLock; private: + bool active; bool running; pthread_t childTid; cMutex mutex; @@ -86,13 +87,30 @@ void Lock(void) { mutex.Lock(); } void Unlock(void) { mutex.Unlock(); } virtual void Action(void) = 0; + ///< A derived cThread class must implement the code it wants to + ///< execute as a separate thread in this function. If this is + ///< a loop, it must check Running() repeatedly to see whether + ///< it's time to stop. + bool Running(void) { return running; } + ///< Returns false if a derived cThread object shall leave its Action() + ///< function. void Cancel(int WaitSeconds = 0); + ///< Cancels the thread by first setting 'running' to false, so that + ///< the Action() loop can finish in an orderly fashion and then waiting + ///< up to WaitSeconds seconds for the thread to actually end. If the + ///< thread doesn't end by itself, it is killed. public: cThread(const char *Description = NULL); + ///< Creates a new thread. + ///< If Description is present, a log file entry will be made when + ///< the thread starts and stops. The Start() function must be called + ///< to actually start the thread. virtual ~cThread(); void SetDescription(const char *Description, ...); bool Start(void); + ///< Actually starts the thread. bool Active(void); + ///< Checks whether the thread is still alive. static bool EmergencyExit(bool Request = false); };
=================================================================== RCS file: ./RCS/transfer.c retrieving revision 1.28 diff -u -r1.28 ./transfer.c --- ./transfer.c 2005/02/19 14:38:55 1.28 +++ ./transfer.c 2005/08/14 10:55:03 @@ -21,7 +21,6 @@ ringBuffer = new cRingBufferLinear(TRANSFERBUFSIZE, TS_SIZE * 2, true, "Transfer"); remux = new cRemux(VPid, APids, Setup.UseDolbyDigital ? DPids : NULL, SPids); needsBufferReserve = Setup.UseDolbyDigital && VPid != 0 && DPids && DPids[0] != 0; - active = false; }
cTransfer::~cTransfer() @@ -34,21 +33,17 @@
void cTransfer::Activate(bool On) { - if (On) { - if (!active) - Start(); - } - else if (active) { - active = false; + if (On) + Start(); + else Cancel(3); - } }
void cTransfer::Receive(uchar *Data, int Length) { - if (IsAttached() && active) { + if (IsAttached() && Running()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && active) + if (p != Length && Running()) ringBuffer->ReportOverflow(Length - p); return; } @@ -70,8 +65,7 @@ bool GotBufferReserve = false; int RequiredBufferReserve = KILOBYTE(DvbCardWith4MBofSDRAM ? 288 : 576); #endif - active = true; - while (active) { + while (Running()) { #ifdef FW_NEEDS_BUFFER_RESERVE_FOR_AC3 if (needsBufferReserve && !GotBufferReserve) { //XXX For dolby we've to fill the buffer because the firmware does @@ -145,7 +139,6 @@ } } } - active = false; }
// --- cTransferControl ------------------------------------------------------ =================================================================== RCS file: ./RCS/transfer.h retrieving revision 1.9 diff -u -r1.9 ./transfer.h --- ./transfer.h 2005/01/15 16:39:39 1.9 +++ ./transfer.h 2005/08/13 10:16:02 @@ -21,7 +21,6 @@ cRingBufferLinear *ringBuffer; cRemux *remux; bool needsBufferReserve; - bool active; protected: virtual void Activate(bool On); virtual void Receive(uchar *Data, int Length);
On 14 Aug 2005 Klaus Schmidinger Klaus.Schmidinger@cadsoft.de wrote:
Ok, here it is. cThread::Active() is as it was before, and the new function is called cThread::Running() as suggested by Luca Olivetti.
I run this version since some hours. No noticeable difference. All plugins compiled fine (without any change) and seem to work normal (mp3, mplayer, dvd).
If something show up in the next days, I'll report.
Regards.
-
C.Y.M -
Gunnar Roth -
Klaus Schmidinger -
Luca Olivetti -
s.huelswitt＠gmx.de -
Ville Skyttä