vo_opengl_cb: simplify API uninitialization

Until now, calling mpv_opengl_cb_uninit_gl() at a "bad moment" could
make the whole thing to explode. The API user was asked to avoid such
situations by calling it only in "good moments". But this was probably a
bit too subtle and could easily be overlooked.

Integrate the approach the qml example uses directly into the
implementation. If the OpenGL context is to be unitialized, forcefully
disable video, and block until this is done.

5 files changed, 59 insertions, 21 deletions

diff --git a/DOCS/client_api_examples/qml/mpvrenderer.cpp b/DOCS/client_api_examples/qml/mpvrenderer.cpp
index 13c1f0a747..6d214dfba8 100644
--- a/DOCS/client_api_examples/qml/mpvrenderer.cpp
+++ b/DOCS/client_api_examples/qml/mpvrenderer.cpp
@@ -32,15 +32,9 @@ public:
 
     virtual ~MpvRenderer()
     {
-        // Before we can really destroy the OpenGL state, we must make sure
-        // that the video output is destroyed. This is important for some
-        // forms of hardware decoding, where the decoder shares some state
-        // with the video output and the OpenGL context.
-        // Deselecting the video track is the easiest way to achieve this in
-        // a synchronous way. If no file is playing, setting the property
-        // will fail and do nothing.
-        mpv::qt::set_property_variant(mpv, "vid", "no");
-
+        // Until this call is done, we need to make sure the player remains
+        // alive. This is done implicitly with the mpv::qt::Handle instance
+        // in this class.
         mpv_opengl_cb_uninit_gl(mpv_gl);
     }
 
diff --git a/libmpv/opengl_cb.h b/libmpv/opengl_cb.h
index 3f5010b26e..8f7e1df76d 100644
--- a/libmpv/opengl_cb.h
+++ b/libmpv/opengl_cb.h
@@ -96,6 +96,17 @@ extern "C" {
  *    as used with mpv_opengl_cb_init_gl()
  *  - never can be called from within the callbacks set with
  *    mpv_set_wakeup_callback() or mpv_opengl_cb_set_update_callback()
+ *
+ * Context and handle lifecycle
+ * ----------------------------
+ *
+ * Video initialization will fail if the OpenGL context was not initialized yet
+ * (with mpv_opengl_cb_init_gl()). Likewise, mpv_opengl_cb_uninit_gl() will
+ * disable video.
+ *
+ * When the mpv core is destroyed (e.g. via mpv_terminate_destroy()), the OpenGL
+ * context must have been uninitialized. If this doesn't happen, undefined
+ * behavior will result.
  */
 
 /**
@@ -181,10 +192,8 @@ int mpv_opengl_cb_render(mpv_opengl_cb_context *ctx, int fbo, int vp[4]);
 /**
  * Destroy the mpv OpenGL state.
  *
- * This will trigger undefined behavior (i.e. crash hard) if the hardware
- * decoder is still active, because the OpenGL hardware decoding interop state
- * can't be destroyed synchronously. If no hardware decoding is active, the
- * state can be destroyed at any time.
+ * If video is still active (e.g. a file playing), video will be disabled
+ * forcefully.
  *
  * Calling this multiple times is ok.
  *
diff --git a/player/client.c b/player/client.c
index e83f3568f8..7d75b22256 100644
--- a/player/client.c
+++ b/player/client.c
@@ -1613,6 +1613,15 @@ int64_t mpv_get_time_us(mpv_handle *ctx)
     return mp_time_us();
 }
 
+// Used by vo_opengl_cb to synchronously uninitialize video.
+void kill_video(struct mp_client_api *client_api)
+{
+    struct MPContext *mpctx = client_api->mpctx;
+    mp_dispatch_lock(mpctx->dispatch);
+    mp_switch_track(mpctx, STREAM_VIDEO, NULL);
+    mp_dispatch_unlock(mpctx->dispatch);
+}
+
 #include "libmpv/opengl_cb.h"
 
 #if HAVE_GL
@@ -1620,7 +1629,7 @@ static mpv_opengl_cb_context *opengl_cb_get_context(mpv_handle *ctx)
 {
     mpv_opengl_cb_context *cb = ctx->mpctx->gl_cb_ctx;
     if (!cb) {
-        cb = mp_opengl_create(ctx->mpctx->global, ctx->mpctx->osd);
+        cb = mp_opengl_create(ctx->mpctx->global, ctx->mpctx->osd, ctx->clients);
         ctx->mpctx->gl_cb_ctx = cb;
     }
     return cb;
diff --git a/player/client.h b/player/client.h
index aeb2e886e7..656e3601cb 100644
--- a/player/client.h
+++ b/player/client.h
@@ -42,6 +42,8 @@ struct mpv_opengl_cb_context;
 struct mpv_global;
 struct osd_state;
 struct mpv_opengl_cb_context *mp_opengl_create(struct mpv_global *g,
-                                               struct osd_state *osd);
+                                               struct osd_state *osd,
+                                               struct mp_client_api *client_api);
+void kill_video(struct mp_client_api *client_api);
 
 #endif
diff --git a/video/out/vo_opengl_cb.c b/video/out/vo_opengl_cb.c
index d2d4221e94..991a90f6cf 100644
--- a/video/out/vo_opengl_cb.c
+++ b/video/out/vo_opengl_cb.c
@@ -50,10 +50,12 @@ struct vo_priv {
 
 struct mpv_opengl_cb_context {
     struct mp_log *log;
+    struct mp_client_api *client_api;
 
     pthread_mutex_t lock;
 
     // --- Protected by lock
+    bool initialized;
     mpv_opengl_cb_update_fn update_cb;
     void *update_cb_ctx;
     struct mp_image *waiting_frame;
@@ -72,11 +74,11 @@ struct mpv_opengl_cb_context {
     GL *gl;
     struct gl_video *renderer;
     struct gl_hwdec *hwdec;
+    struct mp_hwdec_info hwdec_info; // it's also semi-immutable after init
 
     // --- Immutable or semi-threadsafe.
 
     struct osd_state *osd;
-    struct mp_hwdec_info hwdec_info;
     const char *hwapi;
 
     struct vo *active;
@@ -86,20 +88,26 @@ static void free_ctx(void *ptr)
 {
     mpv_opengl_cb_context *ctx = ptr;
 
+    // This can trigger if the client API user doesn't call
+    // mpv_opengl_cb_uninit_gl() properly.
+    assert(!ctx->initialized);
+
     pthread_mutex_destroy(&ctx->lock);
 }
 
 struct mpv_opengl_cb_context *mp_opengl_create(struct mpv_global *g,
-                                               struct osd_state *osd)
+                                               struct osd_state *osd,
+                                               struct mp_client_api *client_api)
 {
     mpv_opengl_cb_context *ctx = talloc_zero(NULL, mpv_opengl_cb_context);
     talloc_set_destructor(ctx, free_ctx);
-    ctx->log = mp_log_new(ctx, g->log, "opengl-cb");
     pthread_mutex_init(&ctx->lock, NULL);
 
     ctx->gl = talloc_zero(ctx, GL);
 
+    ctx->log = mp_log_new(ctx, g->log, "opengl-cb");
     ctx->osd = osd;
+    ctx->client_api = client_api;
 
     switch (g->opts->hwdec_api) {
     case HWDEC_AUTO:    ctx->hwapi = "auto"; break;
@@ -149,6 +157,7 @@ int mpv_opengl_cb_init_gl(struct mpv_opengl_cb_context *ctx, const char *exts,
     ctx->renderer = gl_video_init(ctx->gl, ctx->log, ctx->osd);
     if (!ctx->renderer)
         return MPV_ERROR_UNSUPPORTED;
+
     ctx->hwdec = gl_hwdec_load_api(ctx->log, ctx->gl, ctx->hwapi, &ctx->hwdec_info);
     gl_video_set_hwdec(ctx->renderer, ctx->hwdec);
 
@@ -157,6 +166,7 @@ int mpv_opengl_cb_init_gl(struct mpv_opengl_cb_context *ctx, const char *exts,
         ctx->imgfmt_supported[n - IMGFMT_START] =
             gl_video_check_format(ctx->renderer, n);
     }
+    ctx->initialized = true;
     pthread_mutex_unlock(&ctx->lock);
 
     gl_video_unset_gl_state(ctx->renderer);
@@ -165,6 +175,18 @@ int mpv_opengl_cb_init_gl(struct mpv_opengl_cb_context *ctx, const char *exts,
 
 int mpv_opengl_cb_uninit_gl(struct mpv_opengl_cb_context *ctx)
 {
+    // Bring down the decoder etc., which still might be using the hwdec
+    // context. Setting initialized=false guarantees it can't come back.
+    pthread_mutex_lock(&ctx->lock);
+    ctx->initialized = false;
+    pthread_mutex_unlock(&ctx->lock);
+
+    kill_video(ctx->client_api);
+
+    pthread_mutex_lock(&ctx->lock);
+    assert(!ctx->active);
+    pthread_mutex_unlock(&ctx->lock);
+
     gl_video_uninit(ctx->renderer);
     ctx->renderer = NULL;
     gl_hwdec_uninit(ctx->hwdec);
@@ -298,9 +320,6 @@ static int control(struct vo *vo, uint32_t request, void *data)
         pthread_mutex_unlock(&p->ctx->lock);
         return VO_TRUE;
     case VOCTRL_GET_HWDEC_INFO: {
-        // Warning: in theory, the API user could destroy the OpenGL context
-        //          while the decoder uses the hwdec thing, and bad things would
-        //          happen. Currently, the API user is told not to do this.
         struct mp_hwdec_info **arg = data;
         *arg = p->ctx ? &p->ctx->hwdec_info : NULL;
         return true;
@@ -334,6 +353,11 @@ static int preinit(struct vo *vo)
     }
 
     pthread_mutex_lock(&p->ctx->lock);
+    if (!p->ctx->initialized) {
+        MP_FATAL(vo, "OpenGL context not initialized.\n");
+        pthread_mutex_unlock(&p->ctx->lock);
+        return -1;
+    }
     p->ctx->active = vo;
     p->ctx->reconfigured = true;
     assert(vo->osd == p->ctx->osd);