client API: improve mpv_set_property() handling of MPV_FORMAT_NODE

If a mpv_node wrapped a string, the behavior was different from calling
mpv_set_property() with MPV_FORMAT_STRING directly. Change this.

The original intention was to be strict about types if MPV_FORMAT_NODE
is used. But I think the result was less than ideal, and the same change
towards less strict behavior was made to mpv_set_option() ages ago.

DOCS/client-api-changes.rst

@@ -32,6 +32,15 @@ API changes
 
 ::
 
+ --- mpv 0.17.1 ---
+ 1.21   - mpv_set_property() changes behavior with MPV_FORMAT_NODE. Before this
+          change it rejected mpv_nodes with format==MPV_FORMAT_STRING if the
+          property was not a string or did not have special mechanisms in place
+          the function failed. Now it always invokes the option string parser,
+          and mpv_node with a basic data type works exactly as if the function
+          is invoked with that type directly. This new behavior is equivalent
+          to mpv_set_option().
+          This also affects the mp.set_property_native() Lua function.
  --- mpv 0.12.0 ---
  1.20   - deprecate "GL_MP_D3D_interfaces"/"glMPGetD3DInterface", and introduce
           "GL_MP_MPGetNativeDisplay"/"glMPGetNativeDisplay" (this is a

libmpv/client.h

@@ -215,7 +215,7 @@ extern "C" {
  * relational operators (<, >, <=, >=).
  */
 #define MPV_MAKE_VERSION(major, minor) (((major) << 16) | (minor) | 0UL)
-#define MPV_CLIENT_API_VERSION MPV_MAKE_VERSION(1, 20)
+#define MPV_CLIENT_API_VERSION MPV_MAKE_VERSION(1, 21)
 
 /**
  * Return the MPV_CLIENT_API_VERSION the mpv source has been compiled with.
@@ -780,6 +780,10 @@ void mpv_free_node_contents(mpv_node *node);
  * mpv_set_property() to change settings during playback, because the property
  * mechanism guarantees that changes take effect immediately.
  *
+ * Using a format other than MPV_FORMAT_NODE is equivalent to constructing a
+ * mpv_node with the given format and data, and passing the mpv_node to this
+ * function.
+ *
  * @param name Option name. This is the same as on the mpv command line, but
  *             without the leading "--".
  * @param format see enum mpv_format.
@@ -877,7 +881,13 @@ int mpv_command_node_async(mpv_handle *ctx, uint64_t reply_userdata,
  * usually will fail with MPV_ERROR_PROPERTY_FORMAT. In some cases, the data
  * is automatically converted and access succeeds. For example, MPV_FORMAT_INT64
  * is always converted to MPV_FORMAT_DOUBLE, and access using MPV_FORMAT_STRING
- * usually invokes a string parser.
+ * usually invokes a string parser. The same happens when calling this function
+ * with MPV_FORMAT_NODE: the underlying format may be converted to another
+ * type if possible.
+ *
+ * Using a format other than MPV_FORMAT_NODE is equivalent to constructing a
+ * mpv_node with the given format and data, and passing the mpv_node to this
+ * function. (Before API version 1.21, this was different.)
  *
  * @param name The property name. See input.rst for a list of properties.
  * @param format see enum mpv_format.

options/m_option.c

@@ -106,6 +106,21 @@ const m_option_t *m_option_list_find(const m_option_t *list, const char *name)
     return m_option_list_findb(list, bstr0(name));
 }
 
+int m_option_set_node_or_string(struct mp_log *log, const m_option_t *opt,
+                                const char *name, void *dst, struct mpv_node *src)
+{
+    if (src->format == MPV_FORMAT_STRING) {
+        // The af and vf option unfortunately require this, because the
+        // option name includes the "action".
+        bstr optname = bstr0(name), a, b;
+        if (bstr_split_tok(optname, "/", &a, &b))
+            optname = b;
+        return m_option_parse(log, opt, optname, bstr0(src->u.string), dst);
+    } else {
+        return m_option_set_node(opt, dst, src);
+    }
+}
+
 // Default function that just does a memcpy
 
 static void copy_opt(const m_option_t *opt, void *dst, const void *src)

options/m_option.h

@@ -504,6 +504,10 @@ static inline int m_option_set_node(const m_option_t *opt, void *dst,
     return M_OPT_UNKNOWN;
 }
 
+// Call m_option_parse for strings, m_option_set_node otherwise.
+int m_option_set_node_or_string(struct mp_log *log, const m_option_t *opt,
+                                const char *name, void *dst, struct mpv_node *src);
+
 // see m_option_type.get
 static inline int m_option_get_node(const m_option_t *opt, void *ta_parent,
                                     struct mpv_node *dst, void *src)

options/m_property.c

@@ -104,16 +104,8 @@ int m_property_do(struct mp_log *log, const struct m_property *prop_list,
         return str != NULL;
     }
     case M_PROPERTY_SET_STRING: {
-        if (!log)
+        struct mpv_node node = { .format = MPV_FORMAT_STRING, .u.string = arg };
-            return M_PROPERTY_ERROR;
+        return do_action(prop_list, name, M_PROPERTY_SET_NODE, &node, ctx);
-        bstr optname = bstr0(name), a, b;
-        if (bstr_split_tok(optname, "/", &a, &b))
-            optname = b;
-        if (m_option_parse(log, &opt, optname, bstr0(arg), &val) < 0)
-            return M_PROPERTY_ERROR;
-        r = do_action(prop_list, name, M_PROPERTY_SET, &val, ctx);
-        m_option_free(&opt, &val);
-        return r;
     }
     case M_PROPERTY_SWITCH: {
         if (!log)
@@ -163,11 +155,12 @@ int m_property_do(struct mp_log *log, const struct m_property *prop_list,
         return r;
     }
     case M_PROPERTY_SET_NODE: {
+        if (!log)
+            return M_PROPERTY_ERROR;
         if ((r = do_action(prop_list, name, M_PROPERTY_SET_NODE, arg, ctx)) !=
             M_PROPERTY_NOT_IMPLEMENTED)
             return r;
-        struct mpv_node *node = arg;
+        int err = m_option_set_node_or_string(log, &opt, name, &val, arg);
-        int err = m_option_set_node(&opt, &val, node);
         if (err == M_OPT_UNKNOWN) {
             r = M_PROPERTY_NOT_IMPLEMENTED;
         } else if (err < 0) {

player/client.c

@@ -1071,44 +1071,28 @@ static void setproperty_fn(void *arg)
     struct setproperty_request *req = arg;
     const struct m_option *type = get_mp_type(req->format);
 
-    int err;
+    struct mpv_node *node;
-    switch (req->format) {
+    struct mpv_node tmp;
-    case MPV_FORMAT_STRING: {
+    if (req->format == MPV_FORMAT_NODE) {
-        // Go through explicit string conversion. M_PROPERTY_SET_NODE doesn't
+        node = req->data;
-        // do this, because it tries to be somewhat type-strict. But the client
+    } else {
-        // needs a way to set everything by string.
+        tmp.format = req->format;
-        char *s = *(char **)req->data;
+        memcpy(&tmp.u, req->data, type->type->size);
-        MP_VERBOSE(req->mpctx, "Set property string: %s='%s'\n", req->name, s);
+        node = &tmp;
-        err = mp_property_do(req->name, M_PROPERTY_SET_STRING, s, req->mpctx);
-        break;
-    }
-    case MPV_FORMAT_NODE:
-    case MPV_FORMAT_FLAG:
-    case MPV_FORMAT_INT64:
-    case MPV_FORMAT_DOUBLE: {
-        struct mpv_node node;
-        if (req->format == MPV_FORMAT_NODE) {
-            node = *(struct mpv_node *)req->data;
-        } else {
-            // These are basically emulated via mpv_node.
-            node.format = req->format;
-            memcpy(&node.u, req->data, type->type->size);
-        }
-        if (mp_msg_test(req->mpctx->log, MSGL_V)) {
-            struct m_option ot = {.type = &m_option_type_node};
-            char *t = m_option_print(&ot, &node);
-            MP_VERBOSE(req->mpctx, "Set property: %s=%s\n", req->name, t ? t : "?");
-            talloc_free(t);
-        }
-        err = mp_property_do(req->name, M_PROPERTY_SET_NODE, &node, req->mpctx);
-        break;
-    }
-    default:
-        abort();
     }
 
+    int err = mp_property_do(req->name, M_PROPERTY_SET_NODE, node, req->mpctx);
+
     req->status = translate_property_error(err);
 
+    if (mp_msg_test(req->mpctx->log, MSGL_V)) {
+        struct m_option ot = {.type = &m_option_type_node};
+        char *t = m_option_print(&ot, node);
+        MP_VERBOSE(req->mpctx, "Set property: %s=%s -> %d\n",
+                   req->name, t ? t : "?", err);
+        talloc_free(t);
+    }
+
     if (req->reply_ctx) {
         status_reply(req->reply_ctx, MPV_EVENT_SET_PROPERTY_REPLY,
                      req->userdata, req->status);