[PATCH] Use cThread::mutex with absolute cCondVar::TimedWait()

[Marko Mäkelä <marko.makela@xxxxxx>]

Hi,

I would propose the following patch, or some equivalent interface that 
would allow cThread::mutex to be used with some cCondVar in derived 
classes:

diff --git a/thread.h b/thread.h
index 16c4bd75..cd1d98ab 100644
--- a/thread.h
+++ b/thread.h
@@ -83,7 +83,9 @@ private:
   bool running;
   pthread_t childTid;
   tThreadId childThreadId;
+protected:
   cMutex mutex;
+private:
   char *description;
   bool lowPriority;
   static tThreadId mainThreadId;

Because cThread::mutx is declared private and there is no helper member 
function, derived classes that wish to use condition variables have to 
instantiate a separate cMutex for that. Here is an example from the 
rpihddevice plugin that illustrates my point:

diff --git a/omx.c b/omx.c
--- a/omx.c
+++ b/omx.c
@@ -119,17 +119,17 @@ const char* cOmx::errStr(int err)
 void cOmx::Action(void)
 {
 	cTimeMs timer;
-	m_mutex.Lock();
+	Lock();
 	while (Running())
 	{
 		while (m_portEvents.empty())
-			if (m_portEventsAdded.TimedWait(m_mutex, 10))
+			if (!m_portEventsAdded.TimedWait(mutex, 10))
 				goto timeout;
 
 		{
 			const Event event = m_portEvents.front();
 			m_portEvents.pop();
-			m_mutex.Unlock();
+			Unlock();
 
 			switch (event.event)
 			{

Actually, there is a bug above: the condition for the TimedWait() call 
was inverted.

This code illustrates another limitation: There is no way to pass an 
absolute time to cCondVar::TimedWait(). On each call, a relative wake-up 
time (milliseconds from the current time) will be converted into an 
absolute time. If there was a way, we would be able to remove the 
"cTimeMs timer" and some related system calls, and have this loop both 
wake up every 100 milliseconds, and process events as soon as they 
arrive. Here is the VDR part of the patch:

diff --git a/thread.c b/thread.c
index 93eb8c0d..3dcb44d4 100644
--- a/thread.c
+++ b/thread.c
@@ -36,7 +36,7 @@
 #define dbglocking(a...)
 #endif
 
-static bool GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow)
+bool cCondVar::GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow)
 {
   struct timeval now;
   if (gettimeofday(&now, NULL) == 0) {           // get current time
@@ -81,7 +81,7 @@ bool cCondWait::Wait(int TimeoutMs)
   if (!signaled) {
      if (TimeoutMs) {
         struct timespec abstime;
-        if (GetAbsTime(&abstime, TimeoutMs)) {
+        if (cCondVar::GetAbsTime(&abstime, TimeoutMs)) {
            while (!signaled) {
                  if (pthread_cond_timedwait(&cond, &mutex, &abstime) == ETIMEDOUT)
                     break;
@@ -129,20 +129,27 @@ void cCondVar::Wait(cMutex &Mutex)
      }
 }
 
+bool cCondVar::TimedWait(cMutex &Mutex, const timespec &abstime)
+{
+  int err = 0;
+
+  if (int locked = Mutex.locked) {
+     Mutex.locked = 0; // have to clear the locked count here, as pthread_cond_timedwait
+                       // does an implicit unlock of the mutex.
+     err = pthread_cond_timedwait(&cond, &Mutex.mutex, &abstime);
+     Mutex.locked = locked;
+     }
+  return err != ETIMEDOUT;
+}
+
 bool cCondVar::TimedWait(cMutex &Mutex, int TimeoutMs)
 {
   bool r = true; // true = condition signaled, false = timeout
 
   if (Mutex.locked) {
      struct timespec abstime;
-     if (GetAbsTime(&abstime, TimeoutMs)) {
-        int locked = Mutex.locked;
-        Mutex.locked = 0; // have to clear the locked count here, as pthread_cond_timedwait
-                          // does an implicit unlock of the mutex.
-        if (pthread_cond_timedwait(&cond, &Mutex.mutex, &abstime) == ETIMEDOUT)
-           r = false;
-        Mutex.locked = locked;
-        }
+     if (GetAbsTime(&abstime, TimeoutMs))
+        r = TimedWait(Mutex, abstime);
      }
   return r;
 }
@@ -174,7 +181,7 @@ bool cRwLock::Lock(bool Write, int TimeoutMs)
   int Result = 0;
   struct timespec abstime;
   if (TimeoutMs) {
-     if (!GetAbsTime(&abstime, TimeoutMs))
+     if (!cCondVar::GetAbsTime(&abstime, TimeoutMs))
         TimeoutMs = 0;
      }
   if (Write) {
diff --git a/thread.h b/thread.h
index 16c4bd75..04bb4cc5 100644
--- a/thread.h
+++ b/thread.h
@@ -49,7 +49,9 @@ public:
   ~cCondVar();
   void Wait(cMutex &Mutex);
   bool TimedWait(cMutex &Mutex, int TimeoutMs);
+  bool TimedWait(cMutex &Mutex, const timespec &abstime);
   void Broadcast(void);
+  static bool GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow);
   };
 
 class cRwLock {

I did not complete the change to rpihddevice cOmx::Action() yet. We may 
be forced to retain two mutexes after all, because OMX_EmptyThisBuffer() 
as well as one of the functions called from cOmx::StopAudio() would hang 
indefinitely, while holding the cOmx::mutex, blocking the cOmx::Action() 
thread. According to 
https://forums.raspberrypi.com/viewtopic.php?t=313170 the interface is 
largely deprecated. I will try to figure out something.

Best regards,

	Marko

_______________________________________________
vdr mailing list
vdr@xxxxxxxxxxx
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr