From 193b125c48af20286a99397efee4bffbec89ae77 Mon Sep 17 00:00:00 2001
From: Andreas Traczyk <andreas.traczyk@savoirfairelinux.com>
Date: Tue, 30 May 2023 17:08:10 -0400
Subject: [PATCH] video: shm: cleanup lock in shmrenderer
Adds an atomic bool used to exit the render in case frameSize is reset by the daemon.
Change-Id: I8c2ae01a3de79704f83930507ee147a34c1628b8
---
src/libclient/shmrenderer.cpp | 39 +++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/src/libclient/shmrenderer.cpp b/src/libclient/shmrenderer.cpp
index 734023f0d..50123d4f8 100644
--- a/src/libclient/shmrenderer.cpp
+++ b/src/libclient/shmrenderer.cpp
@@ -22,7 +22,7 @@
#include "dbus/videomanager.h"
#include <QDebug>
-#include <QMutex>
+#include <QReadWriteLock>
#include <QThread>
#include <sys/ipc.h>
@@ -86,12 +86,13 @@ public:
// This is necessary because the frame rate is not constant.
// The function getNewFrame() will return false if no new frame is available.
thread = QThread::create([this] {
+ running = true;
forever {
if (QThread::currentThread()->isInterruptionRequested()) {
return;
}
- if (!waitForNewFrame()) {
+ if (!waitForNewFrame() || !running) {
continue;
}
@@ -107,10 +108,15 @@ public:
// Request thread loop interruption and then unblock the sem_wait.
thread->requestInterruption();
- // Set the isDestroying flag to true so that the thread loop can exit
+ // Set the frameSize to 0 and notify so that the thread loop can exit
// without emitting the frameUpdated signal for an invalid resolution
// (e.g. smartphone rotation).
// This works as ShmHolder::renderFrame should reset frameSize appropriately.
+ // However, the daemon may change the frameSize to 0 after we set it to 0 and
+ // before the frameSize is checked in the thread loop. So we use the running
+ // flag to make sure the thread loop exits without calling frameUpdated.
+ running = false;
+
shmLock();
shmArea->frameSize = 0;
shmUnlock();
@@ -160,10 +166,14 @@ public:
return false;
}
- if (not frame)
- frame.reset(new lrc::api::video::Frame);
- frame->ptr = shmArea->data + shmArea->readOffset;
- frame->size = shmArea->frameSize;
+ {
+ QWriteLocker lk {&frameMutex};
+ if (not frame)
+ frame.reset(new lrc::api::video::Frame);
+ frame->ptr = shmArea->data + shmArea->readOffset;
+ frame->size = shmArea->frameSize;
+ }
+
frameGen = shmArea->frameGen;
shmUnlock();
@@ -212,8 +222,9 @@ public:
unsigned shmAreaLen;
uint frameGen;
- QMutex mutex;
+ QReadWriteLock frameMutex;
QThread* thread;
+ std::atomic<bool> running {false};
std::shared_ptr<lrc::api::video::Frame> frame;
};
@@ -226,14 +237,14 @@ ShmRenderer::ShmRenderer(const QString& id, const QSize& res, const QString& shm
ShmRenderer::~ShmRenderer()
{
- VideoManager::instance().startShmSink(id(), false);
stopShm();
+ VideoManager::instance().startShmSink(id(), false);
}
Frame
ShmRenderer::currentFrame() const
{
- QMutexLocker lk {&pimpl_->mutex};
+ QReadLocker lk {&pimpl_->frameMutex};
if (auto frame_ptr = pimpl_->frame)
return std::move(*frame_ptr);
return {};
@@ -276,14 +287,14 @@ ShmRenderer::stopShm()
if (pimpl_->fd < 0)
return;
+ pimpl_->stopThread();
+
// Emit the signal before closing the file, this lower the risk of invalid
// memory access
Q_EMIT stopped();
- pimpl_->stopThread();
-
{
- QMutexLocker lk(&pimpl_->mutex);
+ QWriteLocker lk(&pimpl_->frameMutex);
// reset the frame so it doesn't point to an old value
pimpl_->frame.reset();
}
@@ -302,8 +313,6 @@ ShmRenderer::stopShm()
void
ShmRenderer::startRendering()
{
- QMutexLocker lk(&pimpl_->mutex);
-
if (!startShm())
return;
--
GitLab