Replace some sleeps with condition variables #14
Conversation
cOmx::PollVideo(), cOmx::GetBufferUsage(): Protect the calculation with the mutex, and simplify the arithmetics.
|
Replacing the remaining |
| audio /= BUFFERSTAT_FILTER_SIZE * OMX_AUDIO_BUFFERS / 100; | ||
| video /= BUFFERSTAT_FILTER_SIZE * OMX_VIDEO_BUFFERS / 100; | ||
| } | ||
|
|
||
| void cOmx::HandlePortBufferEmptied(eOmxComponent component) | ||
| { | ||
| Lock(); | ||
| m_mutex.Lock(); |
There was a problem hiding this comment.
m_usedAudioBuffers[] and m_usedVideoBuffers[] are protected by cOmx::m_mutex elsewhere. Sure, they could be protected by cThread::mutex, but then some code would have to acquire two mutexes. For these simple counters, it could make sense to upgrade to C++11 (which is supported starting with GCC 4.8.5), make the counters std::atomic<int> and use fetch_sub(1), to avoid any race condition.
Unfortunately, cThread::mutex is not declared protected, but private. Therefore, there is no way for derived classes such as cOmx to use it together with a cCondVar (such as cOmx::m_portEventsAdded). To be able to use cCondVar, we are forced to declare a separate cMutex on our own.
Another somewhat unfortunate abstraction is that there is no variant of cCondVar::TimedWait() that would allow a previously set expiration period to be reused. In cOmx::Action(), we could replace the local variable timer with more clever logic: Set a struct timespec abstime 100 milliseconds into the future, and keep invoking pthread_cond_timedwait() with that expiration time. If and only if a timeout expires, set the timeout 100 milliseconds into the future and perform the periodic action. This would save quite a few context switches due to system calls and wakeups.
There was a problem hiding this comment.
Ok, thanks for the explanations. For the extension of cCondVar::TimedWait() it could make sense to submit a patch to Klaus, as the author of VDR, he's very open to sensible improvements of VDR's core.
There was a problem hiding this comment.
Sure. I will first have to think about an API change that would allow cOmx::Action() to be rewritten in the way I envisioned.
There was a problem hiding this comment.
I proposed some API changes in https://www.linuxtv.org/pipermail/vdr/2023-January/029700.html
and wrote a patch that makes use of it:
diff --git a/omx.c b/omx.c
index 7e517ec..07a6cb1 100644
--- a/omx.c
+++ b/omx.c
@@ -118,15 +118,26 @@ const char* cOmx::errStr(int err)
void cOmx::Action(void)
{
- cTimeMs timer;
+ timespec abstime;
+ if (!cCondVar::GetAbsTime(&abstime, 100))
+ return;
m_mutex.Lock();
while (Running())
{
- while (m_portEvents.empty())
- if (m_portEventsAdded.TimedWait(m_mutex, 10))
- goto timeout;
-
+ if (!m_portEvents.empty());
+ else if (!m_portEventsAdded.TimedWait(m_mutex, abstime))
{
+ memmove(m_usedAudioBuffers, m_usedAudioBuffers + 1,
+ (sizeof m_usedAudioBuffers) -
+ sizeof *m_usedAudioBuffers);
+ memmove(m_usedVideoBuffers, m_usedVideoBuffers + 1,
+ (sizeof m_usedVideoBuffers) -
+ sizeof *m_usedVideoBuffers);
+ if (!cCondVar::GetAbsTime(&abstime, 100))
+ break;
+ continue;
+ }
+
const Event event = m_portEvents.front();
m_portEvents.pop();
m_mutex.Unlock();
@@ -163,19 +174,6 @@ void cOmx::Action(void)
m_mutex.Lock();
}
-
-timeout:
- if (timer.TimedOut())
- {
- timer.Set(100);
- memmove(m_usedAudioBuffers, m_usedAudioBuffers + 1,
- (sizeof m_usedAudioBuffers) -
- sizeof *m_usedAudioBuffers);
- memmove(m_usedVideoBuffers, m_usedVideoBuffers + 1,
- (sizeof m_usedVideoBuffers) -
- sizeof *m_usedVideoBuffers);
- }
- }
m_mutex.Unlock();
}
The final patch would have to be conditional on the VDR version number, once something like my proposal becomes available.
My attempts to replace all m_mutex with mutex (e.g., replace m_mutex.Lock() with Lock()) led to deadlocks, as I noted in #15.
There was a problem hiding this comment.
The call to gettimeofday() inside GetAbsTime() should be relatively cheap nowadays, thanks to the virtual system call implemented in linux-vdso.so, but with the above patch, the abstime could be advanced by more than 0.1 seconds in each step.
To avoid cumulative error, we should probably just add 0.1 seconds to the abstime. In case the system clock could be adjusted backwards, it could be a good idea to first ensure that abstime is at most a few seconds behind the current gettimeofday().
There was a problem hiding this comment.
The above idea is included in #16.
There was a problem hiding this comment.
#15 answers the main question: Why can’t we use cThread::mutex a.k.a. cOmx::mutex? It protects "entry" calls to ilclient_ and OMX_. The cOmx::m_mutex protects data structures in code that is invoked by callback functions of those libraries. The "main thread" that is holding cOmx::mutex may be blocked, waiting for those callback functions to complete. I believe that for this reason, the cOmx::m_portEvents queue is necessary, and the callback functions must use a different mutex. While working on #15, I tried to eliminate the queue, in vain, until I realized why the deadlocks occur.
Also, replace some timed waits with cCondVar::Wait().
This code is only invoked via cRpiAudioDecoder::m_render, mainly from cRpiAudioDecoder::Action(). Only m_render->Flush() will be invoked from cRpiAudioDecoder::DeInit(), which will first ensure that cRpiAudioDecoder::Action() has terminated.
Let us avoid some pointer indirection and heap allocation around Event objects. Passing 2 words by value should not be worse than allocating the data from heap.
I was hoping that this would allow the plugin to work with a smaller GPU memory. It actually did start up with
gpu_mem=128, displaying a HD channel, which I think previously only worked withgpu_mem=192. Still, after pressing the Menu button 4 times (to show and hide full-screen OSD twice) it was unable to open any more full-screen OSD menu atgpu_mem=128.On a quick test, the OSD still seems to be fine.