Improve thread safety for scene items

Scene items previously were removed by calling obs_sceneitem_destroy,
but this proved to be a potential race condition where two different
threads could try to destroy the same scene item at the same time.

Instead of doing that, reference counting is now used on scene items,
and an explicit obs_sceneitem_remove function is used instead for item
removal, which sets a 'removed' variable to ensure it can only be called
exactly one time.

libobs/obs-display.c

@@ -53,8 +53,14 @@ void obs_display_destroy(obs_display_t display)
 
 obs_source_t obs_display_getsource(obs_display_t display, uint32_t channel)
 {
+	obs_source_t source;
 	assert(channel < MAX_CHANNELS);
-	return display->channels[channel];
+
+	source = display->channels[channel];
+	if (source)
+		obs_source_addref(source);
+
+	return source;
 }
 
 void obs_display_setsource(obs_display_t display, uint32_t channel,

libobs/obs-scene.c

@@ -18,14 +18,15 @@
 #include "graphics/math-defs.h"
 #include "obs-scene.h"
 
-static inline void signal_item_destroy(struct obs_scene_item *item,
-		struct calldata *params)
+static inline void signal_item_remove(struct obs_scene_item *item)
 {
-	calldata_setptr(params, "scene", item->parent);
-	calldata_setptr(params, "item", item);
+	struct calldata params = {0};
+	calldata_setptr(&params, "scene", item->parent);
+	calldata_setptr(&params, "item", item);
+
 	signal_handler_signal(item->parent->source->signals, "remove",
-			params);
-	calldata_clear(params);
+			&params);
+	calldata_free(&params);
 }
 
 static const char *scene_getname(const char *locale)
@@ -62,20 +63,18 @@ static void scene_destroy(void *data)
 {
 	struct obs_scene *scene = data;
 	struct obs_scene_item *item = scene->first_item;
-	struct calldata params = {0};
+
+	pthread_mutex_lock(&scene->mutex);
 
 	while (item) {
 		struct obs_scene_item *del_item = item;
 		item = item->next;
 
-		signal_item_destroy(del_item, &params);
-
-		if (del_item->source)
-			obs_source_release(del_item->source);
-		bfree(del_item);
+		obs_sceneitem_remove(del_item);
 	}
 
-	calldata_free(&params);
+	pthread_mutex_unlock(&scene->mutex);
+
 	pthread_mutex_destroy(&scene->mutex);
 	bfree(scene);
 }
@@ -94,6 +93,8 @@ static inline void detach_sceneitem(struct obs_scene_item *item)
 
 	if (item->next)
 		item->next->prev = item->prev;
+
+	item->parent = NULL;
 }
 
 static inline void attach_sceneitem(struct obs_scene_item *item,
@@ -124,7 +125,7 @@ static void scene_video_render(void *data)
 			struct obs_scene_item *del_item = item;
 			item = item->next;
 
-			obs_sceneitem_destroy(del_item);
+			obs_sceneitem_remove(del_item);
 			continue;
 		}
 
@@ -225,9 +226,8 @@ obs_sceneitem_t obs_scene_findsource(obs_scene_t scene, const char *name)
 
 	item = scene->first_item;
 	while (item) {
-		if (strcmp(item->source->name, name) == 0) {
+		if (strcmp(item->source->name, name) == 0)
 			break;
-		}
 
 		item = item->next;
 	}
@@ -247,10 +247,16 @@ void obs_scene_enum_items(obs_scene_t scene,
 
 	item = scene->first_item;
 	while (item) {
+		struct obs_scene_item *next = item->next;
+
+		obs_sceneitem_addref(item);
+
 		if (!callback(scene, item, param))
 			break;
 
-		item = item->next;
+		obs_sceneitem_release(item);
+
+		item = next;
 	}
 
 	pthread_mutex_unlock(&scene->mutex);
@@ -266,6 +272,7 @@ obs_sceneitem_t obs_scene_add(obs_scene_t scene, obs_source_t source)
 	item->source  = source;
 	item->visible = true;
 	item->parent  = scene;
+	item->ref     = 1;
 	vec2_set(&item->scale, 1.0f, 1.0f);
 
 	if (source)
@@ -294,30 +301,53 @@ obs_sceneitem_t obs_scene_add(obs_scene_t scene, obs_source_t source)
 	return item;
 }
 
-int obs_sceneitem_destroy(obs_sceneitem_t item)
+static void obs_sceneitem_destroy(obs_sceneitem_t item)
+{
+	if (item) {
+		if (item->source)
+			obs_source_release(item->source);
+		bfree(item);
+	}
+}
+
+int obs_sceneitem_addref(obs_sceneitem_t item)
+{
+	return ++item->ref;
+}
+
+int obs_sceneitem_release(obs_sceneitem_t item)
 {
 	int ref = 0;
 
-	if (item) {
-		obs_scene_t scene = item->parent;
-		struct calldata params = {0};
-
-		pthread_mutex_lock(&scene->mutex);
-
-		signal_item_destroy(item, &params);
-		detach_sceneitem(item);
-
-		if (item->source)
-			ref = obs_source_release(item->source);
-		bfree(item);
-
-		pthread_mutex_unlock(&scene->mutex);
-		calldata_free(&params);
+	if (item ) {
+		ref = --item->ref;
+		if (!ref)
+			obs_sceneitem_destroy(item);
 	}
 
 	return ref;
 }
 
+void obs_sceneitem_remove(obs_sceneitem_t item)
+{
+	obs_scene_t scene;
+
+	if (item->removed)
+		return;
+
+	item->removed = true;
+
+	signal_item_remove(item);
+
+	scene = item->parent;
+
+	pthread_mutex_lock(&scene->mutex);
+	detach_sceneitem(item);
+	pthread_mutex_unlock(&scene->mutex);
+
+	obs_sceneitem_release(item);
+}
+
 obs_scene_t obs_sceneitem_getscene(obs_sceneitem_t item)
 {
 	return item->parent;
@@ -353,6 +383,7 @@ void obs_sceneitem_setorder(obs_sceneitem_t item, enum order_movement movement)
 	struct obs_scene *scene = item->parent;
 
 	pthread_mutex_lock(&scene->mutex);
+	obs_scene_addref(scene);
 
 	detach_sceneitem(item);
 
@@ -377,6 +408,7 @@ void obs_sceneitem_setorder(obs_sceneitem_t item, enum order_movement movement)
 		attach_sceneitem(item, NULL);
 	}
 
+	obs_scene_release(scene);
 	pthread_mutex_unlock(&scene->mutex);
 }
 

libobs/obs-scene.h

@@ -23,6 +23,9 @@
 /* how obs scene! */
 
 struct obs_scene_item {
+	volatile int          ref;
+	volatile bool         removed;
+
 	struct obs_scene      *parent;
 	struct obs_source     *source;
 	bool                  visible;

libobs/obs.h

@@ -501,14 +501,16 @@ EXPORT void obs_scene_enum_items(obs_scene_t scene,
 /** Adds/creates a new scene item for a source */
 EXPORT obs_sceneitem_t obs_scene_add(obs_scene_t scene, obs_source_t source);
 
-/** Removes/destroys a scene item.  Returns the source reference counter
- * (if any) */
-EXPORT int obs_sceneitem_destroy(obs_sceneitem_t item);
+EXPORT int obs_sceneitem_addref(obs_sceneitem_t item);
+EXPORT int obs_sceneitem_release(obs_sceneitem_t item);
 
-/** Gets the scene parent associated with the scene item */
+/** Removes a scene item. */
+EXPORT void obs_sceneitem_remove(obs_sceneitem_t item);
+
+/** Gets the scene parent associated with the scene item. */
 EXPORT obs_scene_t obs_sceneitem_getscene(obs_sceneitem_t item);
 
-/** Gets the source of a scene item */
+/** Gets the source of a scene item. */
 EXPORT obs_source_t obs_sceneitem_getsource(obs_sceneitem_t item);
 
 /* Functions for gettings/setting specific oriantation of a scene item */

obs/window-basic-main.cpp

@@ -38,6 +38,13 @@ obs_scene_t OBSBasic::GetCurrentScene()
 		nullptr;
 }
 
+obs_sceneitem_t OBSBasic::GetCurrentSceneItem()
+{
+	QListWidgetItem *item = ui->sources->currentItem();
+	return item ? VariantPtr<obs_sceneitem_t>(item->data(Qt::UserRole)) :
+		nullptr;
+}
+
 void OBSBasic::AddScene(obs_source_t source)
 {
 	const char *name  = obs_source_getname(source);
@@ -101,8 +108,6 @@ void OBSBasic::RemoveSceneItem(obs_sceneitem_t item)
 	}
 
 	obs_source_t source = obs_sceneitem_getsource(item);
-	obs_source_addref(source);
-	obs_source_release(source);
 
 	int scenes = sourceSceneRefs[source] - 1;
 	if (scenes == 0) {
@@ -472,17 +477,14 @@ void OBSBasic::AddSource(obs_scene_t scene, const char *id)
 
 void OBSBasic::AddSourcePopupMenu(const QPoint &pos)
 {
-	QListWidgetItem *sel = ui->scenes->currentItem();
+	obs_scene_t scene = GetCurrentScene();
 	const char *type;
 	bool foundValues = false;
 	size_t idx = 0;
 
-	if (!sel)
+	if (!scene)
 		return;
 
-	obs_scene_t scene = VariantPtr<obs_scene_t>(sel->data(Qt::UserRole));
-	obs_scene_addref(scene);
-
 	QMenu popup;
 	while (obs_enum_input_types(idx++, &type)) {
 		const char *name = obs_source_getdisplayname(SOURCE_INPUT,
@@ -500,8 +502,6 @@ void OBSBasic::AddSourcePopupMenu(const QPoint &pos)
 		if (ret)
 			AddSource(scene, ret->data().toString().toUtf8());
 	}
-
-	obs_scene_release(scene);
 }
 
 void OBSBasic::on_actionAddSource_triggered()
@@ -511,23 +511,9 @@ void OBSBasic::on_actionAddSource_triggered()
 
 void OBSBasic::on_actionRemoveSource_triggered()
 {
-	QListWidgetItem *sel = ui->sources->currentItem();
-	if (!sel)
-		return;
-
-	obs_scene_t scene = GetCurrentScene();
-	if (!scene)
-		return;
-
-	gs_entercontext(obs_graphics());
-	obs_scene_addref(scene);
-
-	QVariant userData = sel->data(Qt::UserRole);
-	obs_sceneitem_t item = VariantPtr<obs_sceneitem_t>(userData);
-	obs_sceneitem_destroy(item);
-
-	obs_scene_release(scene);
-	gs_leavecontext();
+	obs_sceneitem_t item = GetCurrentSceneItem();
+	if (item)
+		obs_sceneitem_remove(item);
 }
 
 void OBSBasic::on_actionSourceProperties_triggered()

obs/window-basic-main.hpp

@@ -33,6 +33,7 @@ private:
 	std::unordered_map<obs_source_t, int> sourceSceneRefs;
 
 	obs_scene_t GetCurrentScene();
+	obs_sceneitem_t GetCurrentSceneItem();
 	void AddSceneItem(obs_sceneitem_t item);
 	void RemoveSceneItem(obs_sceneitem_t item);
 	void AddScene(obs_source_t scene);