From 5cc68c792be1d0ae42017f6960ba1d0448646ff5 Mon Sep 17 00:00:00 2001 From: wm4 Date: Sat, 7 Jun 2014 23:15:07 +0200 Subject: 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. --- player/client.c | 97 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 42 deletions(-) (limited to 'player') 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]; } -- cgit v1.2.3