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, 19 insertions, 2 deletions

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