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

@@ -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

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.";

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_;