From 6e1dd92f0c703ce37b7ec896eb89b4d5694e4870 Mon Sep 17 00:00:00 2001 From: jp9000 Date: Sun, 2 Feb 2014 14:26:23 -0700 Subject: [PATCH] Improve thread safety in UI code - Implemented better C++ classes for handling scenes/sources/items in obs.hpp, allowing them to automatically increment and decrement the references of each, as well as assign them to QVariants. - Because QVariants are now using the C++ classes, remove the pointer QVariant wrapper. - Use the new C++ classes with the QVariant user data of list box items, both for the sake of thread safety and to ensure that the data referenced is not freed until removed. NOTE: still might need some testing. - Implemented a source-remove signal from libobs, and start using that signal instead of the source-destroy signal for signalling item removal. --- libobs/obs-scene.c | 8 ++-- libobs/obs-source.c | 19 ++++---- libobs/obs.hpp | 98 +++++++++++++++++++-------------------- obs/qt-ptr-variant.hpp | 37 --------------- obs/window-basic-main.cpp | 37 ++++++++------- obs/window-basic-main.hpp | 8 ++-- 6 files changed, 85 insertions(+), 122 deletions(-) delete mode 100644 obs/qt-ptr-variant.hpp diff --git a/libobs/obs-scene.c b/libobs/obs-scene.c index 0afa1201a..b3c2cd701 100644 --- a/libobs/obs-scene.c +++ b/libobs/obs-scene.c @@ -199,19 +199,17 @@ obs_scene_t obs_scene_create(const char *name) int obs_scene_addref(obs_scene_t scene) { - return obs_source_addref(scene->source); + return scene ? obs_source_addref(scene->source) : 0; } int obs_scene_release(obs_scene_t scene) { - if (scene) - return obs_source_release(scene->source); - return 0; + return scene ? obs_source_release(scene->source) : 0; } obs_source_t obs_scene_getsource(obs_scene_t scene) { - return scene->source; + return scene ? scene->source : NULL; } obs_scene_t obs_scene_fromsource(obs_source_t source) diff --git a/libobs/obs-source.c b/libobs/obs-source.c index 85f8ed86c..27dd557a0 100644 --- a/libobs/obs-source.c +++ b/libobs/obs-source.c @@ -250,20 +250,23 @@ void obs_source_remove(obs_source_t source) pthread_mutex_lock(&data->sources_mutex); - if (!source) + if (!source || source->removed) return; - if (!source->removed) { - source->removed = true; + source->removed = true; - id = da_find(data->sources, &source, 0); - if (id != DARRAY_INVALID) { - da_erase_item(data->sources, &source); - obs_source_release(source); - } + obs_source_addref(source); + + id = da_find(data->sources, &source, 0); + if (id != DARRAY_INVALID) { + da_erase_item(data->sources, &source); + obs_source_release(source); } pthread_mutex_unlock(&data->sources_mutex); + + obs_source_dosignal(source, "source-remove"); + obs_source_release(source); } bool obs_source_removed(obs_source_t source) diff --git a/libobs/obs.hpp b/libobs/obs.hpp index 06aa1aa10..6bba18d5e 100644 --- a/libobs/obs.hpp +++ b/libobs/obs.hpp @@ -26,75 +26,71 @@ /* RAII wrappers */ -class OBSSource { - obs_source_t source; - - OBSSource(OBSSource &&) = delete; - OBSSource(OBSSource const&) = delete; - - OBSSource &operator=(OBSSource const&) = delete; +template class OBSRef { + typedef typename RefClass::type T; + T val; public: - inline OBSSource(obs_source_t source) : source(source) {} - inline ~OBSSource() {obs_source_release(source);} + inline OBSRef() : val(nullptr) {} + inline OBSRef(T val_) : val(val_) {RefClass::AddRef(val);} + inline OBSRef(const OBSRef &ref) : val(ref.val) {RefClass::AddRef(val);} + inline OBSRef(OBSRef &&ref) : val(ref.val) {ref.val = nullptr;} - inline OBSSource& operator=(obs_source_t p) {source = p; return *this;} + inline ~OBSRef() {RefClass::Release(val);} - inline operator obs_source_t() {return source;} - - inline bool operator==(obs_source_t p) const {return source == p;} - inline bool operator!=(obs_source_t p) const {return source != p;} -}; - -class OBSSourceRef { - obs_source_t source; - -public: - inline OBSSourceRef(obs_source_t source) : source(source) + inline OBSRef &operator=(T valIn) { - obs_source_addref(source); - } - - inline OBSSourceRef(const OBSSourceRef &ref) : source(ref.source) - { - obs_source_addref(source); - } - - inline OBSSourceRef(OBSSourceRef &&ref) : source(ref.source) - { - ref.source = NULL; - } - - inline ~OBSSourceRef() {obs_source_release(source);} - - inline OBSSourceRef &operator=(obs_source_t sourceIn) - { - obs_source_addref(sourceIn); - obs_source_release(source); - source = sourceIn; + RefClass::AddRef(valIn); + RefClass::Release(val); + val = valIn; return *this; } - inline OBSSourceRef &operator=(const OBSSourceRef &ref) + inline OBSRef &operator=(const OBSRef &ref) { - obs_source_addref(ref.source); - obs_source_release(source); - source = ref.source; + RefClass::AddRef(ref.val); + RefClass::Release(val); + val = ref.val; return *this; } - inline OBSSourceRef &operator=(OBSSourceRef &&ref) + inline OBSRef &operator=(OBSRef &&ref) { if (this != &ref) { - source = ref.source; - ref.source = NULL; + val = ref.val; + ref.val = NULL; } return *this; } - inline operator obs_source_t() const {return source;} + inline operator T() const {return val;} - inline bool operator==(obs_source_t p) const {return source == p;} - inline bool operator!=(obs_source_t p) const {return source != p;} + inline bool operator==(T p) const {return val == p;} + inline bool operator!=(T p) const {return val != p;} }; + +class OBSSourceRefClass { +public: + typedef obs_source_t type; + static inline void AddRef(type val) {obs_source_addref(val);} + static inline void Release(type val) {obs_source_release(val);} +}; + +class OBSSceneRefClass { +public: + typedef obs_scene_t type; + static inline void AddRef(type val) {obs_scene_addref(val);} + static inline void Release(type val) {obs_scene_release(val);} +}; + +class OBSSceneItemRefClass { +public: + typedef obs_sceneitem_t type; + static inline void AddRef(type val) {obs_sceneitem_addref(val);} + static inline void Release(type val) {obs_sceneitem_release(val);} +}; + +typedef OBSRef OBSSource; +typedef OBSRef OBSScene; +typedef OBSRef OBSSceneItem; diff --git a/obs/qt-ptr-variant.hpp b/obs/qt-ptr-variant.hpp deleted file mode 100644 index 962af5e03..000000000 --- a/obs/qt-ptr-variant.hpp +++ /dev/null @@ -1,37 +0,0 @@ -/****************************************************************************** - Copyright (C) 2013 by Hugh Bailey - - This program is free software: you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation, either version 2 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with this program. If not, see . -******************************************************************************/ - -#include - -/* in case you're wondering, I made this code because qt static asserts would - * fail on forward pointers. so I was forced to make a hack */ - -struct PtrVariantDummy { - void *unused; -}; - -Q_DECLARE_METATYPE(PtrVariantDummy*); - -template static inline T VariantPtr(QVariant v) -{ - return (T)v.value(); -} - -static inline QVariant PtrVariant(void* ptr) -{ - return QVariant::fromValue((PtrVariantDummy*)ptr); -} diff --git a/obs/window-basic-main.cpp b/obs/window-basic-main.cpp index e07c8e20e..620c375aa 100644 --- a/obs/window-basic-main.cpp +++ b/obs/window-basic-main.cpp @@ -25,24 +25,24 @@ #include "window-namedialog.hpp" #include "window-basic-main.hpp" #include "qt-wrappers.hpp" -#include "qt-ptr-variant.hpp" #include "ui_OBSBasic.h" using namespace std; -obs_scene_t OBSBasic::GetCurrentScene() +Q_DECLARE_METATYPE(OBSScene); +Q_DECLARE_METATYPE(OBSSceneItem); + +OBSScene OBSBasic::GetCurrentScene() { QListWidgetItem *item = ui->scenes->currentItem(); - return item ? VariantPtr(item->data(Qt::UserRole)) : - nullptr; + return item ? item->data(Qt::UserRole).value() : nullptr; } -obs_sceneitem_t OBSBasic::GetCurrentSceneItem() +OBSSceneItem OBSBasic::GetCurrentSceneItem() { QListWidgetItem *item = ui->sources->currentItem(); - return item ? VariantPtr(item->data(Qt::UserRole)) : - nullptr; + return item ? item->data(Qt::UserRole).value() : nullptr; } void OBSBasic::AddScene(obs_source_t source) @@ -51,7 +51,7 @@ void OBSBasic::AddScene(obs_source_t source) obs_scene_t scene = obs_scene_fromsource(source); QListWidgetItem *item = new QListWidgetItem(QT_UTF8(name)); - item->setData(Qt::UserRole, PtrVariant(scene)); + item->setData(Qt::UserRole, QVariant::fromValue(OBSScene(scene))); ui->scenes->addItem(item); signal_handler_t handler = obs_source_signalhandler(source); @@ -77,13 +77,14 @@ void OBSBasic::RemoveScene(obs_source_t source) void OBSBasic::AddSceneItem(obs_sceneitem_t item) { - obs_scene_t scene = obs_sceneitem_getscene(item); + obs_scene_t scene = obs_sceneitem_getscene(item); obs_source_t source = obs_sceneitem_getsource(item); - const char *name = obs_source_getname(source); + const char *name = obs_source_getname(source); if (GetCurrentScene() == scene) { QListWidgetItem *listItem = new QListWidgetItem(QT_UTF8(name)); - listItem->setData(Qt::UserRole, PtrVariant(item)); + listItem->setData(Qt::UserRole, + QVariant::fromValue(OBSSceneItem(item))); ui->sources->insertItem(0, listItem); } @@ -100,7 +101,7 @@ void OBSBasic::RemoveSceneItem(obs_sceneitem_t item) QListWidgetItem *listItem = ui->sources->item(i); QVariant userData = listItem->data(Qt::UserRole); - if (item == VariantPtr(userData)) { + if (userData.value() == item) { delete listItem; break; } @@ -185,7 +186,7 @@ void OBSBasic::SourceAdded(void *data, calldata_t params) static_cast(data)->AddScene(source); } -void OBSBasic::SourceDestroyed(void *data, calldata_t params) +void OBSBasic::SourceRemoved(void *data, calldata_t params) { obs_source_t source = (obs_source_t)calldata_ptr(params, "source"); @@ -229,8 +230,8 @@ void OBSBasic::OBSInit() signal_handler_connect(obs_signalhandler(), "source-add", OBSBasic::SourceAdded, this); - signal_handler_connect(obs_signalhandler(), "source-destroy", - OBSBasic::SourceDestroyed, this); + signal_handler_connect(obs_signalhandler(), "source-remove", + OBSBasic::SourceRemoved, this); signal_handler_connect(obs_signalhandler(), "channel-change", OBSBasic::ChannelChanged, this); @@ -246,6 +247,8 @@ void OBSBasic::OBSInit() OBSBasic::~OBSBasic() { + ui->sources->clear(); + ui->scenes->clear(); obs_shutdown(); } @@ -364,7 +367,7 @@ void OBSBasic::on_scenes_itemChanged(QListWidgetItem *item) if (item) { obs_scene_t scene; - scene = VariantPtr(item->data(Qt::UserRole)); + scene = item->data(Qt::UserRole).value(); source = obs_scene_getsource(scene); UpdateSources(scene); } @@ -413,7 +416,7 @@ void OBSBasic::on_actionRemoveScene_triggered() return; QVariant userData = item->data(Qt::UserRole); - obs_scene_t scene = VariantPtr(userData); + obs_scene_t scene = userData.value(); obs_source_t source = obs_scene_getsource(scene); obs_source_remove(source); } diff --git a/obs/window-basic-main.hpp b/obs/window-basic-main.hpp index 3200c2ffa..2395c5c5e 100644 --- a/obs/window-basic-main.hpp +++ b/obs/window-basic-main.hpp @@ -32,8 +32,8 @@ class OBSBasic : public OBSMainWindow { private: std::unordered_map sourceSceneRefs; - obs_scene_t GetCurrentScene(); - obs_sceneitem_t GetCurrentSceneItem(); + OBSScene GetCurrentScene(); + OBSSceneItem GetCurrentSceneItem(); void AddSceneItem(obs_sceneitem_t item); void RemoveSceneItem(obs_sceneitem_t item); void AddScene(obs_source_t scene); @@ -45,7 +45,7 @@ private: static void SceneItemAdded(void *data, calldata_t params); static void SceneItemRemoved(void *data, calldata_t params); static void SourceAdded(void *data, calldata_t params); - static void SourceDestroyed(void *data, calldata_t params); + static void SourceRemoved(void *data, calldata_t params); static void ChannelChanged(void *data, calldata_t params); void ResizePreview(uint32_t cx, uint32_t cy); @@ -87,7 +87,7 @@ private slots: public: explicit OBSBasic(QWidget *parent = 0); - ~OBSBasic(); + virtual ~OBSBasic(); virtual void OBSInit() override;