From 29a51900c6047798244afaca271618caeeeeeee8 Mon Sep 17 00:00:00 2001 From: wm4 Date: Sat, 19 May 2018 17:06:00 +0200 Subject: player: some further cleanup of the mp_cancel crap Alway give each demuxer its own mp_cancel instance. This makes management of the mp_cancel things much easier. Also, instead of having add/remove functions for mp_cancel slaves, replace them with a simpler to use set_parent function. Remove cancel_and_free_demuxer(), which had mpctx as parameter only to check an assumption. With this commit, demuxers have their own mp_cancel, so add demux_cancel_and_free() which makes use of it. --- demux/demux.c | 25 ++++++++++++++++++++-- demux/demux.h | 1 + misc/thread_tools.c | 39 +++++++++++++++------------------- misc/thread_tools.h | 14 ++++++------ player/loadfile.c | 60 ++++++++++++---------------------------------------- stream/stream_file.c | 2 +- 6 files changed, 62 insertions(+), 79 deletions(-) diff --git a/demux/demux.c b/demux/demux.c index 7807a919ae..33e3351b9e 100644 --- a/demux/demux.c +++ b/demux/demux.c @@ -970,6 +970,17 @@ void demux_free(struct demuxer *demuxer) talloc_free(demuxer); } +// Like demux_free(), but trigger an abort, which will force the demuxer to +// terminate immediately. If this wasn't opened with demux_open_url(), there is +// some chance this will accidentally abort other things via demuxer->cancel. +void demux_cancel_and_free(struct demuxer *demuxer) +{ + if (!demuxer) + return; + mp_cancel_trigger(demuxer->cancel); + demux_free(demuxer); +} + // Start the demuxer thread, which reads ahead packets on its own. void demux_start_thread(struct demuxer *demuxer) { @@ -2370,6 +2381,9 @@ done: // Convenience function: open the stream, enable the cache (according to params // and global opts.), open the demuxer. // Also for some reason may close the opened stream if it's not needed. +// demuxer->cancel is not the cancel parameter, but is its own object that will +// be a slave (mp_cancel_set_parent()) to provided cancel object. +// demuxer->cancel is automatically freed. struct demuxer *demux_open_url(const char *url, struct demuxer_params *params, struct mp_cancel *cancel, @@ -2379,18 +2393,25 @@ struct demuxer *demux_open_url(const char *url, if (!params) params = &dummy; assert(!params->does_not_own_stream); // API user error + struct mp_cancel *priv_cancel = mp_cancel_new(NULL); + if (cancel) + mp_cancel_set_parent(priv_cancel, cancel); struct stream *s = stream_create(url, STREAM_READ | params->stream_flags, - cancel, global); - if (!s) + priv_cancel, global); + if (!s) { + talloc_free(priv_cancel); return NULL; + } if (!params->disable_cache) stream_enable_cache_defaults(&s); struct demuxer *d = demux_open(s, params, global); if (d) { + talloc_steal(d->in, priv_cancel); demux_maybe_replace_stream(d); } else { params->demuxer_failed = true; free_stream(s); + talloc_free(priv_cancel); } return d; } diff --git a/demux/demux.h b/demux/demux.h index 6a8a08d8df..f5b203590f 100644 --- a/demux/demux.h +++ b/demux/demux.h @@ -251,6 +251,7 @@ typedef struct { } demux_program_t; void demux_free(struct demuxer *demuxer); +void demux_cancel_and_free(struct demuxer *demuxer); void demux_add_packet(struct sh_stream *stream, demux_packet_t *dp); void demuxer_feed_caption(struct sh_stream *stream, demux_packet_t *dp); diff --git a/misc/thread_tools.c b/misc/thread_tools.c index ecf6bc2381..91b774eb93 100644 --- a/misc/thread_tools.c +++ b/misc/thread_tools.c @@ -105,12 +105,7 @@ static void cancel_destroy(void *p) assert(!c->slaves.head); // API user error - // We can access c->parent without synchronization, because: - // - since c is being destroyed, nobody can explicitly remove it as slave - // at the same time - // - c->parent needs to stay valid as long as the slave exists - if (c->parent) - mp_cancel_remove_slave(c->parent, c); + mp_cancel_set_parent(c, NULL); if (c->wakeup_pipe[0] >= 0) { close(c->wakeup_pipe[0]); @@ -225,25 +220,25 @@ void mp_cancel_set_cb(struct mp_cancel *c, void (*cb)(void *ctx), void *ctx) pthread_mutex_unlock(&c->lock); } -void mp_cancel_add_slave(struct mp_cancel *c, struct mp_cancel *slave) -{ - pthread_mutex_lock(&c->lock); - assert(!slave->parent); - slave->parent = c; - LL_APPEND(siblings, &c->slaves, slave); - retrigger_locked(c); - pthread_mutex_unlock(&c->lock); -} - -void mp_cancel_remove_slave(struct mp_cancel *c, struct mp_cancel *slave) +void mp_cancel_set_parent(struct mp_cancel *slave, struct mp_cancel *parent) { - pthread_mutex_lock(&c->lock); + // We can access c->parent without synchronization, because: + // - concurrent mp_cancel_set_parent() calls to slave are not allowed + // - slave->parent needs to stay valid as long as the slave exists + if (slave->parent == parent) + return; if (slave->parent) { - assert(slave->parent == c); - slave->parent = NULL; - LL_REMOVE(siblings, &c->slaves, slave); + pthread_mutex_lock(&slave->parent->lock); + LL_REMOVE(siblings, &slave->parent->slaves, slave); + pthread_mutex_unlock(&slave->parent->lock); + } + slave->parent = parent; + if (slave->parent) { + pthread_mutex_lock(&slave->parent->lock); + LL_APPEND(siblings, &slave->parent->slaves, slave); + retrigger_locked(slave->parent); + pthread_mutex_unlock(&slave->parent->lock); } - pthread_mutex_unlock(&c->lock); } int mp_cancel_get_fd(struct mp_cancel *c) diff --git a/misc/thread_tools.h b/misc/thread_tools.h index 2198181e6c..89d84ce0b6 100644 --- a/misc/thread_tools.h +++ b/misc/thread_tools.h @@ -67,14 +67,12 @@ void mp_cancel_reset(struct mp_cancel *c); // There is only one callback. Create a slave mp_cancel to get a private one. void mp_cancel_set_cb(struct mp_cancel *c, void (*cb)(void *ctx), void *ctx); -// If c gets triggered, automatically trigger slave. Trying to add a slave more -// than once or to multiple parents is undefined behavior. -// The parent mp_cancel must remain valid until the slave is manually removed -// or destroyed. Destroying a mp_cancel that still has slaves is an error. -void mp_cancel_add_slave(struct mp_cancel *c, struct mp_cancel *slave); - -// Undo mp_cancel_add_slave(). Ignores never added slaves for easier cleanup. -void mp_cancel_remove_slave(struct mp_cancel *c, struct mp_cancel *slave); +// If parent gets triggered, automatically trigger slave. There is only 1 +// parent; setting NULL clears the parent. Freeing slave also automatically +// ends the parent link, but the parent mp_cancel must remain valid until the +// slave is manually removed or destroyed. Destroying a mp_cancel that still +// has slaves is an error. +void mp_cancel_set_parent(struct mp_cancel *slave, struct mp_cancel *parent); // win32 "Event" HANDLE that indicates the current mp_cancel state. void *mp_cancel_get_event(struct mp_cancel *c); diff --git a/player/loadfile.c b/player/loadfile.c index 297ab8ba97..194372e271 100644 --- a/player/loadfile.c +++ b/player/loadfile.c @@ -123,25 +123,6 @@ void mp_abort_trigger_locked(struct MPContext *mpctx, mp_cancel_trigger(abort->cancel); } -static void cancel_and_free_demuxer(struct MPContext *mpctx, - struct demuxer **demuxer) -{ - if (!*demuxer) - return; - - struct mp_cancel *cancel = (*demuxer)->cancel; - assert(cancel != mpctx->playback_abort); - - // Explicitly trigger it so freeing the demuxer can't block on I/O. - if (cancel) - mp_cancel_trigger(cancel); - - demux_free(*demuxer); - *demuxer = NULL; - - talloc_free(cancel); -} - static void uninit_demuxer(struct MPContext *mpctx) { for (int r = 0; r < NUM_PTRACKS; r++) { @@ -163,7 +144,8 @@ static void uninit_demuxer(struct MPContext *mpctx) } mpctx->num_tracks = 0; - cancel_and_free_demuxer(mpctx, &mpctx->demuxer); + demux_cancel_and_free(mpctx->demuxer); + mpctx->demuxer = NULL; } #define APPEND(s, ...) mp_snprintf_cat(s, sizeof(s), __VA_ARGS__) @@ -635,7 +617,7 @@ bool mp_remove_track(struct MPContext *mpctx, struct track *track) in_use |= mpctx->tracks[n]->demuxer == d; if (!in_use) - cancel_and_free_demuxer(mpctx, &d); + demux_cancel_and_free(d); mp_notify(mpctx, MPV_EVENT_TRACKS_CHANGED, NULL); @@ -669,13 +651,10 @@ int mp_add_external_file(struct MPContext *mpctx, char *filename, break; } - struct mp_cancel *demux_cancel = mp_cancel_new(NULL); - mp_cancel_add_slave(cancel, demux_cancel); - mp_core_unlock(mpctx); struct demuxer *demuxer = - demux_open_url(filename, ¶ms, demux_cancel, mpctx->global); + demux_open_url(filename, ¶ms, cancel, mpctx->global); if (demuxer) enable_demux_thread(mpctx, demuxer); @@ -722,17 +701,12 @@ int mp_add_external_file(struct MPContext *mpctx, char *filename, first_num = mpctx->num_tracks - 1; } - mp_cancel_remove_slave(cancel, demux_cancel); - mp_cancel_add_slave(mpctx->playback_abort, demux_cancel); + mp_cancel_set_parent(demuxer->cancel, mpctx->playback_abort); return first_num; err_out: - if (demuxer) { - cancel_and_free_demuxer(mpctx, &demuxer); - } else { - talloc_free(demux_cancel); - } + demux_cancel_and_free(demuxer); if (!mp_cancel_test(cancel)) MP_ERR(mpctx, "Can not open external file %s.\n", disp_filename); return -1; @@ -865,17 +839,14 @@ static void load_chapters(struct MPContext *mpctx) char *chapter_file = mpctx->opts->chapter_file; if (chapter_file && chapter_file[0]) { chapter_file = talloc_strdup(NULL, chapter_file); - struct mp_cancel *cancel = mp_cancel_new(NULL); - mp_cancel_add_slave(mpctx->playback_abort, cancel); mp_core_unlock(mpctx); - struct demuxer *demux = demux_open_url(chapter_file, NULL, cancel, + struct demuxer *demux = demux_open_url(chapter_file, NULL, + mpctx->playback_abort, mpctx->global); mp_core_lock(mpctx); if (demux) { src = demux; free_src = true; - } else { - talloc_free(cancel); } talloc_free(mpctx->chapters); mpctx->chapters = NULL; @@ -891,7 +862,7 @@ static void load_chapters(struct MPContext *mpctx) } } if (free_src) - cancel_and_free_demuxer(mpctx, &src); + demux_cancel_and_free(src); } static void load_per_file_options(m_config_t *conf, @@ -944,11 +915,9 @@ static void cancel_open(struct MPContext *mpctx) pthread_join(mpctx->open_thread, NULL); mpctx->open_active = false; - if (mpctx->open_res_demuxer) { - assert(mpctx->open_res_demuxer->cancel == mpctx->open_cancel); - mpctx->open_cancel = NULL; - cancel_and_free_demuxer(mpctx, &mpctx->open_res_demuxer); - } + if (mpctx->open_res_demuxer) + demux_cancel_and_free(mpctx->open_res_demuxer); + mpctx->open_res_demuxer = NULL; TA_FREEP(&mpctx->open_cancel); TA_FREEP(&mpctx->open_url); @@ -1010,7 +979,7 @@ static void open_demux_reentrant(struct MPContext *mpctx) start_open(mpctx, url, mpctx->playing->stream_flags); // User abort should cancel the opener now. - mp_cancel_add_slave(mpctx->playback_abort, mpctx->open_cancel); + mp_cancel_set_parent(mpctx->open_cancel, mpctx->playback_abort); while (!atomic_load(&mpctx->open_done)) { mp_idle(mpctx); @@ -1020,10 +989,9 @@ static void open_demux_reentrant(struct MPContext *mpctx) } if (mpctx->open_res_demuxer) { - assert(mpctx->open_res_demuxer->cancel == mpctx->open_cancel); mpctx->demuxer = mpctx->open_res_demuxer; mpctx->open_res_demuxer = NULL; - mpctx->open_cancel = NULL; + mp_cancel_set_parent(mpctx->demuxer->cancel, mpctx->playback_abort); } else { mpctx->error_playing = mpctx->open_res_error; } diff --git a/stream/stream_file.c b/stream/stream_file.c index 785e96b12c..20b4d7c0c3 100644 --- a/stream/stream_file.c +++ b/stream/stream_file.c @@ -365,7 +365,7 @@ static int open_f(stream_t *stream) p->cancel = mp_cancel_new(p); if (stream->cancel) - mp_cancel_add_slave(stream->cancel, p->cancel); + mp_cancel_set_parent(p->cancel, stream->cancel); return STREAM_OK; } -- cgit v1.2.3