Merge pull request #365 from Palakis/bugfix/reordersceneitems-crash

ReorderSceneItems: fix crash + refactor Utils

src/Utils.cpp

@@ -173,23 +173,39 @@ obs_data_t* Utils::GetSceneItemData(obs_sceneitem_t* item) {
 	return data;
 }
 
-obs_sceneitem_t* Utils::GetSceneItemFromItem(obs_source_t* source, obs_data_t* item) {
-        OBSSceneItem sceneItem;
-        if (obs_data_has_user_value(item, "id")) {
-                sceneItem = GetSceneItemFromId(source, obs_data_get_int(item, "id"));
-                if (obs_data_has_user_value(item, "name") &&
-                   (QString)obs_source_get_name(obs_sceneitem_get_source(sceneItem)) !=
-                   (QString)obs_data_get_string(item, "name")) {
-                        return nullptr;
-                }
-        }
-        else if (obs_data_has_user_value(item, "name")) {
-                sceneItem = GetSceneItemFromName(source, obs_data_get_string(item, "name"));
-        }
-        return sceneItem;
+obs_sceneitem_t* Utils::GetSceneItemFromItem(obs_scene_t* scene, obs_data_t* itemInfo) {
+	if (!scene) {
+		return nullptr;
+	}
+
+	OBSDataItemAutoRelease idInfoItem = obs_data_item_byname(itemInfo, "id");
+	int id = obs_data_item_get_int(idInfoItem);
+
+	OBSDataItemAutoRelease nameInfoItem = obs_data_item_byname(itemInfo, "name");
+	const char* name = obs_data_item_get_string(nameInfoItem);
+
+	if (idInfoItem) {
+		obs_sceneitem_t* sceneItem = GetSceneItemFromId(scene, id);
+		obs_source_t* sceneItemSource = obs_sceneitem_get_source(sceneItem);
+
+		QString sceneItemName = obs_source_get_name(sceneItemSource);
+		if (nameInfoItem && (QString(name) != sceneItemName)) {
+			return nullptr;
+		}
+
+		return sceneItem;
+	} else if (nameInfoItem) {
+		return GetSceneItemFromName(scene, name);
+	}
+
+	return nullptr;
 }
 
-obs_sceneitem_t* Utils::GetSceneItemFromName(obs_source_t* source, QString name) {
+obs_sceneitem_t* Utils::GetSceneItemFromName(obs_scene_t* scene, QString name) {
+	if (!scene) {
+		return nullptr;
+	}
+
 	struct current_search {
 		QString query;
 		obs_sceneitem_t* result;
@@ -199,11 +215,6 @@ obs_sceneitem_t* Utils::GetSceneItemFromName(obs_source_t* source, QString name)
 	current_search search;
 	search.query = name;
 	search.result = nullptr;
-	search.enumCallback = nullptr;
-
-	OBSScene scene = obs_scene_from_source(source);
-	if (!scene)
-		return nullptr;
 
 	search.enumCallback = [](
 			obs_scene_t* scene,
@@ -236,10 +247,13 @@ obs_sceneitem_t* Utils::GetSceneItemFromName(obs_source_t* source, QString name)
 	return search.result;
 }
 
-// TODO refactor this to unify it with GetSceneItemFromName
-obs_sceneitem_t* Utils::GetSceneItemFromId(obs_source_t* source, size_t id) {
+obs_sceneitem_t* Utils::GetSceneItemFromId(obs_scene_t* scene, int64_t id) {
+	if (!scene) {
+		return nullptr;
+	}
+
 	struct current_search {
-		size_t query;
+		int query;
 		obs_sceneitem_t* result;
 		bool (*enumCallback)(obs_scene_t*, obs_sceneitem_t*, void*);
 	};
@@ -247,21 +261,16 @@ obs_sceneitem_t* Utils::GetSceneItemFromId(obs_source_t* source, size_t id) {
 	current_search search;
 	search.query = id;
 	search.result = nullptr;
-	search.enumCallback = nullptr;
-
-	OBSScene scene = obs_scene_from_source(source);
-	if (!scene)
-		return nullptr;
 
 	search.enumCallback = [](
-			obs_scene_t* scene,
-			obs_sceneitem_t* currentItem,
-			void* param)
+		obs_scene_t* scene,
+		obs_sceneitem_t* currentItem,
+		void* param)
 	{
 		current_search* search = reinterpret_cast<current_search*>(param);
 
 		if (obs_sceneitem_is_group(currentItem)) {
-			obs_sceneitem_group_enum_items(currentItem, search->enumCallback, param);
+			obs_sceneitem_group_enum_items(currentItem, search->enumCallback, search);
 			if (search->result) {
 				return false;
 			}
@@ -319,17 +328,19 @@ obs_source_t* Utils::GetTransitionFromName(QString searchName) {
 	return foundTransition;
 }
 
-obs_source_t* Utils::GetSceneFromNameOrCurrent(QString sceneName) {
+obs_scene_t* Utils::GetSceneFromNameOrCurrent(QString sceneName) {
 	// Both obs_frontend_get_current_scene() and obs_get_source_by_name()
-	// do addref on the return source, so no need to use an OBSSource helper
-	obs_source_t* scene = nullptr;
+	// increase the returned source's refcount
+	OBSSourceAutoRelease sceneSource = nullptr;
 
-	if (sceneName.isEmpty() || sceneName.isNull())
-		scene = obs_frontend_get_current_scene();
-	else
-		scene = obs_get_source_by_name(sceneName.toUtf8());
+	if (sceneName.isEmpty() || sceneName.isNull()) {
+		sceneSource = obs_frontend_get_current_scene();
+	}
+	else {
+		sceneSource = obs_get_source_by_name(sceneName.toUtf8());
+	}
 
-	return scene;
+	return obs_scene_from_source(sceneSource);
 }
 
 obs_data_array_t* Utils::GetScenes() {

src/Utils.h

@@ -36,12 +36,13 @@ class Utils {
 	static obs_data_array_t* StringListToArray(char** strings, const char* key);
 	static obs_data_array_t* GetSceneItems(obs_source_t* source);
 	static obs_data_t* GetSceneItemData(obs_sceneitem_t* item);
-	static obs_sceneitem_t* GetSceneItemFromName(
-		obs_source_t* source, QString name);
-        static obs_sceneitem_t* GetSceneItemFromId(obs_source_t* source, size_t id);
-	static obs_sceneitem_t* GetSceneItemFromItem(obs_source_t* source, obs_data_t* item);
-	static obs_source_t* GetTransitionFromName(QString transitionName);
-	static obs_source_t* GetSceneFromNameOrCurrent(QString sceneName);
+
+	// These two functions support nested lookup into groups
+	static obs_sceneitem_t* GetSceneItemFromName(obs_scene_t* scene, QString name);
+	static obs_sceneitem_t* GetSceneItemFromId(obs_scene_t* scene, int64_t id);
+
+	static obs_sceneitem_t* GetSceneItemFromItem(obs_scene_t* scene, obs_data_t* item);
+	static obs_scene_t* GetSceneFromNameOrCurrent(QString sceneName);
 	static obs_data_t* GetSceneItemPropertiesData(obs_sceneitem_t* item);
 
 	static obs_data_array_t* GetSourceFiltersList(obs_source_t* source, bool includeSettings);
@@ -53,9 +54,8 @@ class Utils {
 
 	// TODO contribute a proper frontend API method for this to OBS and remove this hack
 	static QSpinBox* GetTransitionDurationControl();
-
 	static int GetTransitionDuration(obs_source_t* transition);
-
+	static obs_source_t* GetTransitionFromName(QString transitionName);
 	static bool SetTransitionByName(QString transitionName);
 
 	static QString OBSVersionString();
@@ -71,9 +71,11 @@ class Utils {
 
 	static QString ParseDataToQueryString(obs_data_t* data);
 	static obs_hotkey_t* FindHotkeyByName(QString name);
+
 	static bool ReplayBufferEnabled();
 	static void StartReplayBuffer();
 	static bool IsRPHotkeySet();
+
 	static const char* GetFilenameFormatting();
 	static bool SetFilenameFormatting(const char* filenameFormatting);
 };

src/WSRequestHandler_SceneItems.cpp

@@ -49,13 +49,12 @@ HandlerResponse WSRequestHandler::HandleGetSceneItemProperties(WSRequestHandler*
 	}
 
 	QString sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
 
-	OBSSceneItemAutoRelease sceneItem =
-		Utils::GetSceneItemFromName(scene, itemName);
+	OBSSceneItemAutoRelease sceneItem = Utils::GetSceneItemFromName(scene, itemName);
 	if (!sceneItem) {
 		return req->SendErrorResponse("specified scene item doesn't exist");
 	}
@@ -105,7 +104,7 @@ HandlerResponse WSRequestHandler::HandleSetSceneItemProperties(WSRequestHandler*
 	}
 
 	QString sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -283,7 +282,7 @@ HandlerResponse WSRequestHandler::HandleResetSceneItem(WSRequestHandler* req) {
 	}
 
 	const char* sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -329,7 +328,7 @@ HandlerResponse WSRequestHandler::HandleSetSceneItemRender(WSRequestHandler* req
 	}
 
 	const char* sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -371,7 +370,7 @@ HandlerResponse WSRequestHandler::HandleSetSceneItemPosition(WSRequestHandler* r
 	}
 
 	QString sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene could not be found");
 	}
@@ -419,7 +418,7 @@ HandlerResponse WSRequestHandler::HandleSetSceneItemTransform(WSRequestHandler*
 	}
 
 	QString sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -471,7 +470,7 @@ HandlerResponse WSRequestHandler::HandleSetSceneItemCrop(WSRequestHandler* req)
 	}
 
 	QString sceneName = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -511,7 +510,7 @@ HandlerResponse WSRequestHandler::HandleDeleteSceneItem(WSRequestHandler* req) {
 	}
 
 	const char* sceneName = obs_data_get_string(req->data, "scene");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -564,13 +563,13 @@ HandlerResponse WSRequestHandler::HandleDuplicateSceneItem(WSRequestHandler* req
 	}
 
 	const char* fromSceneName = obs_data_get_string(req->data, "fromScene");
-	OBSSourceAutoRelease fromScene = Utils::GetSceneFromNameOrCurrent(fromSceneName);
+	OBSScene fromScene = Utils::GetSceneFromNameOrCurrent(fromSceneName);
 	if (!fromScene) {
 		return req->SendErrorResponse("requested fromScene doesn't exist");
 	}
 
 	const char* toSceneName = obs_data_get_string(req->data, "toScene");
-	OBSSourceAutoRelease toScene = Utils::GetSceneFromNameOrCurrent(toSceneName);
+	OBSScene toScene = Utils::GetSceneFromNameOrCurrent(toSceneName);
 	if (!toScene) {
 		return req->SendErrorResponse("requested toScene doesn't exist");
 	}
@@ -586,7 +585,7 @@ HandlerResponse WSRequestHandler::HandleDuplicateSceneItem(WSRequestHandler* req
 	data.referenceItem = referenceItem;
 
 	obs_enter_graphics();
-	obs_scene_atomic_update(obs_scene_from_source(toScene), DuplicateSceneItem, &data);
+	obs_scene_atomic_update(toScene, DuplicateSceneItem, &data);
 	obs_leave_graphics();
 
 	obs_sceneitem_t *newItem = data.newItem;
@@ -600,7 +599,7 @@ HandlerResponse WSRequestHandler::HandleDuplicateSceneItem(WSRequestHandler* req
 
 	OBSDataAutoRelease responseData = obs_data_create();
 	obs_data_set_obj(responseData, "item", itemData);
-	obs_data_set_string(responseData, "scene", obs_source_get_name(toScene));
+	obs_data_set_string(responseData, "scene", obs_source_get_name(obs_scene_get_source(toScene)));
 
 	return req->SendOKResponse(responseData);
 }

src/WSRequestHandler_Scenes.cpp

@@ -94,7 +94,7 @@ HandlerResponse WSRequestHandler::HandleGetSceneList(WSRequestHandler* req) {
 */
 HandlerResponse WSRequestHandler::HandleReorderSceneItems(WSRequestHandler* req) {
 	QString sceneName = obs_data_get_string(req->data, "scene");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(sceneName);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(sceneName);
 	if (!scene) {
 		return req->SendErrorResponse("requested scene doesn't exist");
 	}
@@ -104,37 +104,46 @@ HandlerResponse WSRequestHandler::HandleReorderSceneItems(WSRequestHandler* req)
 		return req->SendErrorResponse("sceneItem order not specified");
 	}
 
-	size_t count = obs_data_array_count(items);
+	struct reorder_context {
+		obs_data_array_t* items;
+		bool success;
+		QString errorMessage;
+	};
 
-	std::vector<obs_sceneitem_t*> newOrder;
-	newOrder.reserve(count);
+	struct reorder_context ctx;
+	ctx.success = false;
+	ctx.items = items;
 
-	for (size_t i = 0; i < count; ++i) {
-		OBSDataAutoRelease item = obs_data_array_item(items, i);
+	obs_scene_atomic_update(scene, [](void* param, obs_scene_t* scene) {
+		auto ctx = reinterpret_cast<struct reorder_context*>(param);
 
-		OBSSceneItemAutoRelease sceneItem = Utils::GetSceneItemFromItem(scene, item);
-		obs_sceneitem_release(sceneItem); // ref dec
+		QVector<struct obs_sceneitem_order_info> orderList;
+		struct obs_sceneitem_order_info info;
 
-		if (!sceneItem) {
-			return req->SendErrorResponse("Invalid sceneItem id or name specified");
-		}
-		
-		for (size_t j = 0; j <= i; ++j) {
-			if (sceneItem == newOrder[j]) {
-				return req->SendErrorResponse("Duplicate sceneItem in specified order");
+		size_t itemCount = obs_data_array_count(ctx->items);
+		for (int i = 0; i < itemCount; i++) {
+			OBSDataAutoRelease item = obs_data_array_item(ctx->items, i);
+
+			OBSSceneItemAutoRelease sceneItem = Utils::GetSceneItemFromItem(scene, item);
+			if (!sceneItem) {
+				ctx->success = false;
+				ctx->errorMessage = "Invalid sceneItem id or name specified";
+				return;
 			}
+
+			info.group = nullptr;
+			info.item = sceneItem;
+			orderList.insert(0, info);
 		}
 
-		newOrder.push_back(sceneItem);
-	}
+		ctx->success = obs_scene_reorder_items2(scene, orderList.data(), orderList.size());
+		if (!ctx->success) {
+			ctx->errorMessage = "Invalid sceneItem order";
+		}
+	}, &ctx);
 
-	bool success = obs_scene_reorder_items(obs_scene_from_source(scene), newOrder.data(), count);
-	if (!success) {
-		return req->SendErrorResponse("Invalid sceneItem order");
-	}
-
-	for (auto const& item: newOrder) {
-		obs_sceneitem_release(item);
+	if (!ctx.success) {
+		return req->SendErrorResponse(ctx.errorMessage);
 	}
 
 	return req->SendOKResponse();

src/WSRequestHandler_StudioMode.cpp

@@ -69,12 +69,12 @@ HandlerResponse WSRequestHandler::HandleSetPreviewScene(WSRequestHandler* req) {
 	}
 
 	const char* scene_name = obs_data_get_string(req->data, "scene-name");
-	OBSSourceAutoRelease scene = Utils::GetSceneFromNameOrCurrent(scene_name);
+	OBSScene scene = Utils::GetSceneFromNameOrCurrent(scene_name);
 	if (!scene) {
 		return req->SendErrorResponse("specified scene doesn't exist");
 	}
 
-	obs_frontend_set_current_preview_scene(scene);
+	obs_frontend_set_current_preview_scene(obs_scene_get_source(scene));
 	return req->SendOKResponse();
 }
 

src/obs-websocket.cpp

@@ -18,6 +18,7 @@ with this program. If not, see <https://www.gnu.org/licenses/>
 
 #include <obs-module.h>
 #include <obs-frontend-api.h>
+#include <obs-data.h>
 
 #include <QtCore/QTimer>
 #include <QtWidgets/QAction>