From 40b4e32c413c4fcef01f40bec80f6b4ad0a5be2e Mon Sep 17 00:00:00 2001 From: Matthew Glazar Date: Sat, 16 May 2020 20:35:35 -0700 Subject: [PATCH] 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);