4 files changed, 63 insertions, 21 deletions

diff --git a/video/out/dr_helper.c b/video/out/dr_helper.c
index e826d08dc4..78d4633efb 100644
--- a/video/out/dr_helper.c
+++ b/video/out/dr_helper.c
@@ -12,7 +12,10 @@
 #include "dr_helper.h"
 
 struct dr_helper {
+    pthread_mutex_t thread_lock;
     pthread_t thread;
+    bool thread_valid; // (POSIX defines no "unset" pthread_t value yet)
+
     struct mp_dispatch_queue *dispatch;
     atomic_ullong dr_in_flight;
 
@@ -28,6 +31,8 @@ static void dr_helper_destroy(void *ptr)
     // All references must have been freed on destruction, or we'll have
     // dangling pointers.
     assert(atomic_load(&dr->dr_in_flight) == 0);
+
+    pthread_mutex_destroy(&dr->thread_lock);
 }
 
 struct dr_helper *dr_helper_create(struct mp_dispatch_queue *dispatch,
@@ -38,15 +43,34 @@ struct dr_helper *dr_helper_create(struct mp_dispatch_queue *dispatch,
     struct dr_helper *dr = talloc_ptrtype(NULL, dr);
     talloc_set_destructor(dr, dr_helper_destroy);
     *dr = (struct dr_helper){
-        .thread = pthread_self(),
         .dispatch = dispatch,
         .dr_in_flight = ATOMIC_VAR_INIT(0),
         .get_image = get_image,
         .get_image_ctx = get_image_ctx,
     };
+    pthread_mutex_init(&dr->thread_lock, NULL);
     return dr;
 }
 
+void dr_helper_acquire_thread(struct dr_helper *dr)
+{
+    pthread_mutex_lock(&dr->thread_lock);
+    assert(!dr->thread_valid); // fails on API user errors
+    dr->thread_valid = true;
+    dr->thread = pthread_self();
+    pthread_mutex_unlock(&dr->thread_lock);
+}
+
+void dr_helper_release_thread(struct dr_helper *dr)
+{
+    pthread_mutex_lock(&dr->thread_lock);
+    // Fails on API user errors.
+    assert(dr->thread_valid);
+    assert(pthread_equal(dr->thread, pthread_self()));
+    dr->thread_valid = false;
+    pthread_mutex_unlock(&dr->thread_lock);
+}
+
 struct free_dr_context {
     struct dr_helper *dr;
     AVBufferRef *ref;
@@ -66,13 +90,19 @@ static void dr_thread_free(void *ptr)
 static void free_dr_buffer_on_dr_thread(void *opaque, uint8_t *data)
 {
     struct free_dr_context *ctx = opaque;
+    struct dr_helper *dr = ctx->dr;
+
+    pthread_mutex_lock(&dr->thread_lock);
+    bool on_this_thread =
+        dr->thread_valid && pthread_equal(ctx->dr->thread, pthread_self());
+    pthread_mutex_unlock(&dr->thread_lock);
 
     // The image could be unreffed even on the DR thread. In practice, this
     // matters most on DR destruction.
-    if (pthread_equal(ctx->dr->thread, pthread_self())) {
+    if (on_this_thread) {
         dr_thread_free(ctx);
     } else {
-        mp_dispatch_run(ctx->dr->dispatch, dr_thread_free, ctx);
+        mp_dispatch_enqueue(dr->dispatch, dr_thread_free, ctx);
     }
 }
 
diff --git a/video/out/dr_helper.h b/video/out/dr_helper.h
index cf37c570e2..ff1d268426 100644
--- a/video/out/dr_helper.h
+++ b/video/out/dr_helper.h
@@ -3,18 +3,35 @@
 // This is a helper for implementing thread-safety for DR callbacks. These need
 // to allocate GPU buffers on the GPU thread (e.g. OpenGL with its forced TLS),
 // and the buffers also need to be freed on the GPU thread.
+// This is not a helpful "Dr.", rather it represents Satan in form of C code.
 struct dr_helper;
 
 struct mp_image;
 struct mp_dispatch_queue;
 
-// This MUST be called on the "target" thread (it will call pthread_self()).
 // dr_helper_get_image() calls will use the dispatch queue to run get_image on
-// the target thread too.
+// a target thread, which processes the dispatch queue.
+// Note: the dispatch queue must process outstanding async. work before the
+//       dr_helper instance can be destroyed.
 struct dr_helper *dr_helper_create(struct mp_dispatch_queue *dispatch,
             struct mp_image *(*get_image)(void *ctx, int imgfmt, int w, int h,
                                           int stride_align),
             void *get_image_ctx);
 
+// Make DR release calls (freeing images) reentrant if they are called on this
+// (pthread_self()) thread. That means any free call will directly release the
+// image as allocated with get_image().
+// Only 1 thread can use this at a time. Note that it would make no sense to
+// call this on more than 1 thread, as get_image is assumed not thread-safe.
+void dr_helper_acquire_thread(struct dr_helper *dr);
+
+// This _must_ be called on the same thread as dr_helper_acquire_thread() was
+// called. Every release call must be paired with an acquire call.
+void dr_helper_release_thread(struct dr_helper *dr);
+
+// Allocate an image by running the get_image callback on the target thread.
+// Always blocks on dispatch queue processing. This implies there is no way to
+// allocate a DR'ed image on the render thread (at least not in a way which
+// actually works if you want foreign threads to be able to free them).
 struct mp_image *dr_helper_get_image(struct dr_helper *dr, int imgfmt,
                                      int w, int h, int stride_align);
diff --git a/video/out/vo.c b/video/out/vo.c
index 7aafe28c64..e5b4198f76 100644
--- a/video/out/vo.c
+++ b/video/out/vo.c
@@ -1046,8 +1046,10 @@ static void *vo_thread(void *ptr)
 
     mpthread_set_name("vo");
 
-    if (vo->driver->get_image)
+    if (vo->driver->get_image) {
         in->dr_helper = dr_helper_create(in->dispatch, get_image_vo, vo);
+        dr_helper_acquire_thread(in->dr_helper);
+    }
 
     int r = vo->driver->preinit(vo) ? -1 : 0;
     mp_rendezvous(vo, r); // init barrier
diff --git a/video/out/vo_libmpv.c b/video/out/vo_libmpv.c
index b6c4d9fb06..fffb65b29a 100644
--- a/video/out/vo_libmpv.c
+++ b/video/out/vo_libmpv.c
@@ -46,21 +46,9 @@
  *  And: render thread > VO (wait for present)
  *       VO > render thread (wait for present done, via timeout)
  *
- *  Inherent locking bugs with advanced_control:
- *
- *      In theory, it can deadlock on ctx->lock. Consider if the VO thread calls
- *      forget_frames(), which it does under ctx->lock (e.g. VOCTRL_RESET).
- *      If a frame was a DR image, dr_helper.c will call mp_dispatch_run().
- *      This in turn will call the wakeup callback set with
- *      mpv_render_context_set_update_callback(). The API user will eventually
- *      call mpv_render_context_update(), which performs the dispatch queue work
- *      queued by dr_helper.c.
- *      And then the function tries to acquire ctx->lock. This can deadlock
- *      under specific circumstances. It will _not_ deadlock if the queued work
- *      made the caller release the lock. However, if the caller made queue
- *      some more work (like freeing a second frame), and will keep the lock
- *      until it gets a reply. Both threads will wait on each other, and no
- *      progress can be made.
+ *  Locking gets more complex with advanced_control enabled. Use
+ *  mpv_render_context.dispatch with care; synchronous calls can add lock
+ *  dependencies.
  */
 
 struct vo_priv {
@@ -306,6 +294,11 @@ void mpv_render_context_free(mpv_render_context *ctx)
     assert(!atomic_load(&ctx->in_use));
     assert(!ctx->vo);
 
+    // With the dispatch queue not being served anymore, allow frame free
+    // requests from this thread to be served directly.
+    if (ctx->dr)
+        dr_helper_acquire_thread(ctx->dr);
+
     // Possibly remaining outstanding work.
     mp_dispatch_queue_process(ctx->dispatch, 0);