From 874b9b86e28f6fd601dce23f30e90591b6109e8c Mon Sep 17 00:00:00 2001 From: WarmUpTill <19472752+WarmUpTill@users.noreply.github.com> Date: Mon, 16 Jun 2025 21:55:17 +0200 Subject: [PATCH] Refactor locking of macro segments This should avoid crashes when actions or conditions are performed in parallel to the main macro loop and will improve the UI responsiveness in some scenarios --- lib/macro/macro-segment.hpp | 2 +- lib/macro/macro.cpp | 45 ++++++++++++------- lib/utils/sync-helpers.cpp | 32 +++++++++++++ lib/utils/sync-helpers.hpp | 40 ++++++++++++++++- .../scripting/macro-segment-script-inline.hpp | 3 +- plugins/scripting/macro-segment-script.hpp | 3 +- plugins/video/macro-condition-video.cpp | 1 + 7 files changed, 107 insertions(+), 19 deletions(-) diff --git a/lib/macro/macro-segment.hpp b/lib/macro/macro-segment.hpp index 138e27c0..da4052db 100644 --- a/lib/macro/macro-segment.hpp +++ b/lib/macro/macro-segment.hpp @@ -19,7 +19,7 @@ namespace advss { class Macro; -class EXPORT MacroSegment { +class EXPORT MacroSegment : public Lockable { public: MacroSegment(Macro *m, bool supportsVariableValue); virtual ~MacroSegment() = default; diff --git a/lib/macro/macro.cpp b/lib/macro/macro.cpp index a3697b38..1a53abdb 100644 --- a/lib/macro/macro.cpp +++ b/lib/macro/macro.cpp @@ -173,7 +173,10 @@ static bool checkCondition(const std::shared_ptr &condition) static constexpr auto perfLogThreshold = 300ms; const auto startTime = std::chrono::high_resolution_clock::now(); - const bool conditionMatched = condition->CheckCondition(); + bool conditionMatched = false; + condition->WithLock([&condition, &conditionMatched]() { + conditionMatched = condition->CheckCondition(); + }); const auto endTime = std::chrono::high_resolution_clock::now(); const auto timeSpent = endTime - startTime; @@ -444,9 +447,12 @@ bool Macro::RunActionsHelper( } if (action->Enabled()) { action->LogAction(); + bool actionResult = false; + action->WithLock([&action, &actionResult]() { + actionResult = action->PerformAction(); + }); actionsExecutedSuccessfully = - actionsExecutedSuccessfully && - action->PerformAction(); + actionsExecutedSuccessfully && actionResult; } else { vblog(LOG_INFO, "skipping disabled action %s", action->GetId().c_str()); @@ -933,9 +939,12 @@ bool Macro::Load(obs_data_t *obj) auto newEntry = MacroConditionFactory::Create(id, this); if (newEntry) { _conditions.emplace_back(newEntry); - auto c = _conditions.back().get(); - c->Load(arrayObj); - c->ValidateLogicSelection(root, Name().c_str()); + auto condition = _conditions.back().get(); + condition->WithLock([&]() { + condition->Load(arrayObj); + condition->ValidateLogicSelection( + root, Name().c_str()); + }); } else { blog(LOG_WARNING, "discarding condition entry with unknown id (%s) for macro %s", @@ -948,12 +957,15 @@ bool Macro::Load(obs_data_t *obj) OBSDataArrayAutoRelease actions = obs_data_get_array(obj, "actions"); count = obs_data_array_count(actions); for (size_t i = 0; i < count; i++) { - OBSDataAutoRelease array_obj = obs_data_array_item(actions, i); - std::string id = obs_data_get_string(array_obj, "id"); + OBSDataAutoRelease arrayObj = obs_data_array_item(actions, i); + std::string id = obs_data_get_string(arrayObj, "id"); auto newEntry = MacroActionFactory::Create(id, this); if (newEntry) { _actions.emplace_back(newEntry); - _actions.back()->Load(array_obj); + auto action = _actions.back().get(); + action->WithLock([action, &arrayObj]() { + action->Load(arrayObj); + }); } else { blog(LOG_WARNING, "discarding action entry with unknown id (%s) for macro %s", @@ -966,13 +978,16 @@ bool Macro::Load(obs_data_t *obj) obs_data_get_array(obj, "elseActions"); count = obs_data_array_count(elseActions); for (size_t i = 0; i < count; i++) { - OBSDataAutoRelease array_obj = + OBSDataAutoRelease arrayObj = obs_data_array_item(elseActions, i); - std::string id = obs_data_get_string(array_obj, "id"); + std::string id = obs_data_get_string(arrayObj, "id"); auto newEntry = MacroActionFactory::Create(id, this); if (newEntry) { _elseActions.emplace_back(newEntry); - _elseActions.back()->Load(array_obj); + auto action = _actions.back().get(); + action->WithLock([action, &arrayObj]() { + action->Load(arrayObj); + }); } else { blog(LOG_WARNING, "discarding elseAction entry with unknown id (%s) for macro %s", @@ -989,13 +1004,13 @@ bool Macro::Load(obs_data_t *obj) bool Macro::PostLoad() { for (auto &c : _conditions) { - c->PostLoad(); + c->WithLock([c]() { c->PostLoad(); }); } for (auto &a : _actions) { - a->PostLoad(); + a->WithLock([a]() { a->PostLoad(); }); } for (auto &a : _elseActions) { - a->PostLoad(); + a->WithLock([a]() { a->PostLoad(); }); } return true; } diff --git a/lib/utils/sync-helpers.cpp b/lib/utils/sync-helpers.cpp index 8edff94e..71c59cf5 100644 --- a/lib/utils/sync-helpers.cpp +++ b/lib/utils/sync-helpers.cpp @@ -20,4 +20,36 @@ std::unique_lock *GetLoopLock() return GetSwitcherLoopLock(); } +PerInstanceMutex::PerInstanceMutex() {} + +PerInstanceMutex::~PerInstanceMutex(){}; + +PerInstanceMutex::PerInstanceMutex(const PerInstanceMutex &) {} + +PerInstanceMutex &PerInstanceMutex::operator=(const PerInstanceMutex &) +{ + return *this; +} + +PerInstanceMutex::operator std::mutex &() +{ + return _mtx; +} + +PerInstanceMutex::operator const std::mutex &() const +{ + return _mtx; +} + +std::lock_guard Lockable::Lock() +{ + return std::lock_guard(_mtx); +} + +void Lockable::WithLock(const std::function &func) +{ + const auto lock = Lock(); + func(); +} + } // namespace advss diff --git a/lib/utils/sync-helpers.hpp b/lib/utils/sync-helpers.hpp index 4c88c51a..df531bc1 100644 --- a/lib/utils/sync-helpers.hpp +++ b/lib/utils/sync-helpers.hpp @@ -1,6 +1,8 @@ #pragma once #include "export-symbol-helper.hpp" +#include +#include #include namespace advss { @@ -10,10 +12,46 @@ namespace advss { if (_loading || !_entryData) { \ return; \ } \ - auto lock = LockContext(); + auto lock = _entryData->Lock(); [[nodiscard]] EXPORT std::mutex *GetMutex(); [[nodiscard]] EXPORT std::lock_guard LockContext(); [[nodiscard]] EXPORT std::unique_lock *GetLoopLock(); +#ifdef _MSC_VER +#pragma warning(push) +#pragma warning(disable : 4251) +#endif + +// std::mutex wrapper with no-op copy constructor and assignment operator +class EXPORT PerInstanceMutex { +public: + PerInstanceMutex(); + ~PerInstanceMutex(); + PerInstanceMutex(const PerInstanceMutex &); + PerInstanceMutex &operator=(const PerInstanceMutex &); + + operator std::mutex &(); + operator const std::mutex &() const; + +private: + std::mutex _mtx; +}; + +#ifdef _MSC_VER +#pragma warning(pop) +#endif + +class EXPORT Lockable { +public: + Lockable() = default; + virtual ~Lockable() = default; + + [[nodiscard]] std::lock_guard Lock(); + void WithLock(const std::function &func); + +private: + PerInstanceMutex _mtx; +}; + } // namespace advss diff --git a/plugins/scripting/macro-segment-script-inline.hpp b/plugins/scripting/macro-segment-script-inline.hpp index 88a14b3b..f94cb391 100644 --- a/plugins/scripting/macro-segment-script-inline.hpp +++ b/plugins/scripting/macro-segment-script-inline.hpp @@ -1,13 +1,14 @@ #pragma once #include "file-selection.hpp" #include "inline-script.hpp" +#include "sync-helpers.hpp" #include "variable-text-edit.hpp" #include namespace advss { -class MacroSegmentScriptInline { +class MacroSegmentScriptInline : public Lockable { public: InlineScript::Type GetType() const { return _script.GetType(); } void SetType(InlineScript::Type); diff --git a/plugins/scripting/macro-segment-script.hpp b/plugins/scripting/macro-segment-script.hpp index 8c9a2796..e1ea1325 100644 --- a/plugins/scripting/macro-segment-script.hpp +++ b/plugins/scripting/macro-segment-script.hpp @@ -1,6 +1,7 @@ #pragma once #include "duration-control.hpp" #include "macro-script-handler.hpp" +#include "sync-helpers.hpp" #include #include @@ -11,7 +12,7 @@ class Macro; class MacroAction; class MacroCondition; -class MacroSegmentScript { +class MacroSegmentScript : public Lockable { public: MacroSegmentScript(obs_data_t *defaultSettings, const std::string &propertiesSignalName, diff --git a/plugins/video/macro-condition-video.cpp b/plugins/video/macro-condition-video.cpp index edd3862b..e83ab9cf 100644 --- a/plugins/video/macro-condition-video.cpp +++ b/plugins/video/macro-condition-video.cpp @@ -621,6 +621,7 @@ OCREdit::OCREdit(QWidget *parent, PreviewDialog *previewDialog, _entryData->_ocrParameters.GetCustomConfigFile()); }); QWidget::connect(_reloadConfig, &QPushButton::clicked, [this](bool) { + GUARD_LOADING_AND_LOCK(); _entryData->_ocrParameters.EnableCustomConfig(true); _previewDialog->OCRParametersChanged( _entryData->_ocrParameters);