From 40b4e32c413c4fcef01f40bec80f6b4ad0a5be2e Mon Sep 17 00:00:00 2001 From: Matthew Glazar Date: Sat, 16 May 2020 20:35:35 -0700 Subject: [PATCH 1/3] obs-x264: Refactor tokenizing of options We do a bad job of handling errors in user-supplied x264 options. I want to improve our error handling. To make my job easier, move the code for parsing the x264 options string into its own function. Also, add some tests for the functionality. Aside from a minor tweak to a log message for the opencl option, this commit should not change behavior. --- plugins/obs-x264/CMakeLists.txt | 19 +++++ plugins/obs-x264/obs-x264-options.c | 60 ++++++++++++++ plugins/obs-x264/obs-x264-options.h | 17 ++++ plugins/obs-x264/obs-x264-test.c | 65 +++++++++++++++ plugins/obs-x264/obs-x264.c | 120 +++++++++++++--------------- 5 files changed, 217 insertions(+), 64 deletions(-) create mode 100644 plugins/obs-x264/obs-x264-options.c create mode 100644 plugins/obs-x264/obs-x264-options.h create mode 100644 plugins/obs-x264/obs-x264-test.c diff --git a/plugins/obs-x264/CMakeLists.txt b/plugins/obs-x264/CMakeLists.txt index c0308748a..2b918ba84 100644 --- a/plugins/obs-x264/CMakeLists.txt +++ b/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) diff --git a/plugins/obs-x264/obs-x264-options.c b/plugins/obs-x264/obs-x264-options.c new file mode 100644 index 000000000..8ad712e4e --- /dev/null +++ b/plugins/obs-x264/obs-x264-options.c @@ -0,0 +1,60 @@ +#include +#include +#include +#include +#include +#include +#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, + .input_words = NULL, + }; + } + size_t input_option_count = 0; + for (char **input_word = input_words; *input_word; ++input_word) + input_option_count += 1; + 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; + } + } + return (struct obs_x264_options){ + .count = out_option - out_options, + .options = out_options, + .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); + } + strlist_free(options.input_words); +} diff --git a/plugins/obs-x264/obs-x264-options.h b/plugins/obs-x264/obs-x264-options.h new file mode 100644 index 000000000..6c65dc686 --- /dev/null +++ b/plugins/obs-x264/obs-x264-options.h @@ -0,0 +1,17 @@ +#pragma once + +#include + +struct obs_x264_option { + char *name; + char *value; +}; + +struct obs_x264_options { + size_t count; + struct obs_x264_option *options; + 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); diff --git a/plugins/obs-x264/obs-x264-test.c b/plugins/obs-x264/obs-x264-test.c new file mode 100644 index 000000000..2ede53448 --- /dev/null +++ b/plugins/obs-x264/obs-x264-test.c @@ -0,0 +1,65 @@ +#include +#include +#include +#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); + obs_x264_free_options(options); + + options = obs_x264_parse_options(""); + CHECK(options.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); + 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); + obs_x264_free_options(options); + + // Invalid options are dropped. + options = obs_x264_parse_options( + "ref=3 option_with_no_equal_sign 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); + 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); + obs_x264_free_options(options); +} + +int main() +{ + test_obs_x264_parse_options(); + return 0; +} diff --git a/plugins/obs-x264/obs-x264.c b/plugins/obs-x264/obs-x264.c index 2db39408c..d3f2759b1 100644 --- a/plugins/obs-x264/obs-x264.c +++ b/plugins/obs-x264/obs-x264.c @@ -20,6 +20,7 @@ #include #include #include +#include "obs-x264-options.h" #ifndef _STDINT_H_INCLUDED #define _STDINT_H_INCLUDED @@ -255,72 +256,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); } } @@ -398,7 +390,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); @@ -516,8 +508,8 @@ 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->count; ++i) + set_param(obsx264, options->options[i]); if (!update) { info("settings:\n" @@ -543,18 +535,17 @@ static bool update_settings(struct obs_x264 *obsx264, obs_data_t *settings, 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"); + const char *options_string = obs_data_get_string(settings, "x264opts"); + struct obs_x264_options options = + obs_x264_parse_options(options_string); - 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) @@ -568,9 +559,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 (options.count > 0 && options_string && !update) { + info("custom settings: %s", options_string); + } if (!obsx264->context) apply_x264_profile(obsx264, profile); @@ -578,7 +570,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); From 6190ba250dbeb1b104b1b4af7ad17ac231a3d325 Mon Sep 17 00:00:00 2001 From: Matthew Glazar Date: Sat, 16 May 2020 22:34:43 -0700 Subject: [PATCH 2/3] obs-x264: Log only options given to libx264 The "custom settings" log message is misleading. It simply echoes the user-given string, but does not reflect the options given to libx264 or handled by obs-x264 itself. Change the log message to omit skipped options. --- plugins/obs-x264/obs-x264.c | 41 ++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/plugins/obs-x264/obs-x264.c b/plugins/obs-x264/obs-x264.c index d3f2759b1..1c952dd76 100644 --- a/plugins/obs-x264/obs-x264.c +++ b/plugins/obs-x264/obs-x264.c @@ -16,6 +16,8 @@ ******************************************************************************/ #include +#include +#include #include #include #include @@ -529,15 +531,44 @@ 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 *options_string = obs_data_get_string(settings, "x264opts"); - struct obs_x264_options options = - obs_x264_parse_options(options_string); + struct obs_x264_options options = obs_x264_parse_options( + obs_data_get_string(settings, "x264opts")); bool success = true; @@ -560,8 +591,8 @@ static bool update_settings(struct obs_x264 *obsx264, obs_data_t *settings, if (success) { update_params(obsx264, settings, &options, update); - if (options.count > 0 && options_string && !update) { - info("custom settings: %s", options_string); + if (!update) { + log_custom_options(obsx264, &options); } if (!obsx264->context) From 601aa3ffb1e5ed6afc286863d03c85c2013aae9a Mon Sep 17 00:00:00 2001 From: Matthew Glazar Date: Sat, 16 May 2020 23:10:05 -0700 Subject: [PATCH 3/3] obs-x264: Log ignored options When an x264 option doesn't include a "=", it is silently ignored. This is frustrating for users. Log when part of the options string is ignored. Aside from logging, this commit should not change behavior. --- plugins/obs-x264/obs-x264-options.c | 11 +++++++++++ plugins/obs-x264/obs-x264-options.h | 2 ++ plugins/obs-x264/obs-x264-test.c | 13 +++++++++++-- plugins/obs-x264/obs-x264.c | 3 +++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/plugins/obs-x264/obs-x264-options.c b/plugins/obs-x264/obs-x264-options.c index 8ad712e4e..a1e7a66b7 100644 --- a/plugins/obs-x264/obs-x264-options.c +++ b/plugins/obs-x264/obs-x264-options.c @@ -29,12 +29,17 @@ struct obs_x264_options obs_x264_parse_options(const char *options_string) 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; @@ -42,11 +47,16 @@ struct obs_x264_options obs_x264_parse_options(const char *options_string) 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, }; } @@ -56,5 +66,6 @@ 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); } diff --git a/plugins/obs-x264/obs-x264-options.h b/plugins/obs-x264/obs-x264-options.h index 6c65dc686..12c0674e9 100644 --- a/plugins/obs-x264/obs-x264-options.h +++ b/plugins/obs-x264/obs-x264-options.h @@ -10,6 +10,8 @@ struct obs_x264_option { struct obs_x264_options { size_t count; struct obs_x264_option *options; + size_t ignored_word_count; + char **ignored_words; char **input_words; }; diff --git a/plugins/obs-x264/obs-x264-test.c b/plugins/obs-x264/obs-x264-test.c index 2ede53448..e64e824be 100644 --- a/plugins/obs-x264/obs-x264-test.c +++ b/plugins/obs-x264/obs-x264-test.c @@ -18,16 +18,19 @@ static void test_obs_x264_parse_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"); @@ -36,16 +39,21 @@ static void test_obs_x264_parse_options() 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 dropped. + // Invalid options are ignored. options = obs_x264_parse_options( - "ref=3 option_with_no_equal_sign bframes=8"); + "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. @@ -55,6 +63,7 @@ static void test_obs_x264_parse_options() 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); } diff --git a/plugins/obs-x264/obs-x264.c b/plugins/obs-x264/obs-x264.c index 1c952dd76..4c54670a3 100644 --- a/plugins/obs-x264/obs-x264.c +++ b/plugins/obs-x264/obs-x264.c @@ -510,6 +510,9 @@ static void update_params(struct obs_x264 *obsx264, obs_data_t *settings, else obsx264->params.i_csp = X264_CSP_NV12; + 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]);