From 2baf76e74b449622beed35533ea1b8c57f33cac0 Mon Sep 17 00:00:00 2001
From: Andreas Traczyk <andreas.traczyk@savoirfairelinux.com>
Date: Tue, 21 Feb 2023 18:18:19 -0500
Subject: [PATCH] videoprovider: remove frame subscription on
QVideoSink::destroyed
We can observe that frame update signals can occur after a VideoOutput component's QVideoSink object has started destruction. The deregistration of the object pointer needs to be synchronized to the object's destruction.
Additionally:
+ scope the frame obj mutex locker around checks for no subs
+ rename (un)registerSink to (un)subscribe
+ subscribe will now remove the previous subscription
+ subscribe with an empty id will unsubscribe
Gitlab: #997
Change-Id: I21460564e49189b9276f153e8c1fd3a5cc52c6b2
---
src/app/commoncomponents/VideoView.qml | 9 +---
src/app/videoprovider.cpp | 61 ++++++++++++++++++++------
src/app/videoprovider.h | 6 +--
3 files changed, 52 insertions(+), 24 deletions(-)
diff --git a/src/app/commoncomponents/VideoView.qml b/src/app/commoncomponents/VideoView.qml
index 897c03f4e..12b138098 100644
--- a/src/app/commoncomponents/VideoView.qml
+++ b/src/app/commoncomponents/VideoView.qml
@@ -39,12 +39,7 @@ Item {
property real xScale: contentRect.width / videoOutput.sourceRect.width
property real yScale: contentRect.height / videoOutput.sourceRect.height
- onRendererIdChanged: {
- videoProvider.unregisterSink(videoSink)
- if (rendererId.length !== 0) {
- videoProvider.registerSink(rendererId, videoSink)
- }
- }
+ onRendererIdChanged: videoProvider.subscribe(videoSink, rendererId)
Rectangle {
id: bgRect
@@ -71,8 +66,6 @@ Item {
Behavior on opacity { NumberAnimation { duration: 150 } }
- Component.onDestruction: videoProvider.unregisterSink(videoSink)
-
layer.enabled: opacity
layer.effect: FastBlur {
source: videoOutput
diff --git a/src/app/videoprovider.cpp b/src/app/videoprovider.cpp
index 3cd54f398..99d5f2cfc 100644
--- a/src/app/videoprovider.cpp
+++ b/src/app/videoprovider.cpp
@@ -57,39 +57,74 @@ VideoProvider::VideoProvider(AVModel& avModel, QObject* parent)
}
void
-VideoProvider::registerSink(const QString& id, QVideoSink* obj)
+VideoProvider::subscribe(QObject* obj, const QString& id)
{
+ // First remove any previously existing subscription.
+ unsubscribe(obj);
+
+ if (id.isEmpty()) {
+ return;
+ }
+
+ // Make sure we're dealing with a QVideoSink object.
+ auto sink = qobject_cast<QVideoSink*>(obj);
+ if (sink == nullptr) {
+ qWarning() << Q_FUNC_INFO << "Object must be a QVideoSink.";
+ return;
+ }
+
+ // We need to detect the destruction of the QVideoSink, which is destroyed before
+ // it's parent VideoOutput component emits a Component.destruction signal.
+ // e.i. If we use: Component.onDestruction: videoProvider.removeSubscription(videoSink),
+ // and a frame update occurs, it's possible the QVideoSink is in the process of being,
+ // or has already been destroyed.
+ connect(sink,
+ &QVideoSink::destroyed,
+ this,
+ &VideoProvider::unsubscribe,
+ static_cast<Qt::ConnectionType>(Qt::DirectConnection | Qt::UniqueConnection));
+
QMutexLocker lk(&framesObjsMutex_);
+ // Check if we already have a FrameObject for this id.
auto it = framesObjects_.find(id);
if (it == framesObjects_.end()) {
auto fo = std::make_unique<FrameObject>();
- fo->subscribers.insert(obj);
qDebug() << "Creating new FrameObject for id:" << id;
auto emplaced = framesObjects_.emplace(id, std::move(fo));
if (!emplaced.second) {
- qWarning() << "Couldn't create FrameObject for id:" << id;
+ qWarning() << Q_FUNC_INFO << "Couldn't create FrameObject for id:" << id;
return;
}
+ // Get the iterator to the newly created FrameObject so we can add the subscriber.
it = emplaced.first;
+ } else {
+ // Make sure it's not already subscribed to this QVideoSink.
+ QMutexLocker subsLk(&it->second->mutex);
+ if (it->second->subscribers.contains(sink)) {
+ qWarning() << Q_FUNC_INFO << "QVideoSink already subscribed to id:" << id;
+ return;
+ }
}
- qDebug().noquote() << QString("Adding sink: 0x%1 to subscribers for id: %2")
- .arg((quintptr) obj, QT_POINTER_SIZE * 2, 16, QChar('0'))
+ QMutexLocker subsLk(&it->second->mutex);
+ it->second->subscribers.insert(sink);
+ qDebug().noquote() << QString("Added sink: 0x%1 to subscribers for id: %2")
+ .arg((quintptr) obj, QT_POINTER_SIZE, 16, QChar('0'))
.arg(id);
- it->second->subscribers.insert(obj);
}
void
-VideoProvider::unregisterSink(QVideoSink* obj)
+VideoProvider::unsubscribe(QObject* obj)
{
QMutexLocker lk(&framesObjsMutex_);
for (auto& frameObjIt : qAsConst(framesObjects_)) {
+ QMutexLocker subsLk(&frameObjIt.second->mutex);
auto& subs = frameObjIt.second->subscribers;
- auto it = subs.constFind(obj);
+ auto it = subs.constFind(static_cast<QVideoSink*>(obj));
if (it != subs.cend()) {
- qDebug().noquote() << QString("Removing sink: 0x%1 from subscribers for id: %2")
- .arg((quintptr) obj, QT_POINTER_SIZE * 2, 16, QChar('0'))
- .arg(frameObjIt.first);
subs.erase(it);
+ qDebug().noquote() << QString("Removed sink: 0x%1 from subscribers for id: %2")
+ .arg((quintptr) obj, QT_POINTER_SIZE, 16, QChar('0'))
+ .arg(frameObjIt.first);
return;
}
}
@@ -103,10 +138,10 @@ VideoProvider::frame(const QString& id)
if (it == framesObjects_.end()) {
return {};
}
+ QMutexLocker lk(&it->second->mutex);
if (it->second->subscribers.empty()) {
return {};
}
- QMutexLocker lk(&it->second->mutex);
auto videoFrame = it->second->videoFrame.get();
if (!mapVideoFrame(videoFrame)) {
qWarning() << "QVideoFrame can't be mapped" << id;
@@ -191,10 +226,10 @@ VideoProvider::onFrameUpdated(const QString& id)
if (it == framesObjects_.end()) {
return;
}
+ QMutexLocker lk(&it->second->mutex);
if (it->second->subscribers.empty()) {
return;
}
- QMutexLocker lk(&it->second->mutex);
auto videoFrame = it->second->videoFrame.get();
if (videoFrame == nullptr) {
qWarning() << "QVideoFrame has not been initialized.";
diff --git a/src/app/videoprovider.h b/src/app/videoprovider.h
index 80384502d..1edcf1782 100644
--- a/src/app/videoprovider.h
+++ b/src/app/videoprovider.h
@@ -43,8 +43,8 @@ public:
explicit VideoProvider(AVModel& avModel, QObject* parent = nullptr);
~VideoProvider() = default;
- Q_INVOKABLE void registerSink(const QString& id, QVideoSink* obj);
- Q_INVOKABLE void unregisterSink(QVideoSink* obj);
+ Q_INVOKABLE void subscribe(QObject* obj, const QString& id = {});
+ Q_INVOKABLE void unsubscribe(QObject* obj);
Q_INVOKABLE QString captureVideoFrame(const QString& id);
Q_INVOKABLE QImage captureRawVideoFrame(const QString& id);
@@ -53,9 +53,9 @@ private Q_SLOTS:
void onFrameBufferRequested(const QString& id, AVFrame* avframe);
void onFrameUpdated(const QString& id);
void onRendererStopped(const QString& id);
- QVideoFrame* frame(const QString& id);
private:
+ QVideoFrame* frame(const QString& id);
void copyUnaligned(QVideoFrame* dst, const video::Frame& src);
AVModel& avModel_;
--
GitLab