player: make screenshot commands honor the async flag

And also change input.conf to make all screenshots async. (Except the
every-frame mode, which always uses synchronous mode and ignores the
flag.) By default, the "screenshot" command is still asynchronous,
because scripts etc. might depend on this behavior.

This is only partially async. The code for determining the filename is
still always run synchronously. Only encoding the screenshot and writing
it to disk is asynchronous. We explicitly document the exact behavior as
undefined, so it can be changed any time.

Some of this is a bit messy, because I wanted to avoid duplicating the
message display code between sync and async mode. In async mode, this is
called from a worker thread, which is not safe because showing a message
accesses the thread-unsafe OSD code. So the core has to be locked during
this, which implies accessing the core and all that. So the code has
weird locking calls, and we need to do core destruction in a more
"controlled" manner (thus the outstanding_async field).

(What I'd really want would be the OSD simply showing log messages
instead.)

This is pretty untested, so expect bugs.

Fixes #4250.

DOCS/man/input.rst

@@ -186,6 +186,12 @@ 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.
+
 ``screenshot-to-file "<filename>" [subtitles|video|window]``
     Take a screenshot and save it to a given file. The format of the file will
     be guessed by the extension (and ``--screenshot-format`` is ignored - the
@@ -198,6 +204,9 @@ 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.
 

etc/input.conf

@@ -119,9 +119,9 @@
 #_ cycle video
 #T cycle ontop                          # toggle video window ontop of other windows
 #f cycle fullscreen                     # toggle fullscreen
-#s screenshot                           # take a screenshot
-#S screenshot video                     # ...without subtitles
-#Ctrl+s screenshot window               # ...with subtitles and OSD, and scaled
+#s async screenshot                     # take a screenshot
+#S async screenshot video               # ...without subtitles
+#Ctrl+s async 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
 #e add panscan +0.1                     #      in

player/command.c

@@ -4827,6 +4827,7 @@ int run_command(struct MPContext *mpctx, struct mp_cmd *cmd, struct mpv_node *re
     bool bar_osd = auto_osd || (on_osd & MP_ON_OSD_BAR);
     bool msg_or_nobar_osd = msg_osd && !(auto_osd && opts->osd_bar_visible);
     int osdl = msg_osd ? 1 : OSD_LEVEL_INVISIBLE;
+    bool async = cmd->flags & MP_ASYNC_CMD;
 
     mp_cmd_dump(mpctx->log, cmd->id == MP_CMD_IGNORE ? MSGL_DEBUG : MSGL_V,
                 "Run command:", cmd);
@@ -5366,12 +5367,13 @@ int run_command(struct MPContext *mpctx, struct mp_cmd *cmd, struct mpv_node *re
     case MP_CMD_SCREENSHOT: {
         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, msg_osd);
+        screenshot_request(mpctx, mode, freq, msg_osd, async);
         break;
     }
 
     case MP_CMD_SCREENSHOT_TO_FILE:
-        screenshot_to_file(mpctx, cmd->args[0].v.s, cmd->args[1].v.i, msg_osd);
+        screenshot_to_file(mpctx, cmd->args[0].v.s, cmd->args[1].v.i, msg_osd,
+                           async);
         break;
 
     case MP_CMD_SCREENSHOT_RAW: {

player/core.h

@@ -239,6 +239,12 @@ typedef struct MPContext {
     struct mp_dispatch_queue *dispatch;
     struct mp_cancel *playback_abort;
     bool in_dispatch;
+    // Number of asynchronous tasks that still need to finish until MPContext
+    // destruction is ok. It's implied that the async tasks call
+    // mp_wakeup_core() each time this is decremented.
+    // As using an atomic+wakeup would be racy, this is a normal integer, and
+    // mp_dispatch_lock must be called to change it.
+    int64_t outstanding_async;
 
     struct mp_log *statusline;
     struct osd_state *osd;

player/main.c

@@ -150,7 +150,7 @@ void mp_print_version(struct mp_log *log, int always)
 static void shutdown_clients(struct MPContext *mpctx)
 {
     mp_client_enter_shutdown(mpctx);
-    while (mp_clients_num(mpctx)) {
+    while (mp_clients_num(mpctx) || mpctx->outstanding_async) {
         mp_client_broadcast_event(mpctx, MPV_EVENT_SHUTDOWN, NULL);
         mp_wait_events(mpctx);
     }

player/screenshot.c

@@ -28,6 +28,8 @@
 #include "core.h"
 #include "command.h"
 #include "misc/bstr.h"
+#include "misc/dispatch.h"
+#include "misc/thread_pool.h"
 #include "common/msg.h"
 #include "options/path.h"
 #include "video/mp_image.h"
@@ -50,6 +52,8 @@ typedef struct screenshot_ctx {
     bool osd;
 
     int frameno;
+
+    struct mp_thread_pool *thread_pool;
 } screenshot_ctx;
 
 void screenshot_init(struct MPContext *mpctx)
@@ -63,6 +67,7 @@ void screenshot_init(struct MPContext *mpctx)
 
 #define SMSG_OK 0
 #define SMSG_ERR 1
+#define SMSG_V 2
 
 static void screenshot_msg(screenshot_ctx *ctx, int status, const char *msg,
                            ...) PRINTF_ATTRIBUTE(3,4);
@@ -77,8 +82,14 @@ static void screenshot_msg(screenshot_ctx *ctx, int status, const char *msg,
     s = talloc_vasprintf(NULL, msg, ap);
     va_end(ap);
 
-    MP_MSG(ctx->mpctx, status == SMSG_ERR ? MSGL_ERR : MSGL_INFO, "%s\n", s);
-    if (ctx->osd)
+    int msg_level = MSGL_INFO;
+    if (status == SMSG_ERR)
+        msg_level = MSGL_ERR;
+    if (status == SMSG_V)
+        msg_level = MSGL_V;
+
+    MP_MSG(ctx->mpctx, msg_level, "%s\n", s);
+    if (ctx->osd && msg_level <= MSGL_INFO)
         set_osd_msg(ctx->mpctx, 1, ctx->mpctx->opts->osd_duration, "%s", s);
 
     talloc_free(s);
@@ -92,6 +103,75 @@ 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, SMSG_OK, "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, SMSG_ERR, "Error writing screenshot!");
+        UNLOCK(item)
+    }
+
+    if (item->on_thread) {
+        mp_dispatch_lock(item->mpctx->dispatch);
+        screenshot_msg(ctx, SMSG_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)
+{
+    screenshot_ctx *ctx = mpctx->screenshot_ctx;
+    struct image_writer_opts *gopts = mpctx->opts->screenshot_image_opts;
+
+    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,
+    };
+
+    if (async) {
+        if (!ctx->thread_pool)
+            ctx->thread_pool = mp_thread_pool_create(ctx, 1);
+        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;
+        }
+    }
+
+    if (item)
+        write_screenshot_thread(item);
+}
+
 #ifdef _WIN32
 #define ILLEGAL_FILENAME_CHARS "?\"/\\<>*|:"
 #else
@@ -315,21 +395,6 @@ static void add_subs(struct MPContext *mpctx, struct mp_image *image)
                       OSD_DRAW_SUB_ONLY, image);
 }
 
-static void screenshot_save(struct MPContext *mpctx, struct mp_image *image)
-{
-    screenshot_ctx *ctx = mpctx->screenshot_ctx;
-
-    struct image_writer_opts *opts = mpctx->opts->screenshot_image_opts;
-
-    char *filename = gen_fname(ctx, image_writer_file_ext(opts));
-    if (filename) {
-        screenshot_msg(ctx, SMSG_OK, "Screenshot: '%s'", filename);
-        if (!write_image(image, opts, filename, mpctx->log))
-            screenshot_msg(ctx, SMSG_ERR, "Error writing screenshot!");
-        talloc_free(filename);
-    }
-}
-
 static struct mp_image *screenshot_get(struct MPContext *mpctx, int mode)
 {
     struct mp_image *image = NULL;
@@ -376,7 +441,7 @@ struct mp_image *screenshot_get_rgb(struct MPContext *mpctx, int mode)
 }
 
 void screenshot_to_file(struct MPContext *mpctx, const char *filename, int mode,
-                        bool osd)
+                        bool osd, bool async)
 {
     screenshot_ctx *ctx = mpctx->screenshot_ctx;
     struct image_writer_opts opts = *mpctx->opts->screenshot_image_opts;
@@ -392,9 +457,7 @@ void screenshot_to_file(struct MPContext *mpctx, const char *filename, int mode,
         screenshot_msg(ctx, SMSG_ERR, "Taking screenshot failed.");
         goto end;
     }
-    screenshot_msg(ctx, SMSG_OK, "Screenshot: '%s'", filename);
-    if (!write_image(image, &opts, filename, mpctx->log))
-        screenshot_msg(ctx, SMSG_ERR, "Error writing screenshot!");
+    write_screenshot(mpctx, image, filename, &opts, async);
     talloc_free(image);
 
 end:
@@ -402,7 +465,7 @@ end:
 }
 
 void screenshot_request(struct MPContext *mpctx, int mode, bool each_frame,
-                        bool osd)
+                        bool osd, bool async)
 {
     screenshot_ctx *ctx = mpctx->screenshot_ctx;
 
@@ -423,7 +486,11 @@ void screenshot_request(struct MPContext *mpctx, int mode, bool each_frame,
     struct mp_image *image = screenshot_get(mpctx, mode);
 
     if (image) {
-        screenshot_save(mpctx, image);
+        struct image_writer_opts *opts = mpctx->opts->screenshot_image_opts;
+        char *filename = gen_fname(ctx, image_writer_file_ext(opts));
+        if (filename)
+            write_screenshot(mpctx, image, filename, NULL, async);
+        talloc_free(filename);
     } else {
         screenshot_msg(ctx, SMSG_ERR, "Taking screenshot failed.");
     }
@@ -439,5 +506,5 @@ void screenshot_flip(struct MPContext *mpctx)
         return;
 
     ctx->each_frame = false;
-    screenshot_request(mpctx, ctx->mode, true, ctx->osd);
+    screenshot_request(mpctx, ctx->mode, true, ctx->osd, false);
 }

player/screenshot.h

@@ -31,13 +31,13 @@ void screenshot_init(struct MPContext *mpctx);
 //             screenshot slave command (MP_CMD_SCREENSHOT).
 // osd: show status on OSD
 void screenshot_request(struct MPContext *mpctx, int mode, bool each_frame,
-                        bool osd);
+                        bool osd, bool async);
 
 // 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()
 void screenshot_to_file(struct MPContext *mpctx, const char *filename, int mode,
-                        bool osd);
+                        bool osd, bool async);
 
 // mode is the same as in screenshot_request()
 struct mp_image *screenshot_get_rgb(struct MPContext *mpctx, int mode);

wscript_build.py

@@ -215,6 +215,7 @@ def build(ctx):
         ( "misc/node.c" ),
         ( "misc/ring.c" ),
         ( "misc/rendezvous.c" ),
+        ( "misc/thread_pool.c" ),
 
         ## Options
         ( "options/m_config.c" ),