summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwm4 <wm4@nowhere>2019-09-20 16:31:53 +0200
committerwm4 <wm4@nowhere>2019-09-20 16:31:53 +0200
commitf00af71d12a7d155d4d80f0879931c3f54c7ab9b (patch)
tree6d15f1d352972d77cfbb39bb29d5b7f3ecec4aa3
parent3ae728532dbd8f4baa77a1e8ad74066719613e92 (diff)
downloadmpv-f00af71d12a7d155d4d80f0879931c3f54c7ab9b.tar.bz2
mpv-f00af71d12a7d155d4d80f0879931c3f54c7ab9b.tar.xz
vo_libmpv: fix some more uninit issues
This is mostly for the case when mpv_render_context_free() is called while video is going on. This is supposed to gracefully stop video and deinitialize everything properly. (I feel like it would put too much on the API user to require that video is stopped before calling this function. Whether video is running or not is a fairly highlevel thing, and the API user could not do it in a race-free way.) One problem was that unit() accessed ctx after ctx->in_use was set to false. The update(ctx) call was basically a racy use-after-free. It needed that call to wake up the mpv_render_context_free() loop that waited for VO uninit. Fix this by triggering the wakeup inside the lock, and then doing "barrier" locking in mpv_render_context_free(). Another problem was that the wait loop didn't really wait properly. IT seems the had_kill_update field was a botched attempt to do that. It's indeed quite hairy to do that with update(). Instead make use of the dispatch queue (infinite timeout, using mp_dispatch_interrupt()), which handles the problem of having to wait both for dispatch queue updates and VO uninit at the same time.
-rw-r--r--video/out/vo_libmpv.c59
1 files changed, 35 insertions, 24 deletions
diff --git a/video/out/vo_libmpv.c b/video/out/vo_libmpv.c
index 29af2217b4..3f3127cb2a 100644
--- a/video/out/vo_libmpv.c
+++ b/video/out/vo_libmpv.c
@@ -74,7 +74,6 @@ struct mpv_render_context {
// --- Protected by update_lock
mpv_render_update_fn update_cb;
void *update_cb_ctx;
- bool had_kill_update; // update during termination
pthread_mutex_t lock;
pthread_cond_t video_wait; // paired with lock
@@ -117,8 +116,6 @@ static void update(struct mpv_render_context *ctx)
if (ctx->update_cb)
ctx->update_cb(ctx->update_cb_ctx);
- // For the termination code.
- ctx->had_kill_update = true;
pthread_cond_broadcast(&ctx->update_cond);
pthread_mutex_unlock(&ctx->update_lock);
}
@@ -254,30 +251,42 @@ void mpv_render_context_free(mpv_render_context *ctx)
// a VO could still hold a reference.
mp_set_main_render_context(ctx->client_api, ctx, false);
- // If it's still in use, a VO using it must be active. Destroy the VO, and
- // also bring down the decoder etc., which still might be using the hwdec
- // context. The above removal guarantees it can't come back (so ctx->vo
- // can't change to non-NULL).
if (atomic_load(&ctx->in_use)) {
+ // Start destroy the VO, and also bring down the decoder etc., which
+ // still might be using the hwdec context or use DR images. The above
+ // mp_set_main_render_context() call guarantees it can't come back (so
+ // ctx->vo can't change to non-NULL).
+ // In theory, this races with vo_libmpv exiting and another VO being
+ // used, which is a harmless grotesque corner case.
kill_video_async(ctx->client_api);
while (atomic_load(&ctx->in_use)) {
- // As long as the video decoders are not destroyed, they can still
- // try to allocate new DR images and so on. This is a grotesque
- // corner case, but possible. Also, more likely, DR images need to
- // be released while the video chain is destroyed.
- if (ctx->dispatch)
- mp_dispatch_queue_process(ctx->dispatch, 0);
-
- // Wait for update() calls.
- pthread_mutex_lock(&ctx->update_lock);
- if (!ctx->had_kill_update)
- pthread_cond_wait(&ctx->update_cond, &ctx->update_lock);
- ctx->had_kill_update = false;
- pthread_mutex_unlock(&ctx->update_lock);
+ // As a nasty detail, we need to wait until the VO is released, but
+ // also need to react to update() calls during it (the update calls
+ // are supposed to trigger processing ctx->dispatch). We solve this
+ // by making the VO uninit function call mp_dispatch_interrupt().
+ //
+ // Other than that, processing ctx->dispatch is needed to serve the
+ // video decoder, which might still not be fully destroyed, and e.g.
+ // performs calls to release DR images (or, as a grotesque corner
+ // case may even try to allocate new ones).
+ //
+ // Once the VO is released, ctx->dispatch becomes truly inactive.
+ // (The libmpv API user could call mpv_render_context_update() while
+ // mpv_render_context_free() is being called, but of course this is
+ // invalid.)
+ mp_dispatch_queue_process(ctx->dispatch, INFINITY);
}
}
+ pthread_mutex_lock(&ctx->lock);
+ // Barrier - guarantee uninit() has left the lock region. It will access ctx
+ // until the lock has been released, so we must not proceed with destruction
+ // before we can acquire the lock. (The opposite, uninit() acquiring the
+ // lock, can not happen anymore at this point - we've waited for VO uninit,
+ // and prevented that new VOs can be created.)
+ pthread_mutex_unlock(&ctx->lock);
+
assert(!atomic_load(&ctx->in_use));
assert(!ctx->vo);
@@ -679,12 +688,14 @@ static void uninit(struct vo *vo)
ctx->need_update_external = true;
ctx->need_reset = true;
ctx->vo = NULL;
- pthread_mutex_unlock(&ctx->lock);
- bool state = atomic_exchange(&ctx->in_use, false);
- assert(state); // obviously must have been set
+ // The following do not normally need ctx->lock, however, ctx itself may
+ // become invalid once we release ctx->lock.
+ bool prev_in_use = atomic_exchange(&ctx->in_use, false);
+ assert(prev_in_use); // obviously must have been set
+ mp_dispatch_interrupt(ctx->dispatch);
- update(ctx);
+ pthread_mutex_unlock(&ctx->lock);
}
static int preinit(struct vo *vo)