diff options
| -rw-r--r-- | DOCS/client-api-changes.rst | 5 | ||||
| -rw-r--r-- | libmpv/client.h | 2 | ||||
| -rw-r--r-- | libmpv/render.h | 22 | ||||
| -rw-r--r-- | video/out/dr_helper.c | 36 | ||||
| -rw-r--r-- | video/out/dr_helper.h | 21 | ||||
| -rw-r--r-- | video/out/vo.c | 4 | ||||
| -rw-r--r-- | video/out/vo_libmpv.c | 23 |
7 files changed, 81 insertions, 32 deletions
diff --git a/DOCS/client-api-changes.rst b/DOCS/client-api-changes.rst index 5383456c55..5b595047d0 100644 --- a/DOCS/client-api-changes.rst +++ b/DOCS/client-api-changes.rst @@ -32,6 +32,11 @@ API changes :: --- mpv 0.30.0 --- + 1.105 - Fix deadlock problems with MPV_RENDER_PARAM_ADVANCED_CONTROL and if + the "vd-lavc-dr" option is enabled (which it is by default). + There were no actual API changes. + API users on older API versions and mpv releases should set + "vd-lavc-dr" to "no" to avoid these issues. 1.104 - Deprecate struct mpv_opengl_drm_params. Replaced by mpv_opengl_drm_params_v2 - Deprecate MPV_RENDER_PARAM_DRM_DISPLAY. Replaced by MPV_RENDER_PARAM_DRM_DISPLAY_V2. 1.103 - redo handling of async commands diff --git a/libmpv/client.h b/libmpv/client.h index 2fea40de13..670f45c626 100644 --- a/libmpv/client.h +++ b/libmpv/client.h @@ -223,7 +223,7 @@ extern "C" { * relational operators (<, >, <=, >=). */ #define MPV_MAKE_VERSION(major, minor) (((major) << 16) | (minor) | 0UL) -#define MPV_CLIENT_API_VERSION MPV_MAKE_VERSION(1, 104) +#define MPV_CLIENT_API_VERSION MPV_MAKE_VERSION(1, 105) /** * The API user is allowed to "#define MPV_ENABLE_DEPRECATED 0" before diff --git a/libmpv/render.h b/libmpv/render.h index 6d594e0c06..afea7c2e10 100644 --- a/libmpv/render.h +++ b/libmpv/render.h @@ -79,16 +79,6 @@ extern "C" { * is logged. If you set MPV_RENDER_PARAM_ADVANCED_CONTROL, you promise that * this won't happen, and must absolutely guarantee it, or a real deadlock * will freeze the mpv core thread forever. - * - if MPV_RENDER_PARAM_ADVANCED_CONTROL is used, you currently must call all - * mpv_render*() API functions from the same thread on which the - * mpv_render_context was returned by mpv_render_context_create(). This - * requirement always existed. Not honoring it will lead to UB (deadlocks, - * use of invalid pthread_t handles). This requirement might be removed in - * the future, but will require some considerable work internal to libmpv. - * You can avoid this issue by setting "vd-lavc-dr" to "no". - * - MPV_RENDER_PARAM_ADVANCED_CONTROL has some other libmpv-internal problems, - * which may result in random deadlocks (see top of vo_libmpv.c). - * You can probably avoid this issue by setting "vd-lavc-dr" to "no". * * libmpv functions which are safe to call from a render thread are: * - functions marked with "Safe to be called from mpv render API threads." @@ -104,6 +94,18 @@ extern "C" { * - if the mpv_handle parameter refers to a different mpv core than the one * you're rendering for (very obscure, but allowed) * + * Note about old libmpv version: + * + * Before API version 1.105 (basically in mpv 0.29.x), simply enabling + * MPV_RENDER_PARAM_ADVANCED_CONTROL could cause deadlock issues. This can + * be worked around by setting the "vd-lavc-dr" option to "no". + * In addition, you were required to call all mpv_render*() API functions + * from the same thread on which mpv_render_context_create() was originally + * run (for the same the mpv_render_context). Not honoring it led to UB + * (deadlocks, use of invalid pthread_t handles), even if you moved your GL + * context to a different thread correctly. + * These problems were addressed in API version 1.105 (mpv 0.30.0). + * * Context and handle lifecycle * ---------------------------- * 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); |