screenshot: change async behavior to be in line with new semantics

Basically reimplement the async behavior on top of the async command
code. With this, all screenshot commands are async, and the "async"
prefix basically does nothing. The prefix now behaves exactly like with
other commands that use spawn_thread.

This also means using the prefix in the preset input.conf is pointless
(without effect) and misleading, so remove that.

The each_frame mode was actually particularly painful in making this
change, since the player wants to block for it when writing a
screenshot, and generally doesn't fit into the new infrastructure. It
was still relatively easy to reimplement by copying the original command
and then repeating it on each frame. The waiting is reentrant now, so
move the call in video.c to a "safer" spot.

One way to observe how the new semantics interact with everything is
using the mpv repl script and sending a screenshot command through it.
Without async flag, the script will freeze while writing the screenshot
(while playback continues), while with async flag it continues.

5 files changed, 86 insertions, 130 deletions

diff --git a/DOCS/man/input.rst b/DOCS/man/input.rst
index 94e1ea0b1f..6aa566294f 100644
--- a/DOCS/man/input.rst
+++ b/DOCS/man/input.rst
@@ -186,11 +186,10 @@ List of Input Commands
     second argument (and did not have flags). This syntax is still understood,
     but deprecated and might be removed in the future.
 
-    Setting the ``async`` flag will make encoding and writing the actual image
-    file asynchronous in most cases. (``each-frame`` mode ignores this flag
-    currently.) Requesting async screenshots too early or too often could lead
-    to the same filenames being chosen, and overwriting each others in undefined
-    order.
+    If you combine this command with another one using ``;``, you can use the
+    ``async`` flag to make encoding/writing the image file asynchronous. For
+    normal standalone commands, this is always asynchronous, and the flag has
+    no effect. (This behavior changed with mpv 0.29.0.)
 
 ``screenshot-to-file "<filename>" [subtitles|video|window]``
     Take a screenshot and save it to a given file. The format of the file will
@@ -204,9 +203,6 @@ List of Input Commands
     Like all input command parameters, the filename is subject to property
     expansion as described in `Property Expansion`_.
 
-    The ``async`` flag has an effect on this command (see ``screenshot``
-    command).
-
 ``playlist-next [weak|force]``
     Go to the next entry on the playlist.
 
@@ -905,13 +901,13 @@ command behaves by itself. There are the following cases:
   have an  asynchronous implementation. The async libmpv API still never blocks
   the caller in these cases.
 
-The only exception is the current legacy behavior with screenshot commands,
-which will be fixed later. Using the ``async`` prefix makes them run the file
-saving code in a detached manner.
+Before mpv 0.29.0, the ``async`` prefix was only used by screenshot commands,
+and made them run the file saving code in a detached manner. This is the
+default now, and ``async`` changes behavior only in the ways mentioned above.
 
 Currently the following commands have different waiting characteristics with
 sync vs. async: sub-add, audio-add, sub-reload, audio-reload,
-rescan-external-files.
+rescan-external-files, screenshot, screenshot-to-file.
 
 Input Sections
 --------------
diff --git a/etc/input.conf b/etc/input.conf
index 60cd77cc41..5a9aaccf78 100644
--- a/etc/input.conf
+++ b/etc/input.conf
@@ -133,9 +133,9 @@
 #_ cycle video
 #T cycle ontop                          # toggle video window ontop of other windows
 #f cycle fullscreen                     # toggle fullscreen
-#s async screenshot                     # take a screenshot
-#S async screenshot video               # ...without subtitles
-#Ctrl+s async screenshot window         # ...with subtitles and OSD, and scaled
+#s screenshot                           # take a screenshot
+#S screenshot video                     # ...without subtitles
+#Ctrl+s screenshot window               # ...with subtitles and OSD, and scaled
 #Alt+s screenshot each-frame            # automatically screenshot every frame
 #w add panscan -0.1                     # zoom out with -panscan 0 -fs
 #W add panscan +0.1                     #      in
diff --git a/player/command.c b/player/command.c
index 7d2109be7f..10c58e86d8 100644
--- a/player/command.c
+++ b/player/command.c
@@ -6013,13 +6013,17 @@ const struct mp_cmd_def mp_cmds[] = {
         // backwards compatibility
         OARG_CHOICE(0, ({"unused", 0}, {"single", 0},
                         {"each-frame", 8})),
-    }},
+        },
+        .spawn_thread = true,
+    },
     { "screenshot-to-file", cmd_screenshot_to_file, {
         ARG_STRING,
         OARG_CHOICE(2, ({"video", 0},
                         {"window", 1},
                         {"subtitles", 2})),
-    }},
+        },
+        .spawn_thread = true,
+    },
     { "screenshot-raw", cmd_screenshot_raw, {
         OARG_CHOICE(2, ({"video", 0},
                         {"window", 1},
diff --git a/player/screenshot.c b/player/screenshot.c
index f31628d46c..622d9288b0 100644
--- a/player/screenshot.c
+++ b/player/screenshot.c
@@ -31,7 +31,7 @@
 #include "misc/bstr.h"
 #include "misc/dispatch.h"
 #include "misc/node.h"
-#include "misc/thread_pool.h"
+#include "misc/thread_tools.h"
 #include "common/msg.h"
 #include "options/path.h"
 #include "video/mp_image.h"
@@ -48,13 +48,12 @@
 typedef struct screenshot_ctx {
     struct MPContext *mpctx;
 
-    int mode;
-    bool each_frame;
     bool osd;
 
-    int frameno;
+    // Command to repeat in each-frame mode.
+    struct mp_cmd *each_frame;
 
-    struct mp_thread_pool *thread_pool;
+    int frameno;
 } screenshot_ctx;
 
 void screenshot_init(struct MPContext *mpctx)
@@ -94,73 +93,26 @@ static char *stripext(void *talloc_ctx, const char *s)
     return talloc_asprintf(talloc_ctx, "%.*s", (int)(end - s), s);
 }
 
-struct screenshot_item {
-    bool on_thread;
-    struct MPContext *mpctx;
-    const char *filename;
-    struct mp_image *img;
-    struct image_writer_opts opts;
-};
-
-#define LOCK(item) if (item->on_thread) mp_dispatch_lock(item->mpctx->dispatch);
-#define UNLOCK(item) if (item->on_thread) mp_dispatch_unlock(item->mpctx->dispatch);
-
-static void write_screenshot_thread(void *arg)
-{
-    struct screenshot_item *item = arg;
-    screenshot_ctx *ctx = item->mpctx->screenshot_ctx;
-
-    LOCK(item)
-    screenshot_msg(ctx, MSGL_INFO, "Screenshot: '%s'", item->filename);
-    UNLOCK(item)
-
-    if (!item->img || !write_image(item->img, &item->opts, item->filename,
-                                   item->mpctx->log))
-    {
-        LOCK(item)
-        screenshot_msg(ctx, MSGL_ERR, "Error writing screenshot!");
-        UNLOCK(item)
-    }
-
-    if (item->on_thread) {
-        mp_dispatch_lock(item->mpctx->dispatch);
-        screenshot_msg(ctx, MSGL_V, "Screenshot writing done.");
-        item->mpctx->outstanding_async -= 1;
-        mp_wakeup_core(item->mpctx);
-        mp_dispatch_unlock(item->mpctx->dispatch);
-    }
-
-    talloc_free(item);
-}
-
 static void write_screenshot(struct MPContext *mpctx, struct mp_image *img,
-                             const char *filename, struct image_writer_opts *opts,
-                             bool async)
+                             const char *filename, struct image_writer_opts *opts)
 {
     screenshot_ctx *ctx = mpctx->screenshot_ctx;
     struct image_writer_opts *gopts = mpctx->opts->screenshot_image_opts;
+    struct image_writer_opts opts_copy = opts ? *opts : *gopts;
 
-    struct screenshot_item *item = talloc_zero(NULL, struct screenshot_item);
-    *item = (struct screenshot_item){
-        .mpctx = mpctx,
-        .filename = talloc_strdup(item, filename),
-        .img = talloc_steal(item, mp_image_new_ref(img)),
-        .opts = opts ? *opts : *gopts,
-    };
+    screenshot_msg(ctx, MSGL_V, "Starting screenshot: '%s'", filename);
 
-    if (async) {
-        if (!ctx->thread_pool)
-            ctx->thread_pool = mp_thread_pool_create(ctx, 1, 1, 3);
-        if (ctx->thread_pool) {
-            item->on_thread = true;
-            mpctx->outstanding_async += 1;
-            mp_thread_pool_queue(ctx->thread_pool, write_screenshot_thread, item);
-            item = NULL;
-        }
-    }
+    mp_core_unlock(mpctx);
 
-    if (item)
-        write_screenshot_thread(item);
+    bool ok = img && write_image(img, &opts_copy, filename, mpctx->log);
+
+    mp_core_lock(mpctx);
+
+    if (ok) {
+        screenshot_msg(ctx, MSGL_INFO, "Screenshot: '%s'", filename);
+    } else {
+        screenshot_msg(ctx, MSGL_ERR, "Error writing screenshot!");
+    }
 }
 
 #ifdef _WIN32
@@ -434,7 +386,7 @@ static struct mp_image *screenshot_get(struct MPContext *mpctx, int mode,
     return image;
 }
 
-// mode is the same as in screenshot_request()
+// mode is the same as in screenshot_get()
 static struct mp_image *screenshot_get_rgb(struct MPContext *mpctx, int mode)
 {
     struct mp_image *mpi = screenshot_get(mpctx, mode, false);
@@ -445,12 +397,13 @@ static struct mp_image *screenshot_get_rgb(struct MPContext *mpctx, int mode)
     return res;
 }
 
-// filename: where to store the screenshot; doesn't try to find an alternate
-//           name if the file already exists
-// mode, osd: same as in screenshot_request()
-static void screenshot_to_file(struct MPContext *mpctx, const char *filename,
-                               int mode, bool osd, bool async)
+void cmd_screenshot_to_file(void *p)
 {
+    struct mp_cmd_ctx *cmd = p;
+    struct MPContext *mpctx = cmd->mpctx;
+    const char *filename = cmd->args[0].v.s;
+    int mode = cmd->args[1].v.i;
+    bool osd = cmd->msg_osd;
     screenshot_ctx *ctx = mpctx->screenshot_ctx;
     struct image_writer_opts opts = *mpctx->opts->screenshot_image_opts;
     bool old_osd = ctx->osd;
@@ -462,39 +415,42 @@ static void screenshot_to_file(struct MPContext *mpctx, const char *filename,
         opts.format = format;
     bool high_depth = image_writer_high_depth(&opts);
     struct mp_image *image = screenshot_get(mpctx, mode, high_depth);
+    ctx->osd = old_osd;
     if (!image) {
         screenshot_msg(ctx, MSGL_ERR, "Taking screenshot failed.");
-        goto end;
+        return;
     }
-    write_screenshot(mpctx, image, filename, &opts, async);
+    write_screenshot(mpctx, image, filename, &opts);
     talloc_free(image);
-
-end:
-    ctx->osd = old_osd;
 }
 
-// Request a taking & saving a screenshot of the currently displayed frame.
-// mode: 0: -, 1: save the actual output window contents, 2: with subtitles.
-// each_frame: If set, this toggles per-frame screenshots, exactly like the
-//             screenshot slave command (MP_CMD_SCREENSHOT).
-// osd: show status on OSD
-static void screenshot_request(struct MPContext *mpctx, int mode, bool each_frame,
-                               bool osd, bool async)
+void cmd_screenshot(void *p)
 {
+    struct mp_cmd_ctx *cmd = p;
+    struct MPContext *mpctx = cmd->mpctx;
+    int mode = cmd->args[0].v.i & 3;
+    bool each_frame_toggle = (cmd->args[0].v.i | cmd->args[1].v.i) & 8;
+    bool each_frame_mode = cmd->args[0].v.i & 16;
+    bool osd = cmd->msg_osd;
+
     screenshot_ctx *ctx = mpctx->screenshot_ctx;
 
     if (mode == MODE_SUBTITLES && osd_get_render_subs_in_filter(mpctx->osd))
         mode = 0;
 
-    if (each_frame) {
-        ctx->each_frame = !ctx->each_frame;
-        if (!ctx->each_frame)
-            return;
-    } else {
-        ctx->each_frame = false;
+    if (!each_frame_mode) {
+        if (each_frame_toggle) {
+            if (ctx->each_frame) {
+                TA_FREEP(&ctx->each_frame);
+                return;
+            }
+            ctx->each_frame = talloc_steal(ctx, mp_cmd_clone(cmd->cmd));
+            ctx->each_frame->args[0].v.i |= 16;
+        } else {
+            TA_FREEP(&ctx->each_frame);
+        }
     }
 
-    ctx->mode = mode;
     ctx->osd = osd;
 
     struct image_writer_opts *opts = mpctx->opts->screenshot_image_opts;
@@ -505,7 +461,7 @@ static void screenshot_request(struct MPContext *mpctx, int mode, bool each_fram
     if (image) {
         char *filename = gen_fname(ctx, image_writer_file_ext(opts));
         if (filename)
-            write_screenshot(mpctx, image, filename, NULL, async);
+            write_screenshot(mpctx, image, filename, NULL);
         talloc_free(filename);
     } else {
         screenshot_msg(ctx, MSGL_ERR, "Taking screenshot failed.");
@@ -514,25 +470,6 @@ static void screenshot_request(struct MPContext *mpctx, int mode, bool each_fram
     talloc_free(image);
 }
 
-void cmd_screenshot(void *p)
-{
-    struct mp_cmd_ctx *cmd = p;
-    struct MPContext *mpctx = cmd->mpctx;
-    bool async = cmd->cmd->flags & MP_ASYNC_CMD;
-    int mode = cmd->args[0].v.i & 3;
-    int freq = (cmd->args[0].v.i | cmd->args[1].v.i) >> 3;
-    screenshot_request(mpctx, mode, freq, cmd->msg_osd, async);
-}
-
-void cmd_screenshot_to_file(void *p)
-{
-    struct mp_cmd_ctx *cmd = p;
-    struct MPContext *mpctx = cmd->mpctx;
-    bool async = cmd->cmd->flags & MP_ASYNC_CMD;
-    screenshot_to_file(mpctx, cmd->args[0].v.s, cmd->args[1].v.i, cmd->msg_osd,
-                       async);
-}
-
 void cmd_screenshot_raw(void *p)
 {
     struct mp_cmd_ctx *cmd = p;
@@ -559,6 +496,16 @@ void cmd_screenshot_raw(void *p)
     talloc_steal(ba, img);
 }
 
+static void screenshot_fin(struct mp_cmd_ctx *cmd)
+{
+    void **a = cmd->on_completion_priv;
+    struct MPContext *mpctx = a[0];
+    struct mp_waiter *waiter = a[1];
+
+    mp_waiter_wakeup(waiter, 0);
+    mp_wakeup_core(mpctx);
+}
+
 void screenshot_flip(struct MPContext *mpctx)
 {
     screenshot_ctx *ctx = mpctx->screenshot_ctx;
@@ -566,6 +513,14 @@ void screenshot_flip(struct MPContext *mpctx)
     if (!ctx->each_frame)
         return;
 
-    ctx->each_frame = false;
-    screenshot_request(mpctx, ctx->mode, true, ctx->osd, false);
+    struct mp_waiter wait = MP_WAITER_INITIALIZER;
+    void *a[] = {mpctx, &wait};
+    run_command(mpctx, mp_cmd_clone(ctx->each_frame), screenshot_fin, a);
+
+    // Block (in a reentrant way) until he screenshot was written. Otherwise,
+    // we could pile up screenshot requests forever.
+    while (!mp_waiter_poll(&wait))
+        mp_idle(mpctx);
+
+    mp_waiter_wait(&wait);
 }
diff --git a/player/video.c b/player/video.c
index fde92851a1..7478c64806 100644
--- a/player/video.c
+++ b/player/video.c
@@ -1168,7 +1168,6 @@ void write_video(struct MPContext *mpctx)
             MP_VERBOSE(mpctx, "first video frame after restart shown\n");
         }
     }
-    screenshot_flip(mpctx);
 
     mp_notify(mpctx, MPV_EVENT_TICK, NULL);
 
@@ -1187,6 +1186,8 @@ void write_video(struct MPContext *mpctx)
             mpctx->max_frames--;
     }
 
+    screenshot_flip(mpctx);
+
     mp_wakeup_core(mpctx);
     return;