client API: cleanup mpv_handle termination

This changes how mpv_terminate_destroy() and mpv_detach_destroy()
behave. The doxygen in client.h tries to point out the differences. The
goal is to make this more useful to the API user (making it behave like
refcounting).

This will be refined in follow up commits.

Initialization is unfortunately closely tied to termination, so that
changes as well. This also removes earlier hacks that make sure that
some parts of FFmpeg initialization are run in the playback thread
(instead of the user's thread). This does not matter with standard
FFmpeg, and I have no reason to care about this anymore.

6 files changed, 150 insertions, 113 deletions

diff --git a/DOCS/client-api-changes.rst b/DOCS/client-api-changes.rst
index 845953580a..49964f5071 100644
--- a/DOCS/client-api-changes.rst
+++ b/DOCS/client-api-changes.rst
@@ -33,6 +33,10 @@ API changes
 ::
 
  --- mpv 0.29.0 ---
+ 1.29   - the behavior of mpv_terminate_destroy() and mpv_detach_destroy()
+          changes subtly (see documentation in the header file). In particular,
+          mpv_detach_destroy() will not leave the player running in all
+          situations anymore (it gets closer to refcounting).
  1.28   - deprecate the render opengl_cb API, and replace it with render.h
           and render_gl.h. The goal is allowing support for APIs other than
           OpenGL. The old API is emulated with the new API.
diff --git a/libmpv/client.h b/libmpv/client.h
index 07237c9d4d..536000229e 100644
--- a/libmpv/client.h
+++ b/libmpv/client.h
@@ -210,7 +210,7 @@ extern "C" {
  * relational operators (<, >, <=, >=).
  */
 #define MPV_MAKE_VERSION(major, minor) (((major) << 16) | (minor) | 0UL)
-#define MPV_CLIENT_API_VERSION MPV_MAKE_VERSION(1, 28)
+#define MPV_CLIENT_API_VERSION MPV_MAKE_VERSION(1, 29)
 
 /**
  * The API user is allowed to "#define MPV_ENABLE_DEPRECATED 0" before
@@ -449,9 +449,17 @@ int mpv_initialize(mpv_handle *ctx);
 
 /**
  * Disconnect and destroy the mpv_handle. ctx will be deallocated with this
- * API call. This leaves the player running. If you want to be sure that the
- * player is terminated, send a "quit" command, and wait until the
- * MPV_EVENT_SHUTDOWN event is received, or use mpv_terminate_destroy().
+ * API call.
+ *
+ * Since mpv client API version 1.29:
+ *  If the last mpv_handle is detached, the core player is destroyed. Note that
+ *  internal mpv_handles created due to scripts (e.g. the OSC) will keep the
+ *  player running. (To be fixed in the following commit.)
+ *
+ * Before mpv client API version 1.29:
+ *  This left the player running. If you want to be sure that the
+ *  player is terminated, send a "quit" command, and wait until the
+ *  MPV_EVENT_SHUTDOWN event is received, or use mpv_terminate_destroy().
  */
 void mpv_detach_destroy(mpv_handle *ctx);
 
@@ -466,9 +474,19 @@ void mpv_detach_destroy(mpv_handle *ctx);
  * Since mpv_detach_destroy() is called somewhere on the way, it's not safe to
  * call other functions concurrently on the same context.
  *
- * If this is called on a mpv_handle that was not created with mpv_create(),
- * this function will merely send a quit command and then call
- * mpv_detach_destroy(), without waiting for the actual shutdown.
+ * Since mpv client API version 1.29:
+ *  The first call on any mpv_handle will block until the core is destroyed.
+ *  This means it will wait until other mpv_handle have been destroyed. If you
+ *  want asynchronous destruction, just run the "quit" command, and then react
+ *  to the MPV_EVENT_SHUTDOWN event.
+ *  If another mpv_handle already called mpv_terminate_destroy(), this call will
+ *  not actually block. It will destroy the mpv_handle, and exit immediately,
+ *  while other mpv_handles might still be uninitializing.
+ *
+ * Before mpv client API version 1.29:
+ *  If this is called on a mpv_handle that was not created with mpv_create(),
+ *  this function will merely send a quit command and then call
+ *  mpv_detach_destroy(), without waiting for the actual shutdown.
  */
 void mpv_terminate_destroy(mpv_handle *ctx);
 
diff --git a/player/client.c b/player/client.c
index 4a7307194a..a887a16d2a 100644
--- a/player/client.c
+++ b/player/client.c
@@ -69,6 +69,8 @@ struct mp_client_api {
     int num_clients;
     uint64_t event_masks; // combined events of all clients, or 0 if unknown
     bool shutting_down; // do not allow new clients
+    bool have_terminator; // a client took over the role of destroying the core
+    bool terminate_core_thread; // make libmpv core thread exit
 
     struct mp_custom_protocol *custom_protocols;
     int num_custom_protocols;
@@ -95,7 +97,6 @@ struct observe_property {
 struct mpv_handle {
     // -- immmutable
     char name[MAX_CLIENT_NAME];
-    bool owner;
     struct mp_log *log;
     struct MPContext *mpctx;
     struct mp_client_api *clients;
@@ -172,14 +173,6 @@ void mp_clients_destroy(struct MPContext *mpctx)
     mpctx->clients = NULL;
 }
 
-int mp_clients_num(struct MPContext *mpctx)
-{
-    pthread_mutex_lock(&mpctx->clients->lock);
-    int num_clients = mpctx->clients->num_clients;
-    pthread_mutex_unlock(&mpctx->clients->lock);
-    return num_clients;
-}
-
 // Test for "fuzzy" initialization of all clients. That is, all clients have
 // at least called mpv_wait_event() at least once since creation (or exited).
 bool mp_clients_all_initialized(struct MPContext *mpctx)
@@ -221,13 +214,6 @@ bool mp_client_exists(struct MPContext *mpctx, const char *client_name)
     return r;
 }
 
-void mp_client_enter_shutdown(struct MPContext *mpctx)
-{
-    pthread_mutex_lock(&mpctx->clients->lock);
-    mpctx->clients->shutting_down = true;
-    pthread_mutex_unlock(&mpctx->clients->lock);
-}
-
 struct mpv_handle *mp_new_client(struct mp_client_api *clients, const char *name)
 {
     pthread_mutex_lock(&clients->lock);
@@ -272,6 +258,9 @@ struct mpv_handle *mp_new_client(struct mp_client_api *clients, const char *name
 
     MP_TARRAY_APPEND(clients, clients->clients, clients->num_clients, client);
 
+    if (clients->num_clients == 1 && !clients->mpctx->is_cli)
+        client->fuzzy_initialized = true;
+
     clients->event_masks = 0;
     pthread_mutex_unlock(&clients->lock);
 
@@ -373,24 +362,34 @@ void mpv_wait_async_requests(mpv_handle *ctx)
     pthread_mutex_unlock(&ctx->lock);
 }
 
-void mpv_detach_destroy(mpv_handle *ctx)
+static void get_thread(void *ptr)
+{
+    *(pthread_t *)ptr = pthread_self();
+}
+
+static void mp_destroy_client(mpv_handle *ctx, bool terminate)
 {
     if (!ctx)
         return;
 
+    struct MPContext *mpctx = ctx->mpctx;
+    struct mp_client_api *clients = ctx->clients;
+
     MP_VERBOSE(ctx, "Exiting...\n");
 
+    if (terminate)
+        mpv_command(ctx, (const char*[]){"quit", NULL});
+
     // reserved_events equals the number of asynchronous requests that weren't
     // yet replied. In order to avoid that trying to reply to a removed client
     // causes a crash, block until all asynchronous requests were served.
     mpv_wait_async_requests(ctx);
 
-    osd_set_external(ctx->mpctx->osd, ctx, 0, 0, NULL);
-    mp_input_remove_sections_by_owner(ctx->mpctx->input, ctx->name);
-
-    struct mp_client_api *clients = ctx->clients;
+    osd_set_external(mpctx->osd, ctx, 0, 0, NULL);
+    mp_input_remove_sections_by_owner(mpctx->input, ctx->name);
 
     pthread_mutex_lock(&clients->lock);
+
     for (int n = 0; n < clients->num_clients; n++) {
         if (clients->clients[n] == ctx) {
             MP_TARRAY_REMOVE_AT(clients->clients, clients->num_clients, n);
@@ -409,76 +408,98 @@ void mpv_detach_destroy(mpv_handle *ctx)
             }
             talloc_free(ctx);
             ctx = NULL;
-            // shutdown_clients() sleeps to avoid wasting CPU.
-            // mp_hook_test_completion() also relies on this a bit.
-            mp_wakeup_core(clients->mpctx);
             break;
         }
     }
-    pthread_mutex_unlock(&clients->lock);
     assert(!ctx);
+
+    if (mpctx->is_cli) {
+        terminate = false;
+    } else {
+        // If the last mpv_handle got destroyed, destroy the core.
+        if (clients->num_clients == 0)
+            terminate = true;
+
+        // Reserve the right to destroy mpctx for us.
+        if (clients->have_terminator)
+            terminate = false;
+        clients->have_terminator |= terminate;
+    }
+
+    // mp_shutdown_clients() sleeps to avoid wasting CPU.
+    // mp_hook_test_completion() also relies on this a bit.
+    mp_wakeup_core(mpctx);
+
+    pthread_mutex_unlock(&clients->lock);
+
+    // Note that even if num_clients==0, having set have_terminator keeps mpctx
+    // and the core thread alive.
+    if (terminate) {
+        // Make sure the core stops playing files etc. Being able to lock the
+        // dispatch queue requires that the core thread is still active.
+        mp_dispatch_lock(mpctx->dispatch);
+        mpctx->stop_play = PT_QUIT;
+        mp_dispatch_unlock(mpctx->dispatch);
+
+        // Stop the core thread.
+        pthread_mutex_lock(&clients->lock);
+        clients->terminate_core_thread = true;
+        pthread_mutex_unlock(&clients->lock);
+        mp_wakeup_core(mpctx);
+
+        // Blocking wait for all clients and core thread to terminate.
+        pthread_t playthread;
+        mp_dispatch_run(mpctx->dispatch, get_thread, &playthread);
+
+        pthread_join(playthread, NULL);
+
+        mp_destroy(mpctx);
+    }
 }
 
-static void get_thread(void *ptr)
+void mpv_detach_destroy(mpv_handle *ctx)
 {
-    *(pthread_t *)ptr = pthread_self();
+    mp_destroy_client(ctx, false);
 }
 
 void mpv_terminate_destroy(mpv_handle *ctx)
 {
-    if (!ctx)
-        return;
+    mp_destroy_client(ctx, true);
+}
 
-    if (ctx->mpctx->initialized) {
-        mpv_command(ctx, (const char*[]){"quit", NULL});
-    } else {
-        mp_dispatch_lock(ctx->mpctx->dispatch);
-        ctx->mpctx->stop_play = PT_QUIT;
-        mp_dispatch_unlock(ctx->mpctx->dispatch);
-    }
+static bool can_terminate(struct MPContext *mpctx)
+{
+    struct mp_client_api *clients = mpctx->clients;
 
-    if (!ctx->owner) {
-        mpv_detach_destroy(ctx);
-        return;
-    }
+    pthread_mutex_lock(&clients->lock);
+    bool ok = clients->num_clients == 0 && mpctx->outstanding_async == 0 &&
+              (mpctx->is_cli || clients->terminate_core_thread);
+    pthread_mutex_unlock(&clients->lock);
 
-    mp_dispatch_lock(ctx->mpctx->dispatch);
-    assert(ctx->mpctx->autodetach);
-    ctx->mpctx->autodetach = false;
-    mp_dispatch_unlock(ctx->mpctx->dispatch);
+    return ok;
+}
 
-    pthread_t playthread;
-    mp_dispatch_run(ctx->mpctx->dispatch, get_thread, &playthread);
+// Can be called on the core thread only. Idempotent.
+void mp_shutdown_clients(struct MPContext *mpctx)
+{
+    struct mp_client_api *clients = mpctx->clients;
 
-    mpv_detach_destroy(ctx);
+    // Prevent that new clients can appear.
+    pthread_mutex_lock(&clients->lock);
+    clients->shutting_down = true;
+    pthread_mutex_unlock(&clients->lock);
 
-    // And this is also the reason why we only allow 1 thread (the owner) to
-    // call this function.
-    pthread_join(playthread, NULL);
+    while (!can_terminate(mpctx)) {
+        mp_client_broadcast_event(mpctx, MPV_EVENT_SHUTDOWN, NULL);
+        mp_wait_events(mpctx);
+    }
 }
 
-static void *core_thread(void *tag)
+static void *core_thread(void *p)
 {
-    mpthread_set_name("mpv core");
+    struct MPContext *mpctx = p;
 
-    mpv_handle *ctx = NULL;
-    struct MPContext *mpctx = mp_create();
-    if (mpctx) {
-        mpctx->autodetach = true;
-        ctx = mp_new_client(mpctx->clients, "main");
-        if (ctx) {
-            ctx->owner = true;
-            ctx->fuzzy_initialized = true;
-            m_config_set_profile(mpctx->mconfig, "libmpv", 0);
-        } else {
-            mp_destroy(mpctx);
-        }
-    }
-
-    // Let mpv_create() return, and pass it the handle.
-    mp_rendezvous(tag, (intptr_t)(void *)ctx);
-    if (!ctx)
-        return NULL;
+    mpthread_set_name("mpv core");
 
     while (!mpctx->initialized && mpctx->stop_play != PT_QUIT)
         mp_idle(mpctx);
@@ -487,24 +508,36 @@ static void *core_thread(void *tag)
         mp_play_files(mpctx);
 
     // This actually waits until all clients are gone before actually
-    // destroying mpctx.
-    mp_destroy(mpctx);
+    // destroying mpctx. Actual destruction is done by whatever destroys
+    // the last mpv_handle.
+    mp_shutdown_clients(mpctx);
 
     return NULL;
 }
 
 mpv_handle *mpv_create(void)
 {
-    char tag;
-    pthread_t thread;
-    if (pthread_create(&thread, NULL, core_thread, &tag) != 0)
+    struct MPContext *mpctx = mp_create();
+    if (!mpctx)
         return NULL;
 
-    mpv_handle *res = (void *)mp_rendezvous(&tag, 0);
-    if (!res)
-        pthread_join(thread, NULL);
+    m_config_set_profile(mpctx->mconfig, "libmpv", 0);
 
-    return res;
+    mpv_handle *ctx = mp_new_client(mpctx->clients, "main");
+    if (!ctx) {
+        mp_destroy(mpctx);
+        return NULL;
+    }
+
+    pthread_t thread;
+    if (pthread_create(&thread, NULL, core_thread, mpctx) != 0) {
+        ctx->clients->have_terminator = true; // avoid blocking
+        mpv_terminate_destroy(ctx);
+        mp_destroy(mpctx);
+        return NULL;
+    }
+
+    return ctx;
 }
 
 mpv_handle *mpv_create_client(mpv_handle *ctx, const char *name)
@@ -517,19 +550,12 @@ mpv_handle *mpv_create_client(mpv_handle *ctx, const char *name)
     return new;
 }
 
-static void doinit(void *ctx)
-{
-    void **args = ctx;
-
-    *(int *)args[1] = mp_initialize(args[0], NULL);
-}
-
 int mpv_initialize(mpv_handle *ctx)
 {
-    int res = 0;
-    void *args[2] = {ctx->mpctx, &res};
-    mp_dispatch_run(ctx->mpctx->dispatch, doinit, args);
-    return res == 0 ? 0 : MPV_ERROR_INVALID_PARAMETER;
+    lock_core(ctx);
+    int res = mp_initialize(ctx->mpctx, NULL) ? MPV_ERROR_INVALID_PARAMETER : 0;
+    unlock_core(ctx);
+    return res;
 }
 
 // set ev->data to a new copy of the original data
diff --git a/player/client.h b/player/client.h
index 118f6800f6..deec3c793b 100644
--- a/player/client.h
+++ b/player/client.h
@@ -18,9 +18,8 @@ struct mpv_global;
 #define MAX_CLIENT_NAME 64
 
 void mp_clients_init(struct MPContext *mpctx);
-void mp_client_enter_shutdown(struct MPContext *mpctx);
 void mp_clients_destroy(struct MPContext *mpctx);
-int mp_clients_num(struct MPContext *mpctx);
+void mp_shutdown_clients(struct MPContext *mpctx);
 bool mp_clients_all_initialized(struct MPContext *mpctx);
 
 bool mp_client_exists(struct MPContext *mpctx, const char *client_name);
diff --git a/player/core.h b/player/core.h
index 25c006f2e4..0eca484dc2 100644
--- a/player/core.h
+++ b/player/core.h
@@ -225,7 +225,7 @@ enum playback_status {
 
 typedef struct MPContext {
     bool initialized;
-    bool autodetach;
+    bool is_cli;
     struct mpv_global *global;
     struct MPOpts *opts;
     struct mp_log *log;
diff --git a/player/main.c b/player/main.c
index bb83accb7e..bb80591220 100644
--- a/player/main.c
+++ b/player/main.c
@@ -154,18 +154,9 @@ 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) || mpctx->outstanding_async) {
-        mp_client_broadcast_event(mpctx, MPV_EVENT_SHUTDOWN, NULL);
-        mp_wait_events(mpctx);
-    }
-}
-
 void mp_destroy(struct MPContext *mpctx)
 {
-    shutdown_clients(mpctx);
+    mp_shutdown_clients(mpctx);
 
     mp_uninit_ipc(mpctx->ipc_ctx);
     mpctx->ipc_ctx = NULL;
@@ -199,9 +190,6 @@ void mp_destroy(struct MPContext *mpctx)
 
     uninit_libav(mpctx->global);
 
-    if (mpctx->autodetach)
-        pthread_detach(pthread_self());
-
     mp_msg_uninit(mpctx->global);
     pthread_mutex_destroy(&mpctx->lock);
     talloc_free(mpctx);
@@ -461,6 +449,8 @@ int mpv_main(int argc, char *argv[])
     if (!mpctx)
         return 1;
 
+    mpctx->is_cli = true;
+
     char **options = argv && argv[0] ? argv + 1 : NULL; // skips program name
     int r = mp_initialize(mpctx, options);
     if (r == 0)