summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwm4 <wm4@nowhere>2019-06-16 17:44:27 +0200
committerwm4 <wm4@nowhere>2019-09-19 20:37:05 +0200
commitf2cee223111435a1a6e8a4754537737665fd5d94 (patch)
tree4a8c78ad4d09e91d797dd225fe60dc2de00bdcd5
parent2f64c84b446c389ce3c1afe210dd7b0df388f5ff (diff)
downloadmpv-f2cee223111435a1a6e8a4754537737665fd5d94.tar.bz2
mpv-f2cee223111435a1a6e8a4754537737665fd5d94.tar.xz
demux: another questionable backwards playback mud party
In theory, backward demuxing does way too much work by doing a full cache seek every time you need to step back through a packet queue. In theory, it would be exceedingly more efficient to just iterate backwards through the queue, but this is not possible because I'm too stingy to add 8 bytes per packet for backlinks. (In theory, you could probably come up with some sort of deque, that'd allow efficient iteration into any direction, but other requirements make this tricky, and I'm currently too dumb/lazy to do this. For example, the queue can grow to millions of elements, all while packet pointers need to stay valid.) Another possibility is to "locally" seek the queue. This still has less overhead than a full seek. Also, it just so happens that, as a side effect, this avoids performing range merging, which commit f4b0e7e942 "broke". That wasn't a bug at all, but since range joining is relatively slow, avoiding it is good. This is really just a coincidental side effect, I'm not even quite sure why it happens this way. There are 4 ugly things about this change: 1. To get a keyframe "before" a certain one, we recompute the target PTS, and then subtract 0.001 as arbitrary number to "fudge" it. This isn't the first place where this is done, and hey, it wasn't my damn idea that MPlayer should use floats for timestamps. (At first, it even used 32 bit timestamps.) 2. This is the first time reader_head is reset to an earlier position outside of the seek code. This might cause conceptual problems since this code is now "duplicated". 3. In theory, find_backward_restart_pos() needs to be run again after the backstep. Normally, the seek code calls it explicitly. We could call it right in the new code, but then the damn function would be recursive. We could shuffle the code around to make it a loop, but even then there'd be an offchance into running into an unexpected corner case (aka subtle bug), where it would loop forever. To avoid refactoring the code and having to think too hard about it, make it deferred - add some new state and the check_backward_seek() function for this. Great, even more subtle mutable state for this backwards shit. 4. I forgot this one, but I can assure you, it's bad. Without doubt someone will have to clean up this slightly in the future (or rip it out), but it won't be me.
-rw-r--r--demux/demux.c46
1 files changed, 41 insertions, 5 deletions
diff --git a/demux/demux.c b/demux/demux.c
index 49e75ef48f..79e74523c0 100644
--- a/demux/demux.c
+++ b/demux/demux.c
@@ -211,8 +211,9 @@ struct demux_internal {
// stuff otherwise), which adds complexity on top of it.
bool back_demuxing;
- // For backward demuxing: back-step seek needs to be triggered.
- bool need_back_seek;
+ // For backward demuxing:
+ bool need_back_seek; // back-step seek needs to be triggered
+ bool back_any_need_recheck; // at least 1 ds->back_need_recheck set
bool tracks_switched; // thread needs to inform demuxer of this
@@ -379,6 +380,7 @@ struct demux_stream {
double last_ret_dts;
// Backwards demuxing.
+ bool back_need_recheck; // flag for incremental find_backward_restart_pos work
// pos/dts of the previous keyframe packet returned; always valid if back-
// demuxing is enabled, and back_restart_eof/back_restart_next are false.
int64_t back_restart_pos;
@@ -419,6 +421,9 @@ static bool queue_seek(struct demux_internal *in, double seek_pts, int flags,
static struct demux_packet *compute_keyframe_times(struct demux_packet *pkt,
double *out_kf_min,
double *out_kf_max);
+static void find_backward_restart_pos(struct demux_stream *ds);
+static struct demux_packet *find_seek_target(struct demux_queue *queue,
+ double pts, int flags);
static uint64_t get_foward_buffered_bytes(struct demux_stream *ds)
{
@@ -1291,6 +1296,19 @@ static void perform_backward_seek(struct demux_internal *in)
pthread_mutex_lock(&in->lock);
}
+// For incremental backward demuxing search work.
+static void check_backward_seek(struct demux_internal *in)
+{
+ in->back_any_need_recheck = false;
+
+ for (int n = 0; n < in->num_streams; n++) {
+ struct demux_stream *ds = in->streams[n]->ds;
+
+ if (ds->back_need_recheck)
+ find_backward_restart_pos(ds);
+ }
+}
+
// Search for a packet to resume demuxing from.
// The implementation of this function is quite awkward, because the packet
// queue is a singly linked list without back links, while it needs to search
@@ -1300,7 +1318,9 @@ static void find_backward_restart_pos(struct demux_stream *ds)
{
struct demux_internal *in = ds->in;
- assert(ds->back_restarting);
+ ds->back_need_recheck = false;
+ if (!ds->back_restarting)
+ return;
struct demux_packet *first = ds->reader_head;
struct demux_packet *last = ds->queue->tail;
@@ -1497,8 +1517,20 @@ resume_earlier:
}
if (ds->back_seek_pos != MP_NOPTS_VALUE) {
- ds->back_seek_pos -= in->opts->back_seek_size;
- in->need_back_seek = true;
+ struct demux_packet *t =
+ find_seek_target(ds->queue, ds->back_seek_pos - 0.001, 0);
+ if (t && t != ds->reader_head) {
+ double pts;
+ compute_keyframe_times(t, &pts, NULL);
+ ds->back_seek_pos = MP_PTS_MIN(ds->back_seek_pos, pts);
+ ds_clear_reader_state(ds, false);
+ ds->reader_head = t;
+ ds->back_need_recheck = true;
+ in->back_any_need_recheck = true;
+ } else {
+ ds->back_seek_pos -= in->opts->back_seek_size;
+ in->need_back_seek = true;
+ }
}
}
@@ -2309,6 +2341,10 @@ static bool thread_work(struct demux_internal *in)
perform_backward_seek(in);
return true;
}
+ if (in->back_any_need_recheck) {
+ check_backward_seek(in);
+ return true;
+ }
if (in->seeking) {
execute_seek(in);
return true;