commands: handle property clamping in m_option

Instead of clamping property values to the valid range in each property
implementation, handle it in the property layer. The functionality to
handle clamping for each type is in m_option.c.

It's not really clear whether this is really needed. Normally, the raw
values for M_PROPERTY_SET come only from m_option_type.parse (setting
properties as string) or from m_option_parse.add (using the "switch"
input command). However, since this was already done before, and since
we _really_ want to be sure only to write valid values, add this code
anyway. The newly added warnings/error messages should never actually
be printed during normal operation and are for debugging (if they
happen, we definitely want to see them).

5 files changed, 123 insertions, 27 deletions

diff --git a/command.c b/command.c
index b5b101ce79..c48962c28a 100644
--- a/command.c
+++ b/command.c
@@ -145,7 +145,6 @@ static int mp_property_playback_speed(m_option_t *prop, int action,
     switch (action) {
     case M_PROPERTY_SET:
         opts->playback_speed = *(float *) arg;
-        M_PROPERTY_CLAMP(prop, opts->playback_speed);
         // Adjust time until next frame flip for nosound mode
         mpctx->time_frame *= orig_speed / opts->playback_speed;
         reinit_audio_chain(mpctx);
@@ -196,7 +195,6 @@ static int mp_property_stream_pos(m_option_t *prop, int action, void *arg,
         *(int64_t *) arg = stream_tell(stream);
         return M_PROPERTY_OK;
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int64_t *) arg);
         stream_seek(stream, *(int64_t *) arg);
         return M_PROPERTY_OK;
     }
@@ -286,7 +284,6 @@ static int mp_property_percent_pos(m_option_t *prop, int action,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *)arg);
         pos = *(int *)arg;
         break;
     default:
@@ -306,7 +303,6 @@ static int mp_property_time_pos(m_option_t *prop, int action,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(double *)arg);
         queue_seek(mpctx, MPSEEK_ABSOLUTE, *(double *)arg, 0);
         return M_PROPERTY_OK;
     }
@@ -337,7 +333,6 @@ static int mp_property_chapter(m_option_t *prop, int action, void *arg,
         return M_PROPERTY_OK;
     }
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *)arg);
         step_all = *(int *)arg - chapter;
         chapter += step_all;
         break;
@@ -380,7 +375,6 @@ static int mp_property_edition(m_option_t *prop, int action, void *arg,
     case M_PROPERTY_PRINT:
         return m_property_int_ro(prop, action, arg, edition);
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *)arg);
         edition = *(int *)arg;
         break;
     case M_PROPERTY_SWITCH: {
@@ -464,7 +458,6 @@ static int mp_property_angle(m_option_t *prop, int action, void *arg,
     }
     case M_PROPERTY_SET:
         angle = *(int *)arg;
-        M_PROPERTY_CLAMP(prop, angle);
         break;
     case M_PROPERTY_SWITCH:
         // NOTE: should cycle
@@ -566,7 +559,6 @@ static int mp_property_volume(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(float *) arg);
         mixer_setvolume(&mpctx->mixer, *(float *) arg, *(float *) arg);
         return M_PROPERTY_OK;
     case M_PROPERTY_SWITCH:
@@ -721,7 +713,6 @@ static int mp_property_balance(m_option_t *prop, int action, void *arg,
         return M_PROPERTY_OK;
     }
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(float *)arg);
         mixer_setbalance(&mpctx->mixer, *(float *)arg);
         return M_PROPERTY_OK;
     }
@@ -850,7 +841,6 @@ static int mp_property_fullscreen(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *) arg);
         if (vo_fs == !!*(int *) arg)
             return M_PROPERTY_OK;
         if (mpctx->video_out->config_ok)
@@ -874,7 +864,6 @@ static int mp_property_deinterlace(m_option_t *prop, int action,
         vf->control(vf, VFCTRL_GET_DEINTERLACE, arg);
         return M_PROPERTY_OK;
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *) arg);
         vf->control(vf, VFCTRL_SET_DEINTERLACE, arg);
         return M_PROPERTY_OK;
     }
@@ -997,7 +986,6 @@ static int mp_property_panscan(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(float *) arg);
         vo_panscan = *(float *) arg;
         vo_control(mpctx->video_out, VOCTRL_SET_PANSCAN, NULL);
         return M_PROPERTY_OK;
@@ -1018,7 +1006,6 @@ static int mp_property_vo_flag(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *) arg);
         if (*vo_var == !!*(int *) arg)
             return M_PROPERTY_OK;
         if (mpctx->video_out->config_ok)
@@ -1079,7 +1066,6 @@ static int mp_property_gamma(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *) arg);
         *gamma = *(int *) arg;
         r = set_video_colors(mpctx->sh_video, prop->name, *gamma);
         if (r <= 0)
@@ -1323,7 +1309,6 @@ static int mp_property_sub_scale(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(float *) arg);
         if (opts->ass_enabled)
             opts->ass_font_scale = *(float *) arg;
         else
@@ -1358,7 +1343,6 @@ static int mp_property_tv_color(m_option_t *prop, int action, void *arg,
 
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *) arg);
         return tv_set_color_options(tvh, prop->offset, *(int *) arg);
     case M_PROPERTY_GET:
         return tv_get_color_options(tvh, prop->offset, arg);
diff --git a/m_option.c b/m_option.c
index 48539b3a4c..d48d4016b8 100644
--- a/m_option.c
+++ b/m_option.c
@@ -90,6 +90,14 @@ static void copy_opt(const m_option_t *opt, void *dst, const void *src)
 
 #define VAL(x) (*(int *)(x))
 
+static int clamp_flag(const m_option_t *opt, void *val)
+{
+    if (VAL(val) == opt->min || VAL(val) == opt->max)
+        return 0;
+    VAL(val) = opt->min;
+    return M_OPT_OUT_OF_RANGE;
+}
+
 static int parse_flag(const m_option_t *opt, struct bstr name,
                       struct bstr param, void *dst)
 {
@@ -141,12 +149,29 @@ const m_option_type_t m_option_type_flag = {
     .print = print_flag,
     .copy  = copy_opt,
     .add = add_flag,
+    .clamp = clamp_flag,
 };
 
 // Integer
 
 #undef VAL
 
+static int clamp_longlong(const m_option_t *opt, void *val)
+{
+    long long v = *(long long *)val;
+    int r = 0;
+    if ((opt->flags & M_OPT_MAX) && (v > opt->max)) {
+        v = opt->max;
+        r = M_OPT_OUT_OF_RANGE;
+    }
+    if ((opt->flags & M_OPT_MIN) && (v < opt->min)) {
+        v = opt->min;
+        r = M_OPT_OUT_OF_RANGE;
+    }
+    *(long long *)val = v;
+    return r;
+}
+
 static int parse_longlong(const m_option_t *opt, struct bstr name,
                           struct bstr param, void *dst)
 {
@@ -184,6 +209,22 @@ static int parse_longlong(const m_option_t *opt, struct bstr name,
     return 1;
 }
 
+static int clamp_int(const m_option_t *opt, void *val)
+{
+    long long tmp = *(int *)val;
+    int r = clamp_longlong(opt, &tmp);
+    *(int *)val = tmp;
+    return r;
+}
+
+static int clamp_int64(const m_option_t *opt, void *val)
+{
+    long long tmp = *(int64_t *)val;
+    int r = clamp_longlong(opt, &tmp);
+    *(int64_t *)val = tmp;
+    return r;
+}
+
 static int parse_int(const m_option_t *opt, struct bstr name,
                      struct bstr param, void *dst)
 {
@@ -204,7 +245,6 @@ static int parse_int64(const m_option_t *opt, struct bstr name,
     return r;
 }
 
-
 static char *print_int(const m_option_t *opt, const void *val)
 {
     if (opt->type->size == sizeof(int64_t))
@@ -247,6 +287,7 @@ const m_option_type_t m_option_type_int = {
     .print = print_int,
     .copy  = copy_opt,
     .add = add_int,
+    .clamp = clamp_int,
 };
 
 const m_option_type_t m_option_type_int64 = {
@@ -256,6 +297,7 @@ const m_option_type_t m_option_type_int64 = {
     .print = print_int,
     .copy  = copy_opt,
     .add = add_int64,
+    .clamp = clamp_int64,
 };
 
 static int parse_intpair(const struct m_option *opt, struct bstr name,
@@ -301,6 +343,21 @@ const struct m_option_type m_option_type_intpair = {
     .copy  = copy_opt,
 };
 
+static int clamp_choice(const m_option_t *opt, void *val)
+{
+    int v = *(int *)val;
+    if ((opt->flags & M_OPT_MIN) && (opt->flags & M_OPT_MAX)) {
+        if (v >= opt->min && v <= opt->max)
+            return 0;
+    }
+    ;
+    for (struct m_opt_choice_alternatives *alt = opt->priv; alt->name; alt++) {
+        if (alt->value == v)
+            return 0;
+    }
+    return M_OPT_INVALID;
+}
+
 static int parse_choice(const struct m_option *opt, struct bstr name,
                         struct bstr param, void *dst)
 {
@@ -416,6 +473,7 @@ const struct m_option_type m_option_type_choice = {
     .print = print_choice,
     .copy  = copy_opt,
     .add = add_choice,
+    .clamp = clamp_choice,
 };
 
 // Float
@@ -423,6 +481,22 @@ const struct m_option_type m_option_type_choice = {
 #undef VAL
 #define VAL(x) (*(double *)(x))
 
+static int clamp_double(const m_option_t *opt, void *val)
+{
+    double v = VAL(val);
+    int r = 0;
+    if ((opt->flags & M_OPT_MAX) && (v > opt->max)) {
+        v = opt->max;
+        r = M_OPT_OUT_OF_RANGE;
+    }
+    if ((opt->flags & M_OPT_MIN) && (v < opt->min)) {
+        v = opt->min;
+        r = M_OPT_OUT_OF_RANGE;
+    }
+    VAL(val) = v;
+    return r;
+}
+
 static int parse_double(const m_option_t *opt, struct bstr name,
                         struct bstr param, void *dst)
 {
@@ -514,11 +588,20 @@ const m_option_type_t m_option_type_double = {
     .print = print_double,
     .pretty_print = print_double_f2,
     .copy  = copy_opt,
+    .clamp = clamp_double,
 };
 
 #undef VAL
 #define VAL(x) (*(float *)(x))
 
+static int clamp_float(const m_option_t *opt, void *val)
+{
+    double tmp = VAL(val);
+    int r = clamp_double(opt, &tmp);
+    VAL(val) = tmp;
+    return r;
+}
+
 static int parse_float(const m_option_t *opt, struct bstr name,
                        struct bstr param, void *dst)
 {
@@ -555,6 +638,7 @@ const m_option_type_t m_option_type_float = {
     .pretty_print = print_float_f2,
     .copy  = copy_opt,
     .add = add_float,
+    .clamp = clamp_float,
 };
 
 ///////////// String
@@ -562,6 +646,17 @@ const m_option_type_t m_option_type_float = {
 #undef VAL
 #define VAL(x) (*(char **)(x))
 
+static int clamp_str(const m_option_t *opt, void *val)
+{
+    char *v = VAL(val);
+    int len = v ? strlen(v) : 0;
+    if ((opt->flags & M_OPT_MIN) && (len < opt->min))
+        return M_OPT_OUT_OF_RANGE;
+    if ((opt->flags & M_OPT_MAX) && (len > opt->max))
+        return M_OPT_OUT_OF_RANGE;
+    return 0;
+}
+
 static int parse_str(const m_option_t *opt, struct bstr name,
                      struct bstr param, void *dst)
 {
@@ -620,6 +715,7 @@ const m_option_type_t m_option_type_string = {
     .print = print_str,
     .copy  = copy_str,
     .free  = free_str,
+    .clamp = clamp_str,
 };
 
 //////////// String list
@@ -1150,6 +1246,7 @@ const m_option_type_t m_option_type_time = {
     .pretty_print = pretty_print_time,
     .copy  = copy_opt,
     .add = add_double,
+    .clamp = clamp_double,
 };
 
 
diff --git a/m_option.h b/m_option.h
index 11e9e620fb..166e38e999 100644
--- a/m_option.h
+++ b/m_option.h
@@ -251,6 +251,13 @@ struct m_option_type {
     // add gives merely the direction. The wrap parameter determines whether
     // the value is clipped, or wraps around to the opposite max/min.
     void (*add)(const m_option_t *opt, void *val, double add, bool wrap);
+
+    // Clamp the value in val to the option's valid value range.
+    // Return values:
+    //  M_OPT_OUT_OF_RANGE: val was invalid, and modified (clamped) to be valid
+    //  M_OPT_INVALID:      val was invalid, and can't be made valid
+    //  0:                  val was already valid and is unchanged
+    int (*clamp)(const m_option_t *opt, void *val);
 };
 
 // Option description
diff --git a/m_property.c b/m_property.c
index cb19ff08cb..af5d51dd21 100644
--- a/m_property.c
+++ b/m_property.c
@@ -129,6 +129,24 @@ int m_property_do(const m_option_t *prop_list, const char *name,
         r = do_action(prop_list, name, M_PROPERTY_SET, &val, ctx);
         m_option_free(opt, &val);
         return r;
+    case M_PROPERTY_SET:
+        if ((r =
+             do_action(prop_list, name, M_PROPERTY_GET_TYPE, &opt, ctx)) <= 0)
+            return r;
+        if (!opt->type->clamp) {
+            mp_msg(MSGT_CPLAYER, MSGL_WARN, "Property '%s' without clamp().\n",
+                   name);
+        } else {
+            m_option_copy(opt, &val, arg);
+            r = opt->type->clamp(opt, arg);
+            m_option_free(opt, &val);
+            if (r != 0) {
+                mp_msg(MSGT_CPLAYER, MSGL_ERR,
+                       "Property '%s': invalid value.\n", name);
+                return M_PROPERTY_ERROR;
+            }
+        }
+        return do_action(prop_list, name, M_PROPERTY_SET, arg, ctx);
     }
     return do_action(prop_list, name, action, arg, ctx);
 }
@@ -270,7 +288,6 @@ int m_property_int_range(const m_option_t *prop, int action,
 {
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(int *)arg);
         *var = *(int *)arg;
         return 1;
     }
@@ -305,7 +322,6 @@ int m_property_float_range(const m_option_t *prop, int action,
 {
     switch (action) {
     case M_PROPERTY_SET:
-        M_PROPERTY_CLAMP(prop, *(float *)arg);
         *var = *(float *)arg;
         return 1;
     }
diff --git a/m_property.h b/m_property.h
index 462dad57fc..eba61a1619 100644
--- a/m_property.h
+++ b/m_property.h
@@ -159,14 +159,6 @@ char* mp_property_print(const char *name, void* ctx);
 /// \brief Helper functions for common property types.
 ///@{
 
-/// Clamp a value according to \ref m_option::min and \ref m_option::max.
-#define M_PROPERTY_CLAMP(prop,val) do {                                 \
-        if(((prop)->flags & M_OPT_MIN) && (val) < (prop)->min)          \
-            (val) = (prop)->min;                                        \
-        else if(((prop)->flags & M_OPT_MAX) && (val) > (prop)->max)     \
-            (val) = (prop)->max;                                        \
-    } while(0)
-
 /// Implement get.
 int m_property_int_ro(const m_option_t* prop,int action,
                       void* arg,int var);