chore: Refactor URL source thread and remove unused variables (#103)

* chore: Refactor URL source thread and remove unused variables * chore: Remove unnecessary code and fix memory leak in URL source rendering * chore: Refactor URL source thread and fix memory leak * chore: Use atomic bool for curl_thread_run in URL source data * empty commit trigger rebuild * chore: Refactor URL source thread and remove unnecessary code * Refactor URL source thread and initialize url_source_data explicitly * Refactor URL source thread and use unique_lock instead of lock_guard for output_mapping_mutex
locaal-ai · Jul 7, 2024 · 85a7187 · 85a7187
1 parent d627e45
commit 85a7187
Show file tree

Hide file tree

Showing 7 changed files with 103 additions and 97 deletions.
diff --git a/src/request-data.cpp b/src/request-data.cpp
@@ -180,14 +180,17 @@ void put_inputs_on_json(url_source_request_data *request_data, CURL *curl,
 		std::string source_name = get_source_name_without_prefix(input.source);
 		// Check if the source is a text source
 		if (is_obs_source_text(source_name)) {
-			// Get text from OBS text source
-			obs_data_t *sourceSettings = obs_source_get_settings(
-				obs_get_source_by_name(source_name.c_str()));
-			const char *text = obs_data_get_string(sourceSettings, "text");
-			obs_data_release(sourceSettings);
+			obs_source_t *source = obs_get_source_by_name(source_name.c_str());
 			std::string textStr;
-			if (text != NULL) {
-				textStr = text;
+			if (source) {
+				// Get text from OBS text source
+				obs_data_t *sourceSettings = obs_source_get_settings(source);
+				const char *text = obs_data_get_string(sourceSettings, "text");
+				obs_data_release(sourceSettings);
+				obs_source_release(source);
+				if (text != NULL) {
+					textStr = text;
+				}
 			}
 
 			if (textStr.empty()) {
@@ -207,7 +210,7 @@ void put_inputs_on_json(url_source_request_data *request_data, CURL *curl,
 					// check if the header is Content-Type case insensitive using regex
 					if (std::regex_search(header.first, header_regex) &&
 					    std::regex_search(header.second, header_value_regex)) {
-						nlohmann::json tmp = text;
+						nlohmann::json tmp = textStr;
 						textStr = tmp.dump();
 						// remove '"' from the beginning and end of the string
 						textStr = textStr.substr(1, textStr.size() - 2);

diff --git a/src/ui/outputmapping.cpp b/src/ui/outputmapping.cpp
@@ -16,8 +16,8 @@ font-size: 48px;
 const std::string default_template_string = R"({{output}})";
 } // namespace
 
-OutputMapping::OutputMapping(output_mapping_data *mapping_data_in,
-			     std::function<void()> update_handler_in, QWidget *parent)
+OutputMapping::OutputMapping(const output_mapping_data &mapping_data_in,
+			     update_handler_t update_handler_in, QWidget *parent)
 	: QDialog(parent),
 	  ui(new Ui::OutputMapping),
 	  mapping_data(mapping_data_in),
@@ -49,11 +49,11 @@ OutputMapping::OutputMapping(output_mapping_data *mapping_data_in,
 				ui->checkBox_unhide_Source->blockSignals(true);
 				// set the plainTextEdit_template and plainTextEdit_cssProps to the template_string and css_props of the selected row
 				ui->plainTextEdit_template->setPlainText(
-					this->mapping_data->mappings[row].template_string.c_str());
+					this->mapping_data.mappings[row].template_string.c_str());
 				ui->plainTextEdit_cssProps->setPlainText(
-					this->mapping_data->mappings[row].css_props.c_str());
+					this->mapping_data.mappings[row].css_props.c_str());
 				ui->checkBox_unhide_Source->setChecked(
-					this->mapping_data->mappings[row].unhide_output_source);
+					this->mapping_data.mappings[row].unhide_output_source);
 				ui->plainTextEdit_template->blockSignals(false);
 				ui->plainTextEdit_cssProps->blockSignals(false);
 				ui->checkBox_unhide_Source->blockSignals(false);
@@ -71,23 +71,23 @@ OutputMapping::OutputMapping(output_mapping_data *mapping_data_in,
 		// get the selected row
 		const auto row = ui->tableView->currentIndex().row();
 		// set the template_string of the selected row to the plainTextEdit_template text
-		this->mapping_data->mappings[row].template_string =
+		this->mapping_data.mappings[row].template_string =
 			ui->plainTextEdit_template->toPlainText().toStdString();
 		// call update_handler
-		this->update_handler();
+		this->update_handler(this->mapping_data);
 	});
 	connect(ui->plainTextEdit_cssProps, &QPlainTextEdit::textChanged, [this]() {
 		// get the selected row
 		const auto row = ui->tableView->currentIndex().row();
 		// set the css_props of the selected row to the plainTextEdit_cssProps text
-		this->mapping_data->mappings[row].css_props =
+		this->mapping_data.mappings[row].css_props =
 			ui->plainTextEdit_cssProps->toPlainText().toStdString();
 		// call update_handler
-		this->update_handler();
+		this->update_handler(this->mapping_data);
 	});
 
 	// populate the model with the mapping data
-	for (const auto &mapping : mapping_data->mappings) {
+	for (const auto &mapping : mapping_data.mappings) {
 		model.appendRow(QList<QStandardItem *>() << new QStandardItem(mapping.name.c_str())
 							 << new QStandardItem(""));
 		QComboBox *comboBox = createSourcesComboBox();
@@ -115,7 +115,7 @@ OutputMapping::OutputMapping(output_mapping_data *mapping_data_in,
 	connect(&model, &QStandardItemModel::itemChanged, [this](QStandardItem *item) {
 		// update mapping name
 		if (item->column() == 0) {
-			this->mapping_data->mappings[item->row()].name = item->text().toStdString();
+			this->mapping_data.mappings[item->row()].name = item->text().toStdString();
 		}
 	});
 }
@@ -151,9 +151,9 @@ QComboBox *OutputMapping::createSourcesComboBox()
 			output_name_without_prefix = output_name.substr(8);
 		}
 		// set the css_props of the selected row to the plainTextEdit_cssProps text
-		this->mapping_data->mappings[row].output_source = output_name_without_prefix;
+		this->mapping_data.mappings[row].output_source = output_name_without_prefix;
 		// call update_handler
-		this->update_handler();
+		this->update_handler(this->mapping_data);
 	});
 
 	return comboBox;
@@ -168,19 +168,19 @@ void OutputMapping::addMapping()
 	// set comboBox as the index widget of the last item in the model
 	ui->tableView->setIndexWidget(model.index(model.rowCount() - 1, 1), comboBox);
 	// add a new mapping to the mapping_data
-	this->mapping_data->mappings.push_back(output_mapping{
+	this->mapping_data.mappings.push_back(output_mapping{
 		"Mapping", none_internal_rendering, default_template_string, default_css_props});
 	// call update_handler
-	this->update_handler();
+	this->update_handler(this->mapping_data);
 }
 
 void OutputMapping::removeMapping()
 {
 	// remove the mapping from the mapping_data
-	this->mapping_data->mappings.erase(this->mapping_data->mappings.begin() +
-					   ui->tableView->currentIndex().row());
+	this->mapping_data.mappings.erase(this->mapping_data.mappings.begin() +
+					  ui->tableView->currentIndex().row());
 	// remove row from model
 	model.removeRow(ui->tableView->currentIndex().row());
 	// call update_handler
-	this->update_handler();
+	this->update_handler(this->mapping_data);
 }
diff --git a/src/ui/outputmapping.h b/src/ui/outputmapping.h
@@ -12,12 +12,13 @@ namespace Ui {
 class OutputMapping;
 }
 
+typedef std::function<void(const output_mapping_data &)> update_handler_t;
 class OutputMapping : public QDialog {
 	Q_OBJECT
 
 public:
-	explicit OutputMapping(output_mapping_data *mapping_data_in,
-			       std::function<void()> update_handler, QWidget *parent = nullptr);
+	explicit OutputMapping(const output_mapping_data &mapping_data_in,
+			       update_handler_t update_handler, QWidget *parent = nullptr);
 	~OutputMapping();
 
 	OutputMapping(const OutputMapping &) = delete;
@@ -26,9 +27,9 @@ class OutputMapping : public QDialog {
 private:
 	Ui::OutputMapping *ui;
 	QStandardItemModel model;
-	output_mapping_data *mapping_data;
+	output_mapping_data mapping_data;
 	QComboBox *createSourcesComboBox();
-	std::function<void()> update_handler;
+	update_handler_t update_handler;
 
 private slots:
 	void addMapping();

diff --git a/src/url-source-callbacks.cpp b/src/url-source-callbacks.cpp
@@ -132,12 +132,6 @@ void render_internal(const std::string &text, struct url_source_data *usd,
 	uint32_t width = usd->render_width;
 	uint32_t height = 0;
 
-	if (usd->frame.data[0] != nullptr) {
-		// Free the old render buffer
-		bfree(usd->frame.data[0]);
-		usd->frame.data[0] = nullptr;
-	}
-
 	// render the text with QTextDocument
 	render_text_with_qtextdocument(text, width, height, &renderBuffer, mapping.css_props);
 	// Update the frame
@@ -148,6 +142,9 @@ void render_internal(const std::string &text, struct url_source_data *usd,
 
 	// Send the frame
 	obs_source_output_video(usd->source, &usd->frame);
+
+	bfree(usd->frame.data[0]);
+	usd->frame.data[0] = nullptr;
 }
 
 std::string prepare_text_from_template(const output_mapping &mapping,
@@ -192,15 +189,24 @@ std::string prepare_text_from_template(const output_mapping &mapping,
 
 void output_with_mapping(const request_data_handler_response &response, struct url_source_data *usd)
 {
-	if (usd->output_mapping_data.mappings.empty()) {
+	std::vector<output_mapping> mappings;
+
+	{
+		// lock the mapping mutex to get a local copy of the mappings
+		std::unique_lock<std::mutex> lock(usd->output_mapping_mutex);
+		mappings = usd->output_mapping_data.mappings;
+	}
+
+	// if there are no mappings - log
+	if (mappings.empty()) {
 		// if there are no mappings - log
 		obs_log(LOG_WARNING, "No mappings found");
 		return;
 	}
 
 	bool any_internal_rendering = false;
 	// iterate over the mappings and output the text with each one
-	for (const auto &mapping : usd->output_mapping_data.mappings) {
+	for (const auto &mapping : mappings) {
 		if (usd->request_data.output_type == "Audio (data)") {
 			if (!is_valid_output_source_name(mapping.output_source.c_str())) {
 				obs_log(LOG_ERROR, "Must select an output source for audio output");

diff --git a/src/url-source-data.h b/src/url-source-data.h
@@ -9,9 +9,10 @@
 #include <mutex>
 #include <thread>
 #include <condition_variable>
+#include <atomic>
 
 struct url_source_data {
-	obs_source_t *source;
+	obs_source_t *source = nullptr;
 	struct url_source_request_data request_data;
 	struct request_data_handler_response response;
 	struct output_mapping_data output_mapping_data;
@@ -22,14 +23,14 @@ struct url_source_data {
 	bool send_to_stream = false;
 	uint32_t render_width = 640;
 
-	// Text source to output the text to
 	std::mutex output_mapping_mutex;
-
-	// Use std for thread and mutex
-	std::mutex *curl_mutex = nullptr;
+	std::mutex curl_mutex;
 	std::thread curl_thread;
-	std::condition_variable *curl_thread_cv = nullptr;
-	bool curl_thread_run = false;
+	std::condition_variable curl_thread_cv;
+	std::atomic<bool> curl_thread_run = false;
+
+	// ctor must initialize mutex
+	explicit url_source_data();
 };
 
 #endif
diff --git a/src/url-source-thread.cpp b/src/url-source-thread.cpp
@@ -15,18 +15,9 @@ void curl_loop(struct url_source_data *usd)
 {
 	obs_log(LOG_INFO, "Starting URL Source thread, update timer: %d", usd->update_timer_ms);
 
-	usd->frame.format = VIDEO_FORMAT_BGRA;
-
 	inja::Environment env;
 
-	while (true) {
-		{
-			std::lock_guard<std::mutex> lock(*(usd->curl_mutex));
-			if (!usd->curl_thread_run) {
-				break;
-			}
-		}
-
+	while (usd->curl_thread_run) {
 		// time the request
 		uint64_t request_start_time_ns = get_time_ns();
 
@@ -43,8 +34,6 @@ void curl_loop(struct url_source_data *usd)
 				response.body_parts_parsed.push_back(response.body);
 			}
 
-			// lock the mapping mutex
-			std::lock_guard<std::mutex> lock(usd->output_mapping_mutex);
 			output_with_mapping(response, usd);
 		}
 
@@ -54,26 +43,23 @@ void curl_loop(struct url_source_data *usd)
 		const int64_t sleep_time_ms =
 			(int64_t)(usd->update_timer_ms) - (int64_t)(request_time_ns / 1000000);
 		if (sleep_time_ms > 0) {
-			std::unique_lock<std::mutex> lock(*(usd->curl_mutex));
+			std::unique_lock<std::mutex> lock(usd->curl_mutex);
 			// Sleep for n ns as per the update timer for the remaining time
-			usd->curl_thread_cv->wait_for(lock,
-						      std::chrono::milliseconds(sleep_time_ms));
+			usd->curl_thread_cv.wait_for(lock,
+						     std::chrono::milliseconds(sleep_time_ms));
 		}
 	}
 	obs_log(LOG_INFO, "Stopping URL Source thread");
 }
 
 void stop_and_join_curl_thread(struct url_source_data *usd)
 {
-	{
-		std::lock_guard<std::mutex> lock(*usd->curl_mutex);
-		if (!usd->curl_thread_run) {
-			// Thread is already stopped
-			return;
-		}
-		usd->curl_thread_run = false;
+	if (!usd->curl_thread_run) {
+		// Thread is already stopped
+		return;
 	}
-	usd->curl_thread_cv->notify_all();
+	usd->curl_thread_run = false;
+	usd->curl_thread_cv.notify_all();
 	if (usd->curl_thread.joinable()) {
 		usd->curl_thread.join();
 	}