From ab201ce04235e61534b1070cc49ea455a8296e26 Mon Sep 17 00:00:00 2001 From: wm4 Date: Sat, 18 Apr 2020 00:10:34 +0200 Subject: [PATCH] sd_lavc: mitigate evil rounding issue that could lead to off-by-1 frames A mkv sample file was provided to me, which contained a moving PGS subtitle track, with the same track rendered into the video as reference. The subtitle track appeared to stutter (while the video one was smooth). It turns out this was a timestamp rounding issue in mpv. The subtitle timestamps in the file match the video ones exactly. They're the same within the mpv demuxer too. Unfortunately, the conversion from and to libavcodec timestamps is lossy, because mpv uses a non-integer timebase, while libavcodec supports integers only. See mp_pts_to_av() and mp_pts_from_av(). The recovered timestamp is almost the same, but is off by a very minor part. As a result, the timestamps won't compare equal, and if that happens, display of the subtitle frame is skipped. Subtitle timestamps don't go through this conversion because... libavcodec is special? The libavcodec subtitle API is special. Fix this by giving it a microsecond of slack. This is basically as if we used an internal microseconds integer timebase, but only for the purpose of image subtitle display. The same could happen to sd_ass, except in practice it doesn't. ASS subtitles (well, .ass files) inherently use a timebase incompatible to video, so to ensure frame exactness, ASS timestamps are usually set to slightly before the video frame's. Discussion of better solutions: One could rewrite mpv not to use float timestamps. You'd probably pick some integer timebase instead (like microseconds), which would avoid the libavcodec interop issue. At the very least this would be a lot of work. It would be interesting to know whether the rounding in ther mpv<->lavc timestamp conversion could be fixed to round-trip in this case. The conversion tries to avoid problems by using the source timebase (e.g. milliseconds with mkv). But in general some rounding is unavoidable, because something between decoder and lowest demuxer layer could transform the timestamps. One could extend libavcodec to attach arbitrary information to avpacket and return it in the resulting avframe. To some degree, such a mechanism already exists (side data). But there are certain problems that make this unfeasible and broken. One could pass through exact mpv float timestamps by reinterpret-casting them to int64_t, the FFmpeg timestamp type. Actually mpv used to do this. But there were problems, such as FFmpeg (or things used by FFmpeg) wanting to interpret the timestamps. Awful shit that make mpv change to the current approach. There's probably more but I'm getting bored. With some luck I wasted precious seconds of your life with my nonsense. --- sub/sd_lavc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sub/sd_lavc.c b/sub/sd_lavc.c index 3ae8fc8905..3d2f137dd3 100644 --- a/sub/sd_lavc.c +++ b/sub/sd_lavc.c @@ -387,7 +387,7 @@ static struct sub *get_current(struct sd_lavc_priv *priv, double pts) if (!sub->valid) continue; if (pts == MP_NOPTS_VALUE || - ((sub->pts == MP_NOPTS_VALUE || pts >= sub->pts) && + ((sub->pts == MP_NOPTS_VALUE || pts + 1e-6 >= sub->pts) && (sub->endpts == MP_NOPTS_VALUE || pts < sub->endpts))) { // Ignore "trailing" subtitles with unknown length after 1 minute.