command: extend subprocess command

Add env and detach arguments. This means the command.c code must use the
"new" mp_subprocess2(). So also take this as an opportunity to clean up.
win32 support gets broken by it, because it never made the switch to the
newer function.

The new detach parameter makes the "run" command fully redundant, but I
guess we'll keep it for simplicity. But change its implementation to use
mp_subprocess2() (couldn't do this earlier, because win32).

Privately, I'm going to use the "env" argument to add a key binding that
starts a shell with a FILE environment variable set to the currently
playing file, so this is very useful to me.

Note: breaks windows, so for example youtube-dl on windows will not work
anymore. mp_subprocess2() has to be implemented. The old functions are
gone, and subprocess-win.c is not built anymore. It will probably work
on Cygwin.

DOCS/man/input.rst

@@ -523,12 +523,28 @@ Remember to quote string arguments in input.conf (see `Flat command syntax`_).
     ``capture_stderr`` (``MPV_FORMAT_FLAG``)
         Same as ``capture_stdout``, but for stderr.
 
+    ``detach`` (``MPV_FORMAT_FLAG``)
+        Whether to run the process in detached mode (optional, default: no). In
+        this mode, the process is run in a new process session, and the command
+        does not wait for the process to terminate. If neither
+        ``capture_stdout`` nor ``capture_stderr`` have been set to ``yes``,
+        the command returns immediately after the new process has been started,
+        otherwise the command will read as long as the pipes are open.
+
+    ``env`` (``MPV_FORMAT_NODE_ARRAY[MPV_FORMAT_STRING]``)
+        Set a list of environment variables for the new process (default: empty).
+        If an empty list is passed, the environment of the mpv process is used
+        instead. (Unlike the underlying OS mechanisms, the mpv command cannot
+        start a process with empty environment. Fortunately, that is completely
+        useless.) The format of the list is as in the ``execle()`` syscall. Each
+        string item defines an environment variable as in ``NANME=VALUE``.
+
     The command returns the following result (as ``MPV_FORMAT_NODE_MAP``):
 
     ``status`` (``MPV_FORMAT_INT64``)
         The raw exit status of the process. It will be negative on error. The
         meaning of negative values is undefined, other than meaning error (and
-        does not necessarily correspond to OS low level exit status values).
+        does not correspond to OS low level exit status values).
 
         On Windows, it can happen that a negative return value is returned
         even if the process exits gracefully, because the win32 ``UINT`` exit

osdep/subprocess-dummy.c

@@ -1,9 +1,7 @@
 #include "subprocess.h"
 
-int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx,
-                  subprocess_read_cb on_stdout, subprocess_read_cb on_stderr,
-                  char **error)
+void mp_subprocess2(struct mp_subprocess_opts *opts,
+                    struct mp_subprocess_result *res)
 {
-    *error = "unsupported";
-    return -1;
+    *res = (struct mp_subprocess_result){.error = MP_SUBPROCESS_EUNSUPPORTED};
 }

osdep/subprocess.c

@@ -29,108 +29,6 @@ void mp_devnull(void *ctx, char *data, size_t size)
 {
 }
 
-#if HAVE_POSIX
-
-int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx,
-                  subprocess_read_cb on_stdout, subprocess_read_cb on_stderr,
-                  char **error)
-{
-    struct mp_subprocess_opts opts = {
-        .exe = args[0],
-        .args = args,
-        .cancel = cancel,
-    };
-    opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){
-        .fd = 0, // stdin
-        .src_fd = 0,
-    };
-    opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){
-        .fd = 1, // stdout
-        .on_read = on_stdout,
-        .on_read_ctx = ctx,
-        .src_fd = on_stdout ? -1 : 1,
-    };
-    opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){
-        .fd = 2, // stderr
-        .on_read = on_stderr,
-        .on_read_ctx = ctx,
-        .src_fd = on_stderr ? -1 : 2,
-    };
-    struct mp_subprocess_result res;
-    mp_subprocess2(&opts, &res);
-    if (res.error < 0) {
-        *error = (char *)mp_subprocess_err_str(res.error);
-        return res.error;
-    }
-    return res.exit_status;
-}
-
-void mp_subprocess_detached(struct mp_log *log, char **args)
-{
-    mp_msg_flush_status_line(log);
-
-    struct mp_subprocess_opts opts = {
-        .exe = args[0],
-        .args = args,
-        .fds = {
-            {.fd = 0, .src_fd = 0,},
-            {.fd = 1, .src_fd = 1,},
-            {.fd = 2, .src_fd = 2,},
-        },
-        .num_fds = 3,
-        .detach = true,
-    };
-    struct mp_subprocess_result res;
-    mp_subprocess2(&opts, &res);
-    if (res.error < 0) {
-        mp_err(log, "Starting subprocess failed: %s\n",
-               mp_subprocess_err_str(res.error));
-    }
-}
-
-#else
-
-struct subprocess_args {
-    struct mp_log *log;
-    char **args;
-};
-
-static void *run_subprocess(void *ptr)
-{
-    struct subprocess_args *p = ptr;
-    pthread_detach(pthread_self());
-
-    mp_msg_flush_status_line(p->log);
-
-    char *err = NULL;
-    if (mp_subprocess(p->args, NULL, NULL, NULL, NULL, &err) < 0)
-        mp_err(p->log, "Running subprocess failed: %s\n", err);
-
-    talloc_free(p);
-    return NULL;
-}
-
-void mp_subprocess_detached(struct mp_log *log, char **args)
-{
-    struct subprocess_args *p = talloc_zero(NULL, struct subprocess_args);
-    p->log = mp_log_new(p, log, NULL);
-    int num_args = 0;
-    for (int n = 0; args[n]; n++)
-        MP_TARRAY_APPEND(p, p->args, num_args, talloc_strdup(p, args[n]));
-    MP_TARRAY_APPEND(p, p->args, num_args, NULL);
-    pthread_t thread;
-    if (pthread_create(&thread, NULL, run_subprocess, p))
-        talloc_free(p);
-}
-
-void mp_subprocess2(struct mp_subprocess_opts *opts,
-                    struct mp_subprocess_result *res)
-{
-    *res = (struct mp_subprocess_result){.error = MP_SUBPROCESS_EUNSUPPORTED};
-}
-
-#endif
-
 const char *mp_subprocess_err_str(int num)
 {
     // Note: these are visible to the public client API

osdep/subprocess.h

@@ -73,13 +73,4 @@ const char *mp_subprocess_err_str(int num);
 void mp_subprocess2(struct mp_subprocess_opts *opts,
                     struct mp_subprocess_result *res);
 
-// Start a subprocess. Uses callbacks to read from stdout and stderr.
-// Returns any of MP_SUBPROCESS_*, or a value >=0 for the process exir
-int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx,
-                  subprocess_read_cb on_stdout, subprocess_read_cb on_stderr,
-                  char **error);
-
-struct mp_log;
-void mp_subprocess_detached(struct mp_log *log, char **args);
-
 #endif

player/command.c

@@ -5242,48 +5242,54 @@ static void cmd_run(void *p)
     char **args = talloc_zero_array(NULL, char *, cmd->num_args + 1);
     for (int n = 0; n < cmd->num_args; n++)
         args[n] = cmd->args[n].v.s;
-    mp_subprocess_detached(mpctx->log, args);
+    mp_msg_flush_status_line(mpctx->log);
+    struct mp_subprocess_opts opts = {
+        .exe = args[0],
+        .args = args,
+        .fds = { {0, .src_fd = 0}, {1, .src_fd = 1}, {2, .src_fd = 2} },
+        .num_fds = 3,
+        .detach = true,
+    };
+    struct mp_subprocess_result res;
+    mp_subprocess2(&opts, &res);
+    if (res.error < 0) {
+        mp_err(mpctx->log, "Starting subprocess failed: %s\n",
+               mp_subprocess_err_str(res.error));
+    }
     talloc_free(args);
 }
 
-struct subprocess_cb_ctx {
+struct subprocess_fd_ctx {
     struct mp_log *log;
     void* talloc_ctx;
     int64_t max_size;
-    bool capture[3];
-    bstr output[3];
+    int msgl;
+    bool capture;
+    bstr output;
 };
 
-static void subprocess_output(struct subprocess_cb_ctx *ctx, int fd,
-                              char *data, size_t size)
+static void subprocess_read(void *p, char *data, size_t size)
 {
-    if (ctx->capture[fd]) {
-        if (ctx->output[fd].len < ctx->max_size)
-            bstr_xappend(ctx->talloc_ctx, &ctx->output[fd], (bstr){data, size});
+    struct subprocess_fd_ctx *ctx = p;
+    if (ctx->capture) {
+        if (ctx->output.len < ctx->max_size)
+            bstr_xappend(ctx->talloc_ctx, &ctx->output, (bstr){data, size});
     } else {
-        int msgl = fd == 2 ? MSGL_ERR : MSGL_INFO;
-        mp_msg(ctx->log, msgl, "%.*s", (int)size, data);
+        mp_msg(ctx->log, ctx->msgl, "%.*s", (int)size, data);
     }
 }
 
-static void subprocess_stdout(void *p, char *data, size_t size)
-{
-    struct subprocess_cb_ctx *ctx = p;
-    subprocess_output(ctx, 1, data, size);
-}
-
-static void subprocess_stderr(void *p, char *data, size_t size)
-{
-    struct subprocess_cb_ctx *ctx = p;
-    subprocess_output(ctx, 2, data, size);
-}
-
 static void cmd_subprocess(void *p)
 {
     struct mp_cmd_ctx *cmd = p;
     struct MPContext *mpctx = cmd->mpctx;
     char **args = cmd->args[0].v.str_list;
     bool playback_only = cmd->args[1].v.i;
+    bool detach = cmd->args[5].v.i;
+    char **env = cmd->args[6].v.str_list;
+
+    if (env && !env[0])
+        env = NULL; // do not actually set an empty environment
 
     if (!args || !args[0]) {
         MP_ERR(mpctx, "program name missing\n");
@@ -5292,12 +5298,19 @@ static void cmd_subprocess(void *p)
     }
 
     void *tmp = talloc_new(NULL);
-    struct subprocess_cb_ctx ctx = {
-        .log = mp_log_new(tmp, mpctx->log, cmd->cmd->sender),
-        .talloc_ctx = tmp,
-        .max_size = cmd->args[2].v.i,
-        .capture = {0, cmd->args[3].v.i, cmd->args[4].v.i},
-    };
+
+    struct mp_log *fdlog = mp_log_new(tmp, mpctx->log, cmd->cmd->sender);
+    struct subprocess_fd_ctx fdctx[3];
+    for (int fd = 0; fd < 3; fd++) {
+        fdctx[fd] = (struct subprocess_fd_ctx) {
+            .log = fdlog,
+            .talloc_ctx = tmp,
+            .max_size = cmd->args[2].v.i,
+            .msgl = fd == 2 ? MSGL_ERR : MSGL_INFO,
+        };
+    }
+    fdctx[1].capture = cmd->args[3].v.i;
+    fdctx[2].capture = cmd->args[4].v.i;
 
     pthread_mutex_lock(&mpctx->abort_lock);
     cmd->abort->coupled_to_playback = playback_only;
@@ -5306,9 +5319,40 @@ static void cmd_subprocess(void *p)
 
     mp_core_unlock(mpctx);
 
+    struct mp_subprocess_opts opts = {
+        .exe = args[0],
+        .args = args,
+        .env = env,
+        .cancel = cmd->abort->cancel,
+        .detach = detach,
+        .fds = {
+            {
+                .fd = 0, // stdin
+                .src_fd = 0,
+            },
+        },
+        .num_fds = 1,
+    };
+
+    // stdout, stderr
+    for (int fd = 1; fd < 3; fd++) {
+        bool capture = fdctx[fd].capture || !detach;
+        opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){
+            .fd = fd,
+            .src_fd = capture ? -1 : fd,
+            .on_read = capture ? subprocess_read : NULL,
+            .on_read_ctx = &fdctx[fd],
+        };
+    }
+
+    struct mp_subprocess_result sres;
+    mp_subprocess2(&opts, &sres);
+    int status = sres.exit_status;
     char *error = NULL;
-    int status = mp_subprocess(args, cmd->abort->cancel, &ctx,
-                               subprocess_stdout, subprocess_stderr, &error);
+    if (sres.error < 0) {
+        error = (char *)mp_subprocess_err_str(sres.error);
+        status = sres.error;
+    }
 
     mp_core_lock(mpctx);
 
@@ -5318,14 +5362,14 @@ static void cmd_subprocess(void *p)
     node_map_add_flag(res, "killed_by_us", status == MP_SUBPROCESS_EKILLED_BY_US);
     node_map_add_string(res, "error_string", error ? error : "");
     const char *sname[] = {NULL, "stdout", "stderr"};
-    for (int n = 1; n < 3; n++) {
-        if (!ctx.capture[n])
+    for (int fd = 1; fd < 3; fd++) {
+        if (!fdctx[fd].capture)
             continue;
         struct mpv_byte_array *ba =
-            node_map_add(res, sname[n], MPV_FORMAT_BYTE_ARRAY)->u.ba;
+            node_map_add(res, sname[fd], MPV_FORMAT_BYTE_ARRAY)->u.ba;
         *ba = (struct mpv_byte_array){
-            .data = talloc_steal(ba, ctx.output[n].start),
-            .size = ctx.output[n].len,
+            .data = talloc_steal(ba, fdctx[fd].output.start),
+            .size = fdctx[fd].output.len,
         };
     }
 
@@ -5963,6 +6007,8 @@ const struct mp_cmd_def mp_cmds[] = {
                 OPTDEF_INT64(64 * 1024 * 1024)},
             {"capture_stdout", OPT_FLAG(v.i), .flags = MP_CMD_OPT_ARG},
             {"capture_stderr", OPT_FLAG(v.i), .flags = MP_CMD_OPT_ARG},
+            {"detach", OPT_FLAG(v.i), .flags = MP_CMD_OPT_ARG},
+            {"env", OPT_STRINGLIST(v.str_list), .flags = MP_CMD_OPT_ARG},
         },
         .spawn_thread = true,
         .can_abort = true,

test/tests.c

@@ -118,13 +118,19 @@ void assert_text_files_equal_impl(const char *file, int line,
     char *path_ref = mp_tprintf(4096, "%s/%s", ctx->ref_path, ref);
     char *path_new = mp_tprintf(4096, "%s/%s", ctx->out_path, new);
 
-    char *errstr = NULL;
-    int res = mp_subprocess((char*[]){"diff", "-u", "--", path_ref, path_new, 0},
-                            NULL, NULL, NULL, NULL, &errstr);
+    struct mp_subprocess_opts opts = {
+        .exe = "diff",
+        .args = (char*[]){"diff", "-u", "--", path_ref, path_new, 0},
+        .fds = { {0, .src_fd = 0}, {1, .src_fd = 1}, {2, .src_fd = 2} },
+        .num_fds = 3,
+    };
+
+    struct mp_subprocess_result res;
+    mp_subprocess2(&opts, &res);
 
-    if (res) {
-        if (res == 1)
-            MP_WARN(ctx, "Note: %s\n", err);
+    if (res.error || res.exit_status) {
+        if (res.error)
+            MP_WARN(ctx, "Note: %s\n", mp_subprocess_err_str(res.error));
         MP_FATAL(ctx, "Giving up.\n");
         abort();
     }

wscript_build.py

@@ -208,7 +208,7 @@ def swift(task):
 
     subprocess_c = ctx.pick_first_matching_dep([
         ( "osdep/subprocess-posix.c",            "posix" ),
-        ( "osdep/subprocess-win.c",              "win32-desktop" ),
+        # broken ( "osdep/subprocess-win.c",              "win32-desktop" ),
         ( "osdep/subprocess-dummy.c" ),
     ])