client API: fix potential deadlock problems by throwing more shit at it

The render API (vo_libmpv) had potential deadlock problems with
MPV_RENDER_PARAM_ADVANCED_CONTROL. This required vd-lavc-dr to be
enabled (the default). I never observed these deadlocks in the wild
(doesn't mean they didn't happen), although I could specifically provoke
them with some code changes.

The problem was mostly about DR (direct rendering, letting the video
decoder write to OpenGL buffer memory). Allocating/freeing a DR image
needs to be done on the OpenGL thread, even though _lots_ of threads are
involved with handling images. Freeing a DR image is a special case that
can happen any time. dr_helper.c does most of the evil magic of
achieving this. Unfortunately, there was a (sort of) circular lock
dependency: freeing an image while certain internal locks are held would
trigger the user's context update callback, which in turn would call
mpv_render_context_update(), which processed all pending free requests,
and then acquire an internal lock - which the caller might not release
until a further DR image could be freed.

"Solve" this by making freeing DR images asynchronous. This is slightly
risky, but actually not much. The DR images will be free'd eventually.
The biggest disadvantage is probably that debugging might get trickier.

Any solution to this problem will probably add images to free to some
sort of queue, and then process it later. I considered making this more
explicit (so there'd be a point where the caller forcibly waits for all
queued items to be free'd), but discarded these ideas as this probably
would only increase complexity.

Another consequence is that freeing DR images on the GL thread is not
synchronous anymore. Instead, it mpv_render_context_update() will do it
with a delay. This seems roundabout, but doesn't actually change
anything, and avoids additional code.

This also fixes that the render API required the render API user to
remain on the same thread, even though this wasn't documented. As such,
it was a bug. OpenGL essentially forces you to do all GL usage on a
single thread, but in theory the API user could for example move the GL
context to another thread.

The API bump is because I think you can't make enough noise about this.
Since we don't backport fixes to old versions, I'm specifically stating
that old versions are broken, and I'm supplying workarounds.

Internally, dr_helper_create() does not use pthread_self() anymore, thus
the vo.c change. I think it's better to make binding to the current
thread as explicit as possible.

Of course it's not sure that this fixes all deadlocks (probably not).

1 files changed, 8 insertions, 15 deletions

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);