Merge pull request #2942 from strager/x264-options

obs-x264: Log when options are ignored

plugins/obs-x264/CMakeLists.txt

@@ -4,6 +4,18 @@ find_package(Libx264 REQUIRED)
 include_directories(${LIBX264_INCLUDE_DIRS})
 add_definitions(${LIBX264_DEFINITIONS})
 
+set(obs-x264-util_HEADERS
+	obs-x264-options.h)
+
+set(obs-x264-util_SOURCES
+	obs-x264-options.c)
+
+add_library(obs-x264-util STATIC
+	${obs-x264-util_HEADERS}
+	${obs-x264-util_SOURCES})
+target_link_libraries(obs-x264-util PRIVATE libobs)
+set_target_properties(obs-x264-util PROPERTIES FOLDER "plugins")
+
 set(obs-x264_SOURCES
 	obs-x264.c
 	obs-x264-plugin-main.c)
@@ -16,10 +28,17 @@ if(WIN32)
 endif()
 
 add_library(obs-x264 MODULE
+	${obs-x264_HEADERS}
 	${obs-x264_SOURCES})
 target_link_libraries(obs-x264
 	libobs
+	obs-x264-util
 	${LIBX264_LIBRARIES})
 set_target_properties(obs-x264 PROPERTIES FOLDER "plugins")
 
 install_obs_plugin_with_data(obs-x264 data)
+
+add_executable(obs-x264-test obs-x264-test.c)
+set_target_properties(obs-x264-test PROPERTIES FOLDER "plugins")
+target_link_libraries(obs-x264-test PRIVATE libobs obs-x264-util)
+add_test(NAME obs-x264-test COMMAND obs-x264-test)

plugins/obs-x264/obs-x264-options.c

@@ -0,0 +1,71 @@
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
+#include <util/bmem.h>
+#include <util/dstr.h>
+#include "obs-x264-options.h"
+
+static bool getparam(const char *param, char **name, const char **value)
+{
+	const char *assign;
+
+	if (!param || !*param || (*param == '='))
+		return false;
+
+	assign = strchr(param, '=');
+	if (!assign || !*assign || !*(assign + 1))
+		return false;
+
+	*name = bstrdup_n(param, assign - param);
+	*value = assign + 1;
+	return true;
+}
+
+struct obs_x264_options obs_x264_parse_options(const char *options_string)
+{
+	char **input_words = strlist_split(options_string, ' ', false);
+	if (!input_words) {
+		return (struct obs_x264_options){
+			.count = 0,
+			.options = NULL,
+			.ignored_word_count = 0,
+			.ignored_words = NULL,
+			.input_words = NULL,
+		};
+	}
+	size_t input_option_count = 0;
+	for (char **input_word = input_words; *input_word; ++input_word)
+		input_option_count += 1;
+	char **ignored_words =
+		bmalloc(input_option_count * sizeof(*ignored_words));
+	char **ignored_word = ignored_words;
+	struct obs_x264_option *out_options =
+		bmalloc(input_option_count * sizeof(*out_options));
+	struct obs_x264_option *out_option = out_options;
+	for (char **input_word = input_words; *input_word; ++input_word) {
+		if (getparam(*input_word, &out_option->name,
+			     &out_option->value)) {
+			++out_option;
+		} else {
+			*ignored_word = *input_word;
+			++ignored_word;
+		}
+	}
+	return (struct obs_x264_options){
+		.count = out_option - out_options,
+		.options = out_options,
+		.ignored_word_count = ignored_word - ignored_words,
+		.ignored_words = ignored_words,
+		.input_words = input_words,
+	};
+}
+
+void obs_x264_free_options(struct obs_x264_options options)
+{
+	for (size_t i = 0; i < options.count; ++i) {
+		bfree(options.options[i].name);
+	}
+	bfree(options.ignored_words);
+	strlist_free(options.input_words);
+}

plugins/obs-x264/obs-x264-options.h

@@ -0,0 +1,19 @@
+#pragma once
+
+#include <stddef.h>
+
+struct obs_x264_option {
+	char *name;
+	char *value;
+};
+
+struct obs_x264_options {
+	size_t count;
+	struct obs_x264_option *options;
+	size_t ignored_word_count;
+	char **ignored_words;
+	char **input_words;
+};
+
+struct obs_x264_options obs_x264_parse_options(const char *options_string);
+void obs_x264_free_options(struct obs_x264_options options);

plugins/obs-x264/obs-x264-test.c

@@ -0,0 +1,74 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include "obs-x264-options.h"
+
+#define CHECK(condition)                                                    \
+	do {                                                                \
+		if (!(condition)) {                                         \
+			fprintf(stderr, "%s:%d: error: check failed: %s\n", \
+				__FILE__, __LINE__, #condition);            \
+			exit(1);                                            \
+		}                                                           \
+	} while (0)
+
+static void test_obs_x264_parse_options()
+{
+	struct obs_x264_options options;
+
+	options = obs_x264_parse_options(NULL);
+	CHECK(options.count == 0);
+	CHECK(options.ignored_word_count == 0);
+	obs_x264_free_options(options);
+
+	options = obs_x264_parse_options("");
+	CHECK(options.count == 0);
+	CHECK(options.ignored_word_count == 0);
+	obs_x264_free_options(options);
+
+	options = obs_x264_parse_options("ref=3");
+	CHECK(options.count == 1);
+	CHECK(strcmp(options.options[0].name, "ref") == 0);
+	CHECK(strcmp(options.options[0].value, "3") == 0);
+	CHECK(options.ignored_word_count == 0);
+	obs_x264_free_options(options);
+
+	options = obs_x264_parse_options("ref=3 bframes=8");
+	CHECK(options.count == 2);
+	CHECK(strcmp(options.options[0].name, "ref") == 0);
+	CHECK(strcmp(options.options[0].value, "3") == 0);
+	CHECK(strcmp(options.options[1].name, "bframes") == 0);
+	CHECK(strcmp(options.options[1].value, "8") == 0);
+	CHECK(options.ignored_word_count == 0);
+	obs_x264_free_options(options);
+
+	// Invalid options are ignored.
+	options = obs_x264_parse_options(
+		"ref=3 option_with_no_equal_sign bframes=8 1234");
+	CHECK(options.count == 2);
+	CHECK(strcmp(options.options[0].name, "ref") == 0);
+	CHECK(strcmp(options.options[0].value, "3") == 0);
+	CHECK(strcmp(options.options[1].name, "bframes") == 0);
+	CHECK(strcmp(options.options[1].value, "8") == 0);
+	CHECK(options.ignored_word_count == 2);
+	CHECK(strcmp(options.ignored_words[0], "option_with_no_equal_sign") ==
+	      0);
+	CHECK(strcmp(options.ignored_words[1], "1234") == 0);
+	obs_x264_free_options(options);
+
+	// Extra whitespace is ignored between and around options.
+	options = obs_x264_parse_options("  ref=3   bframes=8  ");
+	CHECK(options.count == 2);
+	CHECK(strcmp(options.options[0].name, "ref") == 0);
+	CHECK(strcmp(options.options[0].value, "3") == 0);
+	CHECK(strcmp(options.options[1].name, "bframes") == 0);
+	CHECK(strcmp(options.options[1].value, "8") == 0);
+	CHECK(options.ignored_word_count == 0);
+	obs_x264_free_options(options);
+}
+
+int main()
+{
+	test_obs_x264_parse_options();
+	return 0;
+}

plugins/obs-x264/obs-x264.c

@@ -16,10 +16,13 @@
 ******************************************************************************/
 
 #include <stdio.h>
+#include <string.h>
+#include <util/bmem.h>
 #include <util/dstr.h>
 #include <util/darray.h>
 #include <util/platform.h>
 #include <obs-module.h>
+#include "obs-x264-options.h"
 
 #ifndef _STDINT_H_INCLUDED
 #define _STDINT_H_INCLUDED
@@ -255,72 +258,63 @@ static const char *validate(struct obs_x264 *obsx264, const char *val,
 	return NULL;
 }
 
-static void override_base_param(struct obs_x264 *obsx264, const char *param,
-				char **preset, char **profile, char **tune)
+static void override_base_param(struct obs_x264 *obsx264,
+				struct obs_x264_option option, char **preset,
+				char **profile, char **tune)
 {
-	char *name;
-	const char *val;
-
-	if (getparam(param, &name, &val)) {
-		if (astrcmpi(name, "preset") == 0) {
-			const char *valid_name = validate(
-				obsx264, val, "preset", x264_preset_names);
-			if (valid_name) {
-				bfree(*preset);
-				*preset = bstrdup(val);
-			}
-
-		} else if (astrcmpi(name, "profile") == 0) {
-			const char *valid_name = validate(
-				obsx264, val, "profile", x264_profile_names);
-			if (valid_name) {
-				bfree(*profile);
-				*profile = bstrdup(val);
-			}
-
-		} else if (astrcmpi(name, "tune") == 0) {
-			const char *valid_name =
-				validate(obsx264, val, "tune", x264_tune_names);
-			if (valid_name) {
-				bfree(*tune);
-				*tune = bstrdup(val);
-			}
+	const char *name = option.name;
+	const char *val = option.value;
+	if (astrcmpi(name, "preset") == 0) {
+		const char *valid_name =
+			validate(obsx264, val, "preset", x264_preset_names);
+		if (valid_name) {
+			bfree(*preset);
+			*preset = bstrdup(val);
 		}
 
-		bfree(name);
+	} else if (astrcmpi(name, "profile") == 0) {
+		const char *valid_name =
+			validate(obsx264, val, "profile", x264_profile_names);
+		if (valid_name) {
+			bfree(*profile);
+			*profile = bstrdup(val);
+		}
+
+	} else if (astrcmpi(name, "tune") == 0) {
+		const char *valid_name =
+			validate(obsx264, val, "tune", x264_tune_names);
+		if (valid_name) {
+			bfree(*tune);
+			*tune = bstrdup(val);
+		}
 	}
 }
 
-static inline void override_base_params(struct obs_x264 *obsx264, char **params,
+static inline void override_base_params(struct obs_x264 *obsx264,
+					const struct obs_x264_options *options,
 					char **preset, char **profile,
 					char **tune)
 {
-	while (*params)
-		override_base_param(obsx264, *(params++), preset, profile,
-				    tune);
+	for (size_t i = 0; i < options->count; ++i)
+		override_base_param(obsx264, options->options[i], preset,
+				    profile, tune);
 }
 
 #define OPENCL_ALIAS "opencl_is_experimental_and_potentially_unstable"
 
-static inline void set_param(struct obs_x264 *obsx264, const char *param)
+static inline void set_param(struct obs_x264 *obsx264,
+			     struct obs_x264_option option)
 {
-	char *name;
-	const char *val;
-
-	if (getparam(param, &name, &val)) {
-		if (strcmp(name, "preset") != 0 &&
-		    strcmp(name, "profile") != 0 && strcmp(name, "tune") != 0 &&
-		    strcmp(name, "fps") != 0 &&
-		    strcmp(name, "force-cfr") != 0 &&
-		    strcmp(name, "width") != 0 && strcmp(name, "height") != 0 &&
-		    strcmp(name, "opencl") != 0) {
-			if (strcmp(name, OPENCL_ALIAS) == 0)
-				strcpy(name, "opencl");
-			if (x264_param_parse(&obsx264->params, name, val) != 0)
-				warn("x264 param: %s failed", param);
-		}
-
-		bfree(name);
+	const char *name = option.name;
+	const char *val = option.value;
+	if (strcmp(name, "preset") != 0 && strcmp(name, "profile") != 0 &&
+	    strcmp(name, "tune") != 0 && strcmp(name, "fps") != 0 &&
+	    strcmp(name, "force-cfr") != 0 && strcmp(name, "width") != 0 &&
+	    strcmp(name, "height") != 0 && strcmp(name, "opencl") != 0) {
+		if (strcmp(option.name, OPENCL_ALIAS) == 0)
+			name = "opencl";
+		if (x264_param_parse(&obsx264->params, name, val) != 0)
+			warn("x264 param: %s=%s failed", name, val);
 	}
 }
 
@@ -384,7 +378,7 @@ enum rate_control {
 };
 
 static void update_params(struct obs_x264 *obsx264, obs_data_t *settings,
-			  char **params, bool update)
+			  const struct obs_x264_options *options, bool update)
 {
 	video_t *video = obs_encoder_video(obsx264->encoder);
 	const struct video_output_info *voi = video_output_get_info(video);
@@ -527,8 +521,11 @@ static void update_params(struct obs_x264 *obsx264, obs_data_t *settings,
 	else
 		obsx264->params.i_csp = X264_CSP_NV12;
 
-	while (*params)
-		set_param(obsx264, *(params++));
+	for (size_t i = 0; i < options->ignored_word_count; ++i)
+		warn("ignoring invalid x264 option: %s",
+		     options->ignored_words[i]);
+	for (size_t i = 0; i < options->count; ++i)
+		set_param(obsx264, options->options[i]);
 
 	if (!update) {
 		info("settings:\n"
@@ -548,24 +545,52 @@ static void update_params(struct obs_x264 *obsx264, obs_data_t *settings,
 	}
 }
 
+static void log_custom_options(struct obs_x264 *obsx264,
+			       const struct obs_x264_options *options)
+{
+	if (options->count == 0) {
+		return;
+	}
+	size_t settings_string_length = 0;
+	for (size_t i = 0; i < options->count; ++i)
+		settings_string_length += strlen(options->options[i].name) +
+					  strlen(options->options[i].value) + 5;
+	size_t buffer_size = settings_string_length + 1;
+	char *settings_string = bmalloc(settings_string_length + 1);
+	char *p = settings_string;
+	size_t remaining_buffer_size = buffer_size;
+	for (size_t i = 0; i < options->count; ++i) {
+		int chars_written = snprintf(p, remaining_buffer_size,
+					     "\n\t%s = %s",
+					     options->options[i].name,
+					     options->options[i].value);
+		assert(chars_written >= 0);
+		assert(chars_written <= remaining_buffer_size);
+		p += chars_written;
+		remaining_buffer_size -= chars_written;
+	}
+	assert(remaining_buffer_size == 1);
+	assert(*p == '\0');
+	info("custom settings: %s", settings_string);
+	bfree(settings_string);
+}
+
 static bool update_settings(struct obs_x264 *obsx264, obs_data_t *settings,
 			    bool update)
 {
 	char *preset = bstrdup(obs_data_get_string(settings, "preset"));
 	char *profile = bstrdup(obs_data_get_string(settings, "profile"));
 	char *tune = bstrdup(obs_data_get_string(settings, "tune"));
-	const char *opts = obs_data_get_string(settings, "x264opts");
+	struct obs_x264_options options = obs_x264_parse_options(
+		obs_data_get_string(settings, "x264opts"));
 
-	char **paramlist;
 	bool success = true;
 
-	paramlist = strlist_split(opts, ' ', false);
-
 	if (!update)
 		blog(LOG_INFO, "---------------------------------");
 
 	if (!obsx264->context) {
-		override_base_params(obsx264, paramlist, &preset, &profile,
+		override_base_params(obsx264, &options, &preset, &profile,
 				     &tune);
 
 		if (preset && *preset)
@@ -579,9 +604,10 @@ static bool update_settings(struct obs_x264 *obsx264, obs_data_t *settings,
 	}
 
 	if (success) {
-		update_params(obsx264, settings, paramlist, update);
-		if (opts && *opts && !update)
-			info("custom settings: %s", opts);
+		update_params(obsx264, settings, &options, update);
+		if (!update) {
+			log_custom_options(obsx264, &options);
+		}
 
 		if (!obsx264->context)
 			apply_x264_profile(obsx264, profile);
@@ -589,7 +615,7 @@ static bool update_settings(struct obs_x264 *obsx264, obs_data_t *settings,
 
 	obsx264->params.b_repeat_headers = false;
 
-	strlist_free(paramlist);
+	obs_x264_free_options(options);
 	bfree(preset);
 	bfree(profile);
 	bfree(tune);