From 419c44ccf6cc522cd69be0a905d8c6f46e528aca Mon Sep 17 00:00:00 2001 From: wm4 Date: Fri, 25 Oct 2019 00:25:05 +0200 Subject: vo_gpu, options: don't return NaN through API Internally, vo_gpu uses NaN for some options to indicate a default value that is different depending on the context (e.g. different scalers). There are 2 problems with this: 1. you couldn't reset the options to their defaults 2. NaN is a damn mess and shouldn't be part of the API The option parser already rejected NaN explicitly, which is why 1. didn't work. Regarding 2., JSON might be a good example, and actually caused a bug report. Fix this by mapping NaN to the special value "default". I think I'd prefer other mechanisms (maybe just having every scaler expose separate options?), but for now this will do. See you in a future commit, which painfully deprecates this and replaces it with something else. I refrained from using "no" (my favorite magic value for "unset" etc.) because then I'd have e.g. make --no-scale-param1 work, which in addition to a lot of effort looks dumb and nobody will use it. Here's also an apology for the shitty added test script. Fixes: #6691 --- DOCS/interface-changes.rst | 3 +++ DOCS/man/options.rst | 18 ++++++++++++------ TOOLS/lua/nan-test.lua | 37 +++++++++++++++++++++++++++++++++++++ options/m_option.c | 37 ++++++++++++++++++++++++++++--------- options/m_option.h | 3 +++ video/out/gpu/video.c | 12 ++++++++---- 6 files changed, 91 insertions(+), 19 deletions(-) create mode 100644 TOOLS/lua/nan-test.lua diff --git a/DOCS/interface-changes.rst b/DOCS/interface-changes.rst index 038b87b6c1..59891272da 100644 --- a/DOCS/interface-changes.rst +++ b/DOCS/interface-changes.rst @@ -191,6 +191,9 @@ Interface changes internal counter that remembered the current position. - remove deprecated ao/vo auto profiles. Consider using scripts like auto-profiles.lua instead. + - --[c]scale-[w]param[1|2] and --tone-mapping-param now accept "default", + and if set to that value, reading them as property will also return + "default", instead of float nan as in previous versions --- mpv 0.28.0 --- - rename --hwdec=mediacodec option to mediacodec-copy, to reflect conventions followed by other hardware video decoding APIs diff --git a/DOCS/man/options.rst b/DOCS/man/options.rst index e147883f29..d537380e78 100644 --- a/DOCS/man/options.rst +++ b/DOCS/man/options.rst @@ -4404,8 +4404,10 @@ The following video options are currently all specific to ``--vo=gpu`` and smooth. ``--scale-param1=``, ``--scale-param2=``, ``--cscale-param1=``, ``--cscale-param2=``, ``--dscale-param1=``, ``--dscale-param2=``, ``--tscale-param1=``, ``--tscale-param2=`` - Set filter parameters. Ignored if the filter is not tunable. Currently, - this affects the following filter parameters: + Set filter parameters. By default, these are set to the special string + ``default``, which maps to a scaler-specific default value. Ignored if the + filter is not tunable. Currently, this affects the following filter + parameters: bcspline Spline parameters (``B`` and ``C``). Defaults to 0.5 for both. @@ -4477,8 +4479,10 @@ The following video options are currently all specific to ``--vo=gpu`` and ``--scale-wparam=``, ``--cscale-wparam=``, ``--cscale-wparam=``, ``--tscale-wparam=`` (Advanced users only) Configure the parameter for the window function given - by ``--scale-window`` etc. Ignored if the window is not tunable. Currently, - this affects the following window parameters: + by ``--scale-window`` etc. By default, these are set to the special string + ``default``, which maps to a window-specific default value. Ignored if the + window is not tunable. Currently, this affects the following window + parameters: kaiser Window parameter (alpha). Defaults to 6.33. @@ -5524,8 +5528,10 @@ The following video options are currently all specific to ``--vo=gpu`` and the display. ``--tone-mapping-param=`` - Set tone mapping parameters. Ignored if the tone mapping algorithm is not - tunable. This affects the following tone mapping algorithms: + Set tone mapping parameters. By default, this is set to the special string + ``default``, which maps to an algorithm-specific default value. Ignored if + the tone mapping algorithm is not tunable. This affects the following tone + mapping algorithms: clip Specifies an extra linear coefficient to multiply into the signal diff --git a/TOOLS/lua/nan-test.lua b/TOOLS/lua/nan-test.lua new file mode 100644 index 0000000000..d3f1c8cc8f --- /dev/null +++ b/TOOLS/lua/nan-test.lua @@ -0,0 +1,37 @@ +-- Test a float property which internally uses NaN. +-- Run with --no-config (or just scale-param1 not set). + +local utils = require 'mp.utils' + +prop_name = "scale-param1" + +-- internal NaN, return string "default" instead of NaN +v = mp.get_property_native(prop_name, "fail") +print("Exp:", "string", "\"default\"") +print("Got:", type(v), utils.to_string(v)) + +v = mp.get_property(prop_name) +print("Exp:", "default") +print("Got:", v) + +-- not representable -> return provided fallback value +v = mp.get_property_number(prop_name, -100) +print("Exp:", -100) +print("Got:", v) + +mp.set_property_native(prop_name, 123) +v = mp.get_property_number(prop_name, -100) +print("Exp:", "number", 123) +print("Got:", type(v), utils.to_string(v)) + +-- try to set an actual NaN +st, msg = mp.set_property_number(prop_name, 0.0/0) +print("Exp:", nil, "") +print("Got:", st, msg) + +-- set default +mp.set_property(prop_name, "default") + +v = mp.get_property(prop_name) +print("Exp:", "default") +print("Got:", v) diff --git a/options/m_option.c b/options/m_option.c index 7bb43ad399..5df8b00291 100644 --- a/options/m_option.c +++ b/options/m_option.c @@ -918,6 +918,11 @@ static int parse_double(struct mp_log *log, const m_option_t *opt, if (bstr_eatstart0(&rest, ":") || bstr_eatstart0(&rest, "/")) tmp_float /= bstrtod(rest, &rest); + if ((opt->flags & M_OPT_DEFAULT_NAN) && bstr_equals0(param, "default")) { + tmp_float = NAN; + goto done; + } + if (rest.len) { mp_err(log, "The %.*s option must be a floating point number or a " "ratio (numerator[:/]denominator): %.*s\n", @@ -931,6 +936,7 @@ static int parse_double(struct mp_log *log, const m_option_t *opt, return M_OPT_OUT_OF_RANGE; } +done: if (dst) VAL(dst) = tmp_float; return 1; @@ -938,12 +944,18 @@ static int parse_double(struct mp_log *log, const m_option_t *opt, static char *print_double(const m_option_t *opt, const void *val) { - return talloc_asprintf(NULL, "%f", VAL(val)); + double f = VAL(val); + if (isnan(f) && (opt->flags & M_OPT_DEFAULT_NAN)) + return talloc_strdup(NULL, "default"); + return talloc_asprintf(NULL, "%f", f); } static char *print_double_f3(const m_option_t *opt, const void *val) { - return talloc_asprintf(NULL, "%.3f", VAL(val)); + double f = VAL(val); + if (isnan(f)) + return print_double(opt, val); + return talloc_asprintf(NULL, "%.3f", f); } static void add_double(const m_option_t *opt, void *val, double add, bool wrap) @@ -989,8 +1001,14 @@ static int double_set(const m_option_t *opt, void *dst, struct mpv_node *src) static int double_get(const m_option_t *opt, void *ta_parent, struct mpv_node *dst, void *src) { - dst->format = MPV_FORMAT_DOUBLE; - dst->u.double_ = *(double *)src; + double f = *(double *)src; + if (isnan(f) && (opt->flags & M_OPT_DEFAULT_NAN)) { + dst->format = MPV_FORMAT_STRING; + dst->u.string = talloc_strdup(ta_parent, "default"); + } else { + dst->format = MPV_FORMAT_DOUBLE; + dst->u.double_ = f; + } return 1; } @@ -1023,12 +1041,14 @@ static int parse_float(struct mp_log *log, const m_option_t *opt, static char *print_float(const m_option_t *opt, const void *val) { - return talloc_asprintf(NULL, "%f", VAL(val)); + double tmp = VAL(val); + return print_double(opt, &tmp); } static char *print_float_f3(const m_option_t *opt, const void *val) { - return talloc_asprintf(NULL, "%.3f", VAL(val)); + double tmp = VAL(val); + return print_double_f3(opt, &tmp); } static void add_float(const m_option_t *opt, void *val, double add, bool wrap) @@ -1057,9 +1077,8 @@ static int float_set(const m_option_t *opt, void *dst, struct mpv_node *src) static int float_get(const m_option_t *opt, void *ta_parent, struct mpv_node *dst, void *src) { - dst->format = MPV_FORMAT_DOUBLE; - dst->u.double_ = VAL(src); - return 1; + double tmp = VAL(src); + return double_get(opt, ta_parent, dst, &tmp); } const m_option_type_t m_option_type_float = { diff --git a/options/m_option.h b/options/m_option.h index a3d008a400..390919141e 100644 --- a/options/m_option.h +++ b/options/m_option.h @@ -424,6 +424,9 @@ char *format_file_size(int64_t size); #define UPDATE_OPTS_MASK \ (((UPDATE_OPT_LAST << 1) - 1) & ~(unsigned)(UPDATE_OPT_FIRST - 1)) +// type_float/type_double: string "default" is parsed as NaN (and reverse) +#define M_OPT_DEFAULT_NAN (1 << 29) + // Like M_OPT_TYPE_OPTIONAL_PARAM. #define M_OPT_OPTIONAL_PARAM (1 << 30) diff --git a/video/out/gpu/video.c b/video/out/gpu/video.c index ea47b308d3..08cd08ebe4 100644 --- a/video/out/gpu/video.c +++ b/video/out/gpu/video.c @@ -342,14 +342,18 @@ static int validate_error_diffusion_opt(struct mp_log *log, const m_option_t *op #define OPT_BASE_STRUCT struct gl_video_opts +// Use for options which use NAN for defaults. +#define OPT_FLOATDEF(name, var, flags) \ + OPT_FLOAT(name, var, (flags) | M_OPT_DEFAULT_NAN) + #define SCALER_OPTS(n, i) \ OPT_STRING_VALIDATE(n, scaler[i].kernel.name, 0, validate_scaler_opt), \ - OPT_FLOAT(n"-param1", scaler[i].kernel.params[0], 0), \ - OPT_FLOAT(n"-param2", scaler[i].kernel.params[1], 0), \ + OPT_FLOATDEF(n"-param1", scaler[i].kernel.params[0], 0), \ + OPT_FLOATDEF(n"-param2", scaler[i].kernel.params[1], 0), \ OPT_FLOAT(n"-blur", scaler[i].kernel.blur, 0), \ OPT_FLOATRANGE(n"-cutoff", scaler[i].cutoff, 0, 0.0, 1.0), \ OPT_FLOATRANGE(n"-taper", scaler[i].kernel.taper, 0, 0.0, 1.0), \ - OPT_FLOAT(n"-wparam", scaler[i].window.params[0], 0), \ + OPT_FLOATDEF(n"-wparam", scaler[i].window.params[0], 0), \ OPT_FLOAT(n"-wblur", scaler[i].window.blur, 0), \ OPT_FLOATRANGE(n"-wtaper", scaler[i].window.taper, 0, 0.0, 1.0), \ OPT_FLOATRANGE(n"-clamp", scaler[i].clamp, 0, 0.0, 1.0), \ @@ -383,7 +387,7 @@ const struct m_sub_options gl_video_conf = { tone_map.scene_threshold_low, 0, 0, 20.0), OPT_FLOATRANGE("hdr-scene-threshold-high", tone_map.scene_threshold_high, 0, 0, 20.0), - OPT_FLOAT("tone-mapping-param", tone_map.curve_param, 0), + OPT_FLOATDEF("tone-mapping-param", tone_map.curve_param, 0), OPT_FLOATRANGE("tone-mapping-max-boost", tone_map.max_boost, 0, 1.0, 10.0), OPT_FLOAT("tone-mapping-desaturate", tone_map.desat, 0), OPT_FLOATRANGE("tone-mapping-desaturate-exponent", -- cgit v1.2.3