GNOME Bugzilla – Bug 707340
qtmux: should NOT use PTS if DTS is missing
Last modified: 2016-11-22 14:24:23 UTC
This commit http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=4f4f6432ccf99779a46be70bb3864ff0b673f7d2 if DTS is missing on a buffer for some reason, PTS is taken instead. But that is not good. Because if there is DTS on next buffer, it may be lower than PTS on previous buffer. Thus you end up with negative delta. Timestamp handling in qtmux is a mess, but this makes it even worse. I think what should happen on missing DTS is this: If DTS are missing at the beginning (probably because it was clipped to segment start) make DTS = 0 This way there might be few buffers at the beginning with 0 duration, but there's not much we can do about it until gstreamer can handle negative DTS. If DTS are missing mid-stream, take the previous sample DTS (or previous sample DTS + previous sample duration).
And by 0 I possibly mean pad->first_ts, although I'm not sure why that is always taken from PTS even though there might be DTS on the buffer...
Also, the above of course only applies for streams with have_dts = true
I also don't think this is right. If it fixes something it should be fixed differently without assigning pts values to global state dts variables, that's just confusing.
If you revert that commit, make sure you error out on buffers that have DTS set but no PTS set. (fwiw, someone needs to rewrite that whole pts/dts handling)
I'll submit a patch today, why should it error out when PTS is not set and DTS is set? While it is a bit weird, what's wrong with just having CTTS 0 in that case?
As for rewriting the timestamp handling, first we need a way to represent negative DTS in gstreamer and agree on consistent DTS interpretation within gstreamer :)
Created attachment 254005 [details] [review] Revert previous patch
Created attachment 254006 [details] [review] Patch This patch does couple o things: 1 When DTS is missing on buffer, it uses either segment start or last buffer end time, which ever is bigger. I think this should produce best results even for cases like segment in mid stream where first few DTS are set to -1 because they are before segment start 2 It sets first_ts to DTS for streams that have DTS. Right now it is always set to first buffer PTS, which doesn't seem to make much sense since DTS is used for durations in such streams. 3 it makes sure duration is a valid number for last buffer (otherwise GST_CLOCK_TIME_NONE is used in calculations for last sample, which doesn't produce good results) :)
Review of attachment 254006 [details] [review]: Looks good and makes sense to me, but ideally please split these into separate patches as it fixes a few separate things. Makes finding causes of bugs with git bisect easier later
Created attachment 254915 [details] [review] qtdemux: use segment.start or last buffer end time in case of missing DTS
Created attachment 254916 [details] [review] qtdemux: make sure duration is a valid number for last buffer
Created attachment 254917 [details] [review] qtdemux: set first_ts to DTS for streams that have DTS
Created attachment 254918 [details] [review] qtmux: use segment.start or last buffer end time in case of missing DTS
Created attachment 254919 [details] [review] qtmux: make sure duration is a valid number for last buffer
Created attachment 254920 [details] [review] qtmux: set first_ts to DTS for streams that have DTS
commit b363832c2ccc66b2edbd9298924af59664e20716 Author: Matej Knopp <matej.knopp@gmail.com> Date: Sat Sep 14 15:56:04 2013 +0200 qtmux: set first_ts to DTS for streams that have DTS https://bugzilla.gnome.org/show_bug.cgi?id=707340 commit 39f7e52266fde3b3c035e22cbcbb2bb1fa207b17 Author: Matej Knopp <matej.knopp@gmail.com> Date: Sat Sep 14 15:55:22 2013 +0200 qtmux: make sure duration is a valid number for last buffer https://bugzilla.gnome.org/show_bug.cgi?id=707340 commit 4e3c13c87c258c9c95e2217d32ab314d12b5fffc Author: Matej Knopp <matej.knopp@gmail.com> Date: Sat Sep 14 15:54:29 2013 +0200 qtmux: use segment.start or last buffer end time in case of missing DTS https://bugzilla.gnome.org/show_bug.cgi?id=707340 commit 85728c04c4ef57af623bf52340a1b8f047266ef1 Author: Matej Knopp <matej.knopp@gmail.com> Date: Tue Sep 3 18:14:04 2013 +0200 Revert qtmux: Use buffer PTS if DTS is not set" This reverts commit f72c3cf71fde622067f41f31a53978ba4c94469d. https://bugzilla.gnome.org/show_bug.cgi?id=707340