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

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

lib/macro/macro.cpp

@@ -173,7 +173,10 @@ static bool checkCondition(const std::shared_ptr<MacroCondition> &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;
 }

lib/utils/sync-helpers.cpp

@@ -20,4 +20,36 @@ std::unique_lock<std::mutex> *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<std::mutex> Lockable::Lock()
+{
+	return std::lock_guard<std::mutex>(_mtx);
+}
+
+void Lockable::WithLock(const std::function<void()> &func)
+{
+	const auto lock = Lock();
+	func();
+}
+
 } // namespace advss

lib/utils/sync-helpers.hpp

@@ -1,6 +1,8 @@
 #pragma once
 #include "export-symbol-helper.hpp"
 
+#include <functional>
+#include <memory>
 #include <mutex>
 
 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<std::mutex> LockContext();
 [[nodiscard]] EXPORT std::unique_lock<std::mutex> *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<std::mutex> Lock();
+	void WithLock(const std::function<void()> &func);
+
+private:
+	PerInstanceMutex _mtx;
+};
+
 } // namespace advss

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 <QLayout>
 
 namespace advss {
 
-class MacroSegmentScriptInline {
+class MacroSegmentScriptInline : public Lockable {
 public:
 	InlineScript::Type GetType() const { return _script.GetType(); }
 	void SetType(InlineScript::Type);

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 <atomic>
 #include <obs-data.h>
@@ -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,

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