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

@@ -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;
 }

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);

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)
+void mp_cancel_set_parent(struct mp_cancel *slave, struct mp_cancel *parent)
 {
-    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)
-{
-    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)

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);

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, &params, demux_cancel, mpctx->global);
+        demux_open_url(filename, &params, 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;
     }

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;
 }