From 2415b695724c15def54a039527ee9eb574ffb65f Mon Sep 17 00:00:00 2001 From: wm4 Date: Sun, 18 Sep 2016 16:06:12 +0200 Subject: player: more option/property consistency fixes Some properties had a different type from their equivalent options (such as mute, volume, deinterlace, edition). This wasn't really sane, as raw option values should be always within their bounds. On the other hand, these properties use a different type to reflect runtime limits (such as range of available editions), or simply to improve the "UI" (you don't want to cycle throuhg the completely useless "auto" value when cycling the "mute" property). Handle this by making them always return the option type, but also allowing them to provide a "constricted" type, which is used for UI purposes. All M_PROPERTY_GET_CONSTRICTED_TYPE changes are related to this. One consequence is that you can set the volume property to arbitrary high values just like with the --volume option, but using the "add" command it still restricts it to the --volume-max range. Also deprecate --chapter, as it is grossly incompatible to the chapter property. We pondered renaming it to --chapters, or introducing a more powerful --range option, but concluded that --start --end is actually enough. These changes appear to take care of the last gross property/option incompatibilities, although there might still be a few lurking. --- DOCS/interface-changes.rst | 2 ++ DOCS/man/input.rst | 29 +++++++++------------- options/m_config.c | 7 ++---- options/m_config.h | 2 ++ options/m_property.c | 11 +++++++++ options/m_property.h | 6 +++++ options/options.c | 3 ++- player/command.c | 61 ++++++++++++++++++++++++---------------------- player/core.h | 2 +- player/video.c | 10 +++++--- 10 files changed, 75 insertions(+), 58 deletions(-) diff --git a/DOCS/interface-changes.rst b/DOCS/interface-changes.rst index 339b7ffd9e..8472da4ff2 100644 --- a/DOCS/interface-changes.rst +++ b/DOCS/interface-changes.rst @@ -64,6 +64,8 @@ Interface changes - deprecate "--vo=direct3d_shaders" - use "--vo=direct3d" instead. Change "--vo=direct3d" to always use shaders by default. - deprecate --playlist-pos option, renamed to --playlist-start + - deprecate the --chapter option, as it is redundant with --start/--end, + and conflicts with the semantics of the "chapter" property - incompatible change to cdda:// protocol options: the part after cdda:// now always sets the device, not the span or speed to be played. No separating extra "/" is needed. The hidden --cdda-device options is also diff --git a/DOCS/man/input.rst b/DOCS/man/input.rst index 17bba1c2ab..1f7862d1c1 100644 --- a/DOCS/man/input.rst +++ b/DOCS/man/input.rst @@ -2079,12 +2079,14 @@ caveats with some properties (due to historical reasons): ``deinterlace`` While video is active, this behaves differently from the option. It will never return the ``auto`` value (but the state as observed by the video - chain). You cannot set ``auto`` either. - - Option changes at runtime are affected by this as well. + chain). If you set ``auto``, the property will set this as the option value, + and will return the actual video chain state as observed instead of auto. ``video-aspect`` - While video is active, always returns the effective aspect ratio. + While video is active, always returns the effective aspect ratio. Setting + a special value (like ``no``, values ``<= 0``) will make the property + set this as option, and return whatever actual aspect was derived from the + option setting. ``brightness`` (and other color options) If ``--vo=xv`` is used, these properties may return the adapter's current @@ -2103,19 +2105,10 @@ caveats with some properties (due to historical reasons): Option changes at runtime are affected by this as well. -``chapter`` - While playback is *not* active, the property behaves like the option, and - you can set a chapter range. While playback is active, you can set only - the current chapter (to which the player will seek immediately). - - Option changes at runtime are affected by this as well. - -``volume`` - When set as option, the maximum (set by ``--volume-max``) is not checked, - while when set as property, the maximum is enforced. - -``mute`` - The option has a deprecated ``auto`` value, which is equal to ``no``. +``edition`` + While a file is loaded, the property will always return the effective + edition, and setting the ``auto`` value will show somewhat strange behavior + (the property eventually switching to whatever is the default edition). ``playlist`` The property is read-only and returns the current internal playlist. The @@ -2138,7 +2131,7 @@ caveats with some properties (due to historical reasons): Strictly speaking, option access via API (e.g. ``mpv_set_option_string()``) has the same problem, and it's only a difference between CLI/API. -``demuxer``, ``idle``, ``length``, ``audio-samplerate``, ``audio-channels``, ``audio-format``, ``fps``, ``cache``, ``playlist-pos`` +``demuxer``, ``idle``, ``length``, ``audio-samplerate``, ``audio-channels``, ``audio-format``, ``fps``, ``cache``, ``playlist-pos``, ``chapter`` These behave completely different as property, but are deprecated (newer aliases which don't conflict have been added). After the deprecation period they will be changed to the proper option behavior. diff --git a/options/m_config.c b/options/m_config.c index 7cc9a46792..6dd72380d9 100644 --- a/options/m_config.c +++ b/options/m_config.c @@ -87,9 +87,6 @@ struct m_opt_backup { void *backup; }; -static struct m_config_option *m_config_get_co_raw(const struct m_config *config, - struct bstr name); - static int parse_include(struct m_config *config, struct bstr param, bool set, int flags) { @@ -556,8 +553,8 @@ static void m_config_add_option(struct m_config *config, MP_TARRAY_APPEND(config, config->opts, config->num_opts, co); } -static struct m_config_option *m_config_get_co_raw(const struct m_config *config, - struct bstr name) +struct m_config_option *m_config_get_co_raw(const struct m_config *config, + struct bstr name) { if (!name.len) return NULL; diff --git a/options/m_config.h b/options/m_config.h index 9440bd5833..8befc51805 100644 --- a/options/m_config.h +++ b/options/m_config.h @@ -190,6 +190,8 @@ int m_config_set_option_node(struct m_config *config, bstr name, int m_config_parse_suboptions(struct m_config *config, char *name, char *subopts); +struct m_config_option *m_config_get_co_raw(const struct m_config *config, + struct bstr name); struct m_config_option *m_config_get_co(const struct m_config *config, struct bstr name); diff --git a/options/m_property.c b/options/m_property.c index 13d3844671..ba8d37b2e6 100644 --- a/options/m_property.c +++ b/options/m_property.c @@ -115,6 +115,10 @@ int m_property_do(struct mp_log *log, const struct m_property *prop_list, M_PROPERTY_NOT_IMPLEMENTED) return r; // Fallback to m_option + r = do_action(prop_list, name, M_PROPERTY_GET_CONSTRICTED_TYPE, &opt, ctx); + if (r <= 0) + return r; + assert(opt.type); if (!opt.type->add) return M_PROPERTY_NOT_IMPLEMENTED; if ((r = do_action(prop_list, name, M_PROPERTY_GET, &val, ctx)) <= 0) @@ -124,6 +128,13 @@ int m_property_do(struct mp_log *log, const struct m_property *prop_list, m_option_free(&opt, &val); return r; } + case M_PROPERTY_GET_CONSTRICTED_TYPE: { + if ((r = do_action(prop_list, name, action, arg, ctx)) >= 0) + return r; + if ((r = do_action(prop_list, name, M_PROPERTY_GET_TYPE, arg, ctx)) >= 0) + return r; + return M_PROPERTY_NOT_IMPLEMENTED; + } case M_PROPERTY_SET: { return do_action(prop_list, name, M_PROPERTY_SET, arg, ctx); } diff --git a/options/m_property.h b/options/m_property.h index 0f8230608b..d6c8c5aab3 100644 --- a/options/m_property.h +++ b/options/m_property.h @@ -48,6 +48,12 @@ enum mp_property_action { // arg: char** M_PROPERTY_PRINT, + // Like M_PROPERTY_GET_TYPE, but get a type that is compatible to the real + // type, but reflect practical limits, such as runtime-available values. + // This is mostly used for "UI" related things. + // (Example: volume property.) + M_PROPERTY_GET_CONSTRICTED_TYPE, + // Switch the property up/down by a given value. // If unimplemented, the property wrapper uses the property type as // fallback. diff --git a/options/options.c b/options/options.c index 2ae0f6ad3b..6db3e945a9 100644 --- a/options/options.c +++ b/options/options.c @@ -296,7 +296,8 @@ const m_option_t mp_opts[] = { #if HAVE_DVDREAD || HAVE_DVDNAV OPT_SUBSTRUCT("", dvd_opts, dvd_conf, 0), #endif /* HAVE_DVDREAD */ - OPT_INTPAIR("chapter", chapterrange, 0), + OPT_INTPAIR("chapter", chapterrange, 0, .deprecation_message = "instead of " + "--chapter=A-B use --start=#A --end=#B+1"), OPT_CHOICE_OR_INT("edition", edition_id, 0, 0, 8190, ({"auto", -1})), #if HAVE_LIBBLURAY diff --git a/player/command.c b/player/command.c index 6c5b3e947d..3e677bfd37 100644 --- a/player/command.c +++ b/player/command.c @@ -279,10 +279,11 @@ int mp_on_set_option(void *ctx, struct m_config_option *co, void *data, int flag // vf*, af*, chapter // OK, is handled separately: playlist // OK, does not conflict on low level: audio-file, sub-file, external-file + // OK, different value ranges, but happens to work for now: volume, edition + // All the other properties are deprecated in their current form. static const char *const no_property[] = { - "volume", // restricts to --volume-max "demuxer", "idle", "length", "audio-samplerate", "audio-channels", - "audio-format", "fps", "cache", "playlist-pos", // different semantics + "audio-format", "fps", "cache", "playlist-pos", "chapter", NULL }; @@ -300,9 +301,19 @@ int mp_on_set_option(void *ctx, struct m_config_option *co, void *data, int flag name = tmp; } - int r = mp_property_do_silent(name, M_PROPERTY_SET, data, mpctx); + struct m_option type = {0}; + + int r = mp_property_do_silent(name, M_PROPERTY_GET_TYPE, &type, mpctx); if (r == M_PROPERTY_UNKNOWN) goto direct_option; // not mapped as property + if (r != M_PROPERTY_OK) + return M_OPT_INVALID; // shouldn't happen + + assert(type.type == co->opt->type); + assert(type.max == co->opt->max); + assert(type.min == co->opt->min); + + r = mp_property_do_silent(name, M_PROPERTY_SET, data, mpctx); if (r != M_PROPERTY_OK) return M_OPT_INVALID; @@ -873,7 +884,7 @@ static int mp_property_chapter(void *ctx, struct m_property *prop, { MPContext *mpctx = ctx; if (!mpctx->playback_initialized) - return mp_property_generic_option(mpctx, prop, action, arg); + return M_PROPERTY_UNAVAILABLE; int chapter = get_current_chapter(mpctx); int num = get_chapter_count(mpctx); @@ -993,14 +1004,9 @@ static int mp_property_edition(void *ctx, struct m_property *prop, int action, void *arg) { MPContext *mpctx = ctx; - if (!mpctx->playback_initialized) - return mp_property_generic_option(mpctx, prop, action, arg); - struct demuxer *demuxer = mpctx->demuxer; - if (!demuxer) - return M_PROPERTY_UNAVAILABLE; - if (demuxer->num_editions <= 0) - return M_PROPERTY_UNAVAILABLE; + if (!mpctx->playback_initialized || !demuxer || demuxer->num_editions <= 0) + return mp_property_generic_option(mpctx, prop, action, arg); int edition = demuxer->edition; @@ -1014,22 +1020,18 @@ static int mp_property_edition(void *ctx, struct m_property *prop, mpctx->opts->edition_id = edition; if (!mpctx->stop_play) mpctx->stop_play = PT_RELOAD_FILE; - mp_wakeup_core(mpctx);; + mp_wakeup_core(mpctx); + break; // make it accessible to the demuxer via option change notify } return M_PROPERTY_OK; } - case M_PROPERTY_GET_TYPE: { - struct m_option opt = { - .type = CONF_TYPE_INT, - .flags = CONF_RANGE, - .min = 0, - .max = demuxer->num_editions - 1, - }; - *(struct m_option *)arg = opt; - return M_PROPERTY_OK; + case M_PROPERTY_GET_CONSTRICTED_TYPE: { + int r = mp_property_generic_option(mpctx, prop, M_PROPERTY_GET_TYPE, arg); + ((struct m_option *)arg)->max = demuxer->num_editions - 1; + return r; } } - return M_PROPERTY_NOT_IMPLEMENTED; + return mp_property_generic_option(mpctx, prop, action, arg); } static int get_edition_entry(int item, int action, void *arg, void *ctx) @@ -1677,7 +1679,7 @@ static int mp_property_volume(void *ctx, struct m_property *prop, struct MPOpts *opts = mpctx->opts; switch (action) { - case M_PROPERTY_GET_TYPE: + case M_PROPERTY_GET_CONSTRICTED_TYPE: *(struct m_option *)arg = (struct m_option){ .type = CONF_TYPE_FLOAT, .flags = M_OPT_RANGE, @@ -1705,7 +1707,7 @@ static int mp_property_mute(void *ctx, struct m_property *prop, { MPContext *mpctx = ctx; - if (action == M_PROPERTY_GET_TYPE) { + if (action == M_PROPERTY_GET_CONSTRICTED_TYPE) { *(struct m_option *)arg = (struct m_option){.type = CONF_TYPE_FLAG}; return M_PROPERTY_OK; } @@ -2373,14 +2375,14 @@ static int mp_property_deinterlace(void *ctx, struct m_property *prop, case M_PROPERTY_GET: *(int *)arg = get_deinterlacing(mpctx) > 0; return M_PROPERTY_OK; - case M_PROPERTY_GET_TYPE: + case M_PROPERTY_GET_CONSTRICTED_TYPE: *(struct m_option *)arg = (struct m_option){.type = CONF_TYPE_FLAG}; return M_PROPERTY_OK; case M_PROPERTY_SET: set_deinterlacing(mpctx, *(int *)arg); return M_PROPERTY_OK; } - return M_PROPERTY_NOT_IMPLEMENTED; + return mp_property_generic_option(mpctx, prop, action, arg); } static int video_simple_refresh_property(void *ctx, struct m_property *prop, @@ -4315,7 +4317,7 @@ static void show_property_osd(MPContext *mpctx, const char *name, int osd_mode) } struct m_option prop = {0}; - mp_property_do(name, M_PROPERTY_GET_TYPE, &prop, mpctx); + mp_property_do(name, M_PROPERTY_GET_CONSTRICTED_TYPE, &prop, mpctx); if ((osd_mode & MP_ON_OSD_BAR) && (prop.flags & CONF_RANGE) == CONF_RANGE) { if (prop.type == CONF_TYPE_INT) { int n = prop.min; @@ -4668,7 +4670,7 @@ static int mp_property_multiply(char *property, double f, struct MPContext *mpct struct m_option opt = {0}; int r; - r = mp_property_do(property, M_PROPERTY_GET_TYPE, &opt, mpctx); + r = mp_property_do(property, M_PROPERTY_GET_CONSTRICTED_TYPE, &opt, mpctx); if (r != M_PROPERTY_OK) return r; assert(opt.type); @@ -5615,7 +5617,8 @@ extern const struct m_sub_options gl_video_conf; void mp_notify_property(struct MPContext *mpctx, const char *property) { - struct m_config_option *co = m_config_get_co(mpctx->mconfig, bstr0(property)); + struct m_config_option *co = + m_config_get_co_raw(mpctx->mconfig, bstr0(property)); if (co) { if (m_config_is_in_group(mpctx->mconfig, &gl_video_conf, co)) { if (mpctx->video_out) diff --git a/player/core.h b/player/core.h index 0f1c8c7e17..22e19e49ef 100644 --- a/player/core.h +++ b/player/core.h @@ -575,7 +575,7 @@ void uninit_video_chain(struct MPContext *mpctx); double calc_average_frame_duration(struct MPContext *mpctx); int init_video_decoder(struct MPContext *mpctx, struct track *track); int get_deinterlacing(struct MPContext *mpctx); -void set_deinterlacing(struct MPContext *mpctx, bool enable); +void set_deinterlacing(struct MPContext *mpctx, int opt_val); // Values of MPOpts.softvol enum { diff --git a/player/video.c b/player/video.c index 4c56ca1a00..3835ef5f95 100644 --- a/player/video.c +++ b/player/video.c @@ -264,14 +264,16 @@ int get_deinterlacing(struct MPContext *mpctx) return enabled; } -void set_deinterlacing(struct MPContext *mpctx, bool enable) +void set_deinterlacing(struct MPContext *mpctx, int opt_val) { - if (enable == (get_deinterlacing(mpctx) > 0)) + if ((opt_val < 0 && mpctx->opts->deinterlace == opt_val) || + (opt_val == (get_deinterlacing(mpctx) > 0))) return; - mpctx->opts->deinterlace = enable; + mpctx->opts->deinterlace = opt_val; recreate_auto_filters(mpctx); - mpctx->opts->deinterlace = get_deinterlacing(mpctx) > 0; + if (opt_val >= 0) + mpctx->opts->deinterlace = get_deinterlacing(mpctx) > 0; } static void recreate_video_filters(struct MPContext *mpctx) -- cgit v1.2.3