From 5114c69c7f85e7cd38d6928e874c5b44c951be60 Mon Sep 17 00:00:00 2001 From: wm4 Date: Fri, 7 Sep 2018 23:02:36 +0200 Subject: demux: change hack for closing subtitle files early Subtitles (and a few other file types, like playlists) are not streamed, but fully read on opening. This means keeping the file handle or network socket open is a waste of resources and could cause other weird behavior. This is why there's a hack to close them after opening. Change this hack to make the demuxer itself do this, which is less weird. (Until recently, demuxer->stream ownership was more complex, which is why it was done this way.) There is some evil shit due to a huge ownership/lifetime mess of various objects. Especially EDL (the currently only nested demuxer case) requires being careful about mp_cancel and passing down stream pointers. As one defensive programming measure, stop accessing the "stream" variable in open_given_type(), even where it would still work. This includes removing a redundant line of code, and removing the peak call, which should not be needed anymore, as the remaining demuxers do this mostly correctly. --- demux/demux.c | 41 ++++++++++++++++++++--------------------- demux/demux.h | 2 +- demux/demux_cue.c | 2 ++ demux/demux_edl.c | 1 + demux/demux_lavf.c | 16 ++++++++-------- demux/demux_libarchive.c | 1 + demux/demux_playlist.c | 2 ++ 7 files changed, 35 insertions(+), 30 deletions(-) diff --git a/demux/demux.c b/demux/demux.c index a7e30e8628..cf94805384 100644 --- a/demux/demux.c +++ b/demux/demux.c @@ -2252,20 +2252,23 @@ static void demux_init_cuesheet(struct demuxer *demuxer) } } -static void demux_maybe_replace_stream(struct demuxer *demuxer) +// A demuxer can use this during opening if all data was read from the stream. +// Calling this after opening was completed is not allowed. Also, if opening +// failed, this must not be called (or trying another demuxer would fail). +// Useful so that e.g. subtitles don't keep the file or socket open. +// Replaces it with a dummy stream for dumb reasons. +// If there's ever the situation where we can't allow the demuxer to close +// the stream, this function could ignore the request. +void demux_close_stream(struct demuxer *demuxer) { struct demux_internal *in = demuxer->in; - assert(!in->threading && demuxer == in->d_user); + assert(!in->threading && demuxer == in->d_thread); - if (demuxer->fully_read) { - MP_VERBOSE(demuxer, "assuming demuxer read all data; closing stream\n"); - free_stream(demuxer->stream); - demuxer->stream = open_memory_stream(NULL, 0); // dummy - in->d_thread->stream = demuxer->stream; - - if (demuxer->desc->control) - demuxer->desc->control(in->d_thread, DEMUXER_CTRL_REPLACE_STREAM, NULL); - } + MP_VERBOSE(demuxer, "demuxer read all data; closing stream\n"); + free_stream(demuxer->stream); + demuxer->stream = open_memory_stream(NULL, 0); // dummy + demuxer->stream->cancel = demuxer->cancel; + in->d_user->stream = demuxer->stream; } static void demux_init_ccs(struct demuxer *demuxer, struct demux_opts *opts) @@ -2335,7 +2338,6 @@ static struct demuxer *open_given_type(struct mpv_global *global, .events = DEMUX_EVENT_ALL, .duration = -1, }; - demuxer->seekable = stream->seekable; struct demux_internal *in = demuxer->in = talloc_ptrtype(demuxer, in); *in = (struct demux_internal){ @@ -2370,12 +2372,8 @@ static struct demuxer *open_given_type(struct mpv_global *global, desc->name, d_level(check)); // not for DVD/BD/DVB in particular - if (stream->seekable && (!params || !params->timeline)) - stream_seek(stream, 0); - - // Peek this much data to avoid that stream_read() run by some demuxers - // will flush previous peeked data. - stream_peek(stream, STREAM_BUFFER_SIZE); + if (demuxer->stream->seekable && (!params || !params->timeline)) + stream_seek(demuxer->stream, 0); in->d_thread->params = params; // temporary during open() int ret = demuxer->desc->open(in->d_thread, check); @@ -2414,8 +2412,8 @@ static struct demuxer *open_given_type(struct mpv_global *global, struct demuxer_params params2 = {0}; params2.timeline = tl; struct demuxer *sub = - open_given_type(global, log, &demuxer_desc_timeline, stream, - ¶ms2, DEMUX_CHECK_FORCE); + open_given_type(global, log, &demuxer_desc_timeline, + demuxer->stream, ¶ms2, DEMUX_CHECK_FORCE); if (sub) { demuxer = sub; } else { @@ -2438,6 +2436,7 @@ static const int d_force[] = {DEMUX_CHECK_FORCE, -1}; // If params->does_not_own_stream==false, this does _not_ free the stream if // opening fails. But if it succeeds, a later demux_free() call will free the // stream. +// This may free the stream parameter on success. static struct demuxer *demux_open(struct stream *stream, struct demuxer_params *params, struct mpv_global *global) @@ -2518,7 +2517,7 @@ struct demuxer *demux_open_url(const char *url, struct demuxer *d = demux_open(s, params, global); if (d) { talloc_steal(d->in, priv_cancel); - demux_maybe_replace_stream(d); + assert(d->cancel); } else { params->demuxer_failed = true; free_stream(s); diff --git a/demux/demux.h b/demux/demux.h index 2c089be823..ed013a043c 100644 --- a/demux/demux.h +++ b/demux/demux.h @@ -32,7 +32,6 @@ enum demux_ctrl { DEMUXER_CTRL_SWITCHED_TRACKS = 1, - DEMUXER_CTRL_REPLACE_STREAM, }; #define MAX_SEEK_RANGES 10 @@ -295,6 +294,7 @@ int demuxer_add_chapter(demuxer_t *demuxer, char *name, double pts, uint64_t demuxer_id); void demux_set_stream_tags(struct demuxer *demuxer, struct sh_stream *sh, struct mp_tags *tags); +void demux_close_stream(struct demuxer *demuxer); void demux_metadata_changed(demuxer_t *demuxer); void demux_update(demuxer_t *demuxer); diff --git a/demux/demux_cue.c b/demux/demux_cue.c index 941f2a88d9..f54c3f3e4a 100644 --- a/demux/demux_cue.c +++ b/demux/demux_cue.c @@ -264,6 +264,8 @@ static int try_open_file(struct demuxer *demuxer, enum demux_check check) return -1; } + demux_close_stream(demuxer); + mp_tags_merge(demuxer->metadata, p->f->tags); return 0; } diff --git a/demux/demux_edl.c b/demux/demux_edl.c index 9fbdbc9acf..c0225737e1 100644 --- a/demux/demux_edl.c +++ b/demux/demux_edl.c @@ -388,6 +388,7 @@ static int try_open_file(struct demuxer *demuxer, enum demux_check check) if (p->data.start == NULL) return -1; bstr_eatstart0(&p->data, HEADER); + demux_close_stream(demuxer); return 0; } diff --git a/demux/demux_lavf.c b/demux/demux_lavf.c index 19997a1855..eae78f9b4c 100644 --- a/demux/demux_lavf.c +++ b/demux/demux_lavf.c @@ -1063,6 +1063,14 @@ static int demux_open_lavf(demuxer_t *demuxer, enum demux_check check) demuxer->seekable = true; } + if (demuxer->fully_read) { + demux_close_stream(demuxer); + if (priv->own_stream) + free_stream(priv->stream); + priv->own_stream = false; + priv->stream = demuxer->stream; + } + return 0; } @@ -1179,18 +1187,10 @@ static void demux_seek_lavf(demuxer_t *demuxer, double seek_pts, int flags) static int demux_lavf_control(demuxer_t *demuxer, int cmd, void *arg) { - lavf_priv_t *priv = demuxer->priv; - switch (cmd) { case DEMUXER_CTRL_SWITCHED_TRACKS: select_tracks(demuxer, 0); return CONTROL_OK; - case DEMUXER_CTRL_REPLACE_STREAM: - if (priv->own_stream) - free_stream(priv->stream); - priv->own_stream = false; - priv->stream = demuxer->stream; - return CONTROL_OK; default: return CONTROL_UNKNOWN; } diff --git a/demux/demux_libarchive.c b/demux/demux_libarchive.c index 41b05368ca..c20990ae64 100644 --- a/demux/demux_libarchive.c +++ b/demux/demux_libarchive.c @@ -85,6 +85,7 @@ static int open_file(struct demuxer *demuxer, enum demux_check check) demuxer->fully_read = true; mp_archive_free(mpa); + demux_close_stream(demuxer); return 0; } diff --git a/demux/demux_playlist.c b/demux/demux_playlist.c index 0aa542534d..2e24f0c6d0 100644 --- a/demux/demux_playlist.c +++ b/demux/demux_playlist.c @@ -391,6 +391,8 @@ static int open_file(struct demuxer *demuxer, enum demux_check check) demuxer->filetype = p->format ? p->format : fmt->name; demuxer->fully_read = true; talloc_free(p); + if (ok) + demux_close_stream(demuxer); return ok ? 0 : -1; } -- cgit v1.2.3