From 756adee999bbc24b79d02fea34f8512239de016c Mon Sep 17 00:00:00 2001 From: wm4 Date: Mon, 15 Dec 2014 14:44:25 +0100 Subject: [PATCH] client API: be more lenient about mpv_suspend/resume mismatches Before this commit, this was defined to trigger undefined behavior. This was nice because it required less code; but on the other hand, Lua as well as IPC support had to check these things manually. Do it directly in the API to avoid code duplication, and to make the API more robust. (The total code size still grows, though...) Since all of the failure cases were originally meant to ruin things forever, there is no way to return error codes. So just print the errors. --- input/ipc.c | 20 ++++---------------- player/client.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- player/client.h | 2 ++ player/lua.c | 33 ++++++--------------------------- 4 files changed, 57 insertions(+), 45 deletions(-) diff --git a/input/ipc.c b/input/ipc.c index 876f40c8a7..7d15ac84d0 100644 --- a/input/ipc.c +++ b/input/ipc.c @@ -61,8 +61,6 @@ struct client_arg { bool close_client_fd; bool writable; - - int suspend_counter; }; static mpv_node *mpv_node_map_get(mpv_node *src, const char *key) @@ -420,21 +418,11 @@ static char *json_execute_command(struct client_arg *arg, void *ta_parent, rc = mpv_request_log_messages(arg->client, cmd_node->u.list->values[1].u.string); } else if (!strcmp("suspend", cmd)) { - if (arg->suspend_counter < INT_MAX) { - mpv_suspend(arg->client); - arg->suspend_counter++; - rc = MPV_ERROR_SUCCESS; - } else { - rc = MPV_ERROR_INVALID_PARAMETER; - } + mpv_suspend(arg->client); + rc = MPV_ERROR_SUCCESS; } else if (!strcmp("resume", cmd)) { - if (arg->suspend_counter > 0) { - mpv_resume(arg->client); - arg->suspend_counter--; - rc = MPV_ERROR_SUCCESS; - } else { - rc = MPV_ERROR_INVALID_PARAMETER; - } + mpv_resume(arg->client); + rc = MPV_ERROR_SUCCESS; } else { mpv_node result_node; diff --git a/player/client.c b/player/client.c index a46a37b10a..11a2fda4d8 100644 --- a/player/client.c +++ b/player/client.c @@ -108,6 +108,7 @@ struct mpv_handle { uint64_t event_mask; bool queued_wakeup; bool choke_warning; + int suspend_count; mpv_event *events; // ringbuffer of max_events entries int max_events; // allocated number of entries in events @@ -300,12 +301,47 @@ void mpv_set_wakeup_callback(mpv_handle *ctx, void (*cb)(void *d), void *d) void mpv_suspend(mpv_handle *ctx) { - mp_dispatch_suspend(ctx->mpctx->dispatch); + bool do_suspend = false; + + pthread_mutex_lock(&ctx->lock); + if (ctx->suspend_count == INT_MAX) { + MP_ERR(ctx, "suspend counter overflow"); + } else { + do_suspend = ctx->suspend_count == 0; + ctx->suspend_count++; + } + pthread_mutex_unlock(&ctx->lock); + + if (do_suspend) + mp_dispatch_suspend(ctx->mpctx->dispatch); } void mpv_resume(mpv_handle *ctx) { - mp_dispatch_resume(ctx->mpctx->dispatch); + bool do_resume = false; + + pthread_mutex_lock(&ctx->lock); + if (ctx->suspend_count == 0) { + MP_ERR(ctx, "suspend counter underflow"); + } else { + do_resume = ctx->suspend_count == 1; + ctx->suspend_count--; + } + pthread_mutex_unlock(&ctx->lock); + + if (do_resume) + mp_dispatch_resume(ctx->mpctx->dispatch); +} + +void mp_resume_all(mpv_handle *ctx) +{ + pthread_mutex_lock(&ctx->lock); + bool do_resume = ctx->suspend_count > 0; + ctx->suspend_count = 0; + pthread_mutex_unlock(&ctx->lock); + + if (do_resume) + mp_dispatch_resume(ctx->mpctx->dispatch); } static void lock_core(mpv_handle *ctx) @@ -325,6 +361,8 @@ void mpv_detach_destroy(mpv_handle *ctx) if (!ctx) return; + mp_resume_all(ctx); + pthread_mutex_lock(&ctx->lock); // reserved_events equals the number of asynchronous requests that weren't // yet replied. In order to avoid that trying to reply to a removed client @@ -677,6 +715,11 @@ mpv_event *mpv_wait_event(mpv_handle *ctx, double timeout) talloc_free_children(event); while (1) { + // This will almost surely lead to a deadlock. (Polling is still ok.) + if (ctx->suspend_count && timeout > 0) { + MP_ERR(ctx, "attempting to wait while core is suspended"); + break; + } if (ctx->num_events) { *event = ctx->events[ctx->first_event]; ctx->first_event = (ctx->first_event + 1) % ctx->max_events; diff --git a/player/client.h b/player/client.h index 4e116b3bca..aeb2e886e7 100644 --- a/player/client.h +++ b/player/client.h @@ -32,6 +32,8 @@ struct mpv_handle *mp_new_client(struct mp_client_api *clients, const char *name struct mp_log *mp_client_get_log(struct mpv_handle *ctx); struct MPContext *mp_client_get_core(struct mpv_handle *ctx); +void mp_resume_all(struct mpv_handle *ctx); + // m_option.c void *node_get_alloc(struct mpv_node *node); diff --git a/player/lua.c b/player/lua.c index 1110137c68..47bcc07110 100644 --- a/player/lua.c +++ b/player/lua.c @@ -80,7 +80,6 @@ struct script_ctx { struct mp_log *log; struct mpv_handle *client; struct MPContext *mpctx; - int suspended; }; #if LUA_VERSION_NUM <= 501 @@ -393,8 +392,7 @@ static int load_lua(struct mpv_handle *client, const char *fname) r = 0; error_out: - if (ctx->suspended) - mpv_resume(ctx->client); + mp_resume_all(client); if (ctx->state) lua_close(ctx->state); talloc_free(ctx); @@ -452,33 +450,20 @@ static int script_find_config_file(lua_State *L) static int script_suspend(lua_State *L) { struct script_ctx *ctx = get_ctx(L); - if (!ctx->suspended) - mpv_suspend(ctx->client); - ctx->suspended++; + mpv_suspend(ctx->client); return 0; } static int script_resume(lua_State *L) { struct script_ctx *ctx = get_ctx(L); - if (ctx->suspended < 1) - luaL_error(L, "trying to resume, but core is not suspended"); - ctx->suspended--; - if (!ctx->suspended) - mpv_resume(ctx->client); + mpv_resume(ctx->client); return 0; } -static void resume_all(struct script_ctx *ctx) -{ - if (ctx->suspended) - mpv_resume(ctx->client); - ctx->suspended = 0; -} - static int script_resume_all(lua_State *L) { - resume_all(get_ctx(L)); + mp_resume_all(get_ctx(L)->client); return 0; } @@ -488,13 +473,7 @@ static int script_wait_event(lua_State *L) { struct script_ctx *ctx = get_ctx(L); - double timeout = luaL_optnumber(L, 1, 1e20); - - // This will almost surely lead to a deadlock. (Polling is still ok.) - if (ctx->suspended && timeout > 0) - luaL_error(L, "attempting to wait while core is suspended"); - - mpv_event *event = mpv_wait_event(ctx->client, timeout); + mpv_event *event = mpv_wait_event(ctx->client, luaL_optnumber(L, 1, 1e20)); lua_newtable(L); // event lua_pushstring(L, mpv_event_name(event->event_id)); // event name @@ -1196,7 +1175,7 @@ static int script_subprocess(lua_State *L) luaL_checktype(L, 1, LUA_TTABLE); void *tmp = mp_lua_PITA(L); - resume_all(ctx); + mp_resume_all(ctx->client); lua_getfield(L, 1, "args"); // args int num_args = mp_lua_len(L, -1);