diff options
author | wm4 <wm4@nowhere> | 2014-06-07 23:15:07 +0200 |
---|---|---|
committer | wm4 <wm4@nowhere> | 2014-06-07 23:16:46 +0200 |
commit | 5cc68c792be1d0ae42017f6960ba1d0448646ff5 (patch) | |
tree | e8180a6373667a51855abcc41469d5ac98f27040 | |
parent | fca608ccb95e4167e1885483dfd30ba71007cbb4 (diff) | |
download | mpv-5cc68c792be1d0ae42017f6960ba1d0448646ff5.tar.bz2 mpv-5cc68c792be1d0ae42017f6960ba1d0448646ff5.tar.xz |
client API: restructure waiting, do log msg wakeup properly
Until now, availability of new log messages (through the mechanism
associated with mpv_request_log_messages()) did not wakeup the client
API properly. Commit 3b7402b5 was basically a hack to improve that
somewhat, but it wasn't a solution.
The main problem is that the client API itself is producing messages, so
the message callback would attempt to lock the client API lock,
resulting in a deadlock. Even if the lock was recursive, we'd run into
lock-order issues.
Solve this by using a separate lock for waiting and wakeup. Also, since
it's a natural addition, avoid redundant wakeups. This means the wakeup
callback as well as the wakeup pipe will be triggered only once until
the next mpv_wait_event() call happens.
This might make the wakeup callback be invoked in a reentrant way for
the first time, for example if a mpv_* function prints to a log. Adjust
the docs accordingly. (Note that non-reentrant beheavior was never
guaranteed - basically the wakeup callback is somewhat dangerous and
inconvenient.)
Also remove some traces of unneeded code. ctx->shutdown for one was
never set, and probably a leftover of an abandoned idea.
-rw-r--r-- | libmpv/client.h | 4 | ||||
-rw-r--r-- | player/client.c | 97 |
2 files changed, 58 insertions, 43 deletions
diff --git a/libmpv/client.h b/libmpv/client.h index cc4ff99088..d0d0402da7 100644 --- a/libmpv/client.h +++ b/libmpv/client.h @@ -1175,7 +1175,9 @@ void mpv_wakeup(mpv_handle *ctx); * must not make any assumptions of the environment, and you must return as * soon as possible. You are not allowed to call any client API functions * inside of the callback. In particular, you should not do any processing in - * the callback, but wake up another thread that does all the work. + * the callback, but wake up another thread that does all the work. It's also + * possible that the callback is called from a thread while a mpv API function + * is called (i.e. it can be reentrant). * * In general, the client API expects you to call mpv_wait_event() to receive * notifications, and the wakeup callback is merely a helper utility to make diff --git a/player/client.c b/player/client.c index fcee36a383..2f8a092b4e 100644 --- a/player/client.c +++ b/player/client.c @@ -87,17 +87,21 @@ struct mpv_handle { struct mpv_event_property cur_property_event; pthread_mutex_t lock; + + pthread_mutex_t wakeup_lock; pthread_cond_t wakeup; + // -- protected by wakeup_lock + bool need_wakeup; + void (*wakeup_cb)(void *d); + void *wakeup_cb_ctx; + int wakeup_pipe[2]; + // -- protected by lock uint64_t event_mask; bool queued_wakeup; - bool shutdown; bool choke_warning; - void (*wakeup_cb)(void *d); - void *wakeup_cb_ctx; - int wakeup_pipe[2]; mpv_event *events; // ringbuffer of max_events entries int max_events; // allocated number of entries in events @@ -111,11 +115,9 @@ struct mpv_handle { int properties_updating; struct mp_log_buffer *messages; - int messages_level; }; static bool gen_property_change_event(struct mpv_handle *ctx); -static void recreate_message_buffer(mpv_handle *ctx); void mp_clients_init(struct MPContext *mpctx) { @@ -190,6 +192,7 @@ struct mpv_handle *mp_new_client(struct mp_client_api *clients, const char *name .wakeup_pipe = {-1, -1}, }; pthread_mutex_init(&client->lock, NULL); + pthread_mutex_init(&client->wakeup_lock, NULL); pthread_cond_init(&client->wakeup, NULL); MP_TARRAY_APPEND(clients, clients->clients, clients->num_clients, client); @@ -216,21 +219,39 @@ struct MPContext *mp_client_get_core(struct mpv_handle *ctx) static void wakeup_client(struct mpv_handle *ctx) { - pthread_cond_signal(&ctx->wakeup); - if (ctx->wakeup_cb) - ctx->wakeup_cb(ctx->wakeup_cb_ctx); - if (ctx->wakeup_pipe[0] != -1) - write(ctx->wakeup_pipe[1], &(char){0}, 1); + pthread_mutex_lock(&ctx->wakeup_lock); + if (!ctx->need_wakeup) { + ctx->need_wakeup = true; + pthread_cond_signal(&ctx->wakeup); + if (ctx->wakeup_cb) + ctx->wakeup_cb(ctx->wakeup_cb_ctx); + if (ctx->wakeup_pipe[0] != -1) + write(ctx->wakeup_pipe[1], &(char){0}, 1); + } + pthread_mutex_unlock(&ctx->wakeup_lock); } -void mpv_set_wakeup_callback(mpv_handle *ctx, void (*cb)(void *d), void *d) +// Note: the caller has to deal with sporadic wakeups. +static int wait_wakeup(struct mpv_handle *ctx, int64_t end) { + int r = 0; + pthread_mutex_unlock(&ctx->lock); + pthread_mutex_lock(&ctx->wakeup_lock); + if (!ctx->need_wakeup) + r = mpthread_cond_timedwait(&ctx->wakeup, &ctx->wakeup_lock, end); + if (r == 0) + ctx->need_wakeup = false; + pthread_mutex_unlock(&ctx->wakeup_lock); pthread_mutex_lock(&ctx->lock); + return r; +} + +void mpv_set_wakeup_callback(mpv_handle *ctx, void (*cb)(void *d), void *d) +{ + pthread_mutex_lock(&ctx->wakeup_lock); ctx->wakeup_cb = cb; ctx->wakeup_cb_ctx = d; - // Update the message callback - recreate_message_buffer(ctx); - pthread_mutex_unlock(&ctx->lock); + pthread_mutex_unlock(&ctx->wakeup_lock); } void mpv_suspend(mpv_handle *ctx) @@ -266,7 +287,7 @@ void mpv_detach_destroy(mpv_handle *ctx) // causes a crash, block until all asynchronous requests were served. ctx->event_mask = 0; while (ctx->reserved_events || ctx->properties_updating) - pthread_cond_wait(&ctx->wakeup, &ctx->lock); + wait_wakeup(ctx, INT64_MAX); pthread_mutex_unlock(&ctx->lock); struct mp_client_api *clients = ctx->clients; @@ -282,6 +303,7 @@ void mpv_detach_destroy(mpv_handle *ctx) } mp_msg_log_buffer_destroy(ctx->messages); pthread_cond_destroy(&ctx->wakeup); + pthread_mutex_destroy(&ctx->wakeup_lock); pthread_mutex_destroy(&ctx->lock); if (ctx->wakeup_pipe[0] != -1) { close(ctx->wakeup_pipe[0]); @@ -524,13 +546,15 @@ mpv_event *mpv_wait_event(mpv_handle *ctx, double timeout) { mpv_event *event = ctx->cur_event; + pthread_mutex_lock(&ctx->lock); + if (timeout < 0) timeout = 1e20; + if (ctx->queued_wakeup) + timeout = 0; int64_t deadline = mp_add_timeout(mp_time_us(), timeout); - pthread_mutex_lock(&ctx->lock); - *event = (mpv_event){0}; talloc_free_children(event); @@ -544,10 +568,6 @@ mpv_event *mpv_wait_event(mpv_handle *ctx, double timeout) } if (gen_property_change_event(ctx)) break; - if (ctx->shutdown) { - event->event_id = MPV_EVENT_SHUTDOWN; - break; - } if (ctx->messages) { // Poll the log message queue. Currently we can't/don't do better. struct mp_log_buffer_entry *msg = @@ -565,11 +585,7 @@ mpv_event *mpv_wait_event(mpv_handle *ctx, double timeout) break; } } - if (ctx->queued_wakeup) - break; - if (timeout == 0) - break; - int r = mpthread_cond_timedwait(&ctx->wakeup, &ctx->lock, deadline); + int r = wait_wakeup(ctx, deadline); if (r == ETIMEDOUT) break; } @@ -1275,17 +1291,10 @@ int mpv_load_config_file(mpv_handle *ctx, const char *filename) return 0; } -// called locked -static void recreate_message_buffer(mpv_handle *ctx) +static void msg_wakeup(void *p) { - mp_msg_log_buffer_destroy(ctx->messages); - ctx->messages = NULL; - if (ctx->messages_level >= 0) { - int size = ctx->messages_level >= MSGL_V ? 10000 : 1000; - ctx->messages = - mp_msg_log_buffer_new(ctx->mpctx->global, size, ctx->messages_level, - ctx->wakeup_cb, ctx->wakeup_cb_ctx); - } + mpv_handle *ctx = p; + wakeup_client(ctx); } int mpv_request_log_messages(mpv_handle *ctx, const char *min_level) @@ -1301,19 +1310,23 @@ int mpv_request_log_messages(mpv_handle *ctx, const char *min_level) return MPV_ERROR_INVALID_PARAMETER; pthread_mutex_lock(&ctx->lock); - ctx->messages_level = level; - recreate_message_buffer(ctx); - + mp_msg_log_buffer_destroy(ctx->messages); + ctx->messages = NULL; + if (level >= 0) { + int size = level >= MSGL_V ? 10000 : 1000; + ctx->messages = mp_msg_log_buffer_new(ctx->mpctx->global, size, level, + msg_wakeup, ctx); + } pthread_mutex_unlock(&ctx->lock); return 0; } int mpv_get_wakeup_pipe(mpv_handle *ctx) { - pthread_mutex_lock(&ctx->lock); + pthread_mutex_lock(&ctx->wakeup_lock); if (ctx->wakeup_pipe[0] == -1) mp_make_wakeup_pipe(ctx->wakeup_pipe); - pthread_mutex_unlock(&ctx->lock); + pthread_mutex_unlock(&ctx->wakeup_lock); return ctx->wakeup_pipe[0]; } |