GNOME Bugzilla – Bug 656115
mpegpsdemux: handle SCR jumps/discontinuities more gracefully
Last modified: 2011-12-10 13:38:13 UTC
Created attachment 193384 [details] [review] patch v1 One of my dvds jump on some position and miss about 1 minute of stream. The reason was mpeg timestamps. On some position scr difference is negative. It produced negative timestamps. Since it was converted to unsigned value, gstreamer timestamps was invalid. Instead of increasing mpeg ts, they was decreasing till it started to be positive.
Review of attachment 193384 [details] [review]: Since the code just above specifically tests if the new ts is higher or lower and orders the operation based on this, it would make more sense to just make diff a guint64 in the first place. The check should then just work without having to test for < 0. Various typos/spelling/grammar mistakes too (diffenrence, reajust, they was, scip). I think you're not a native English speaker, but some are easily fixed on re-read typos.
Created attachment 202362 [details] [review] patch v2
I pushed this a while ago but forgot to close the bug: commit 2099a3945957fa0ddf10cd8b2ddef0e54f86bcdf Author: Alexey Fisher <bug-track@fisher-privat.net> Date: Tue Nov 29 12:20:51 2011 +0100 mpegpsdemux: recalculate adjust if difference is negative One of my dvds jump on some position and miss about 1 minute of stream. The reason was mpeg timestamps. On some position scr difference is negative. It produced negative timestamps. Since it was converted to unsigned value, gstreamer timestamps was invalid. Instead of increasing mpeg ts, they was decreasing till it started to be positive. The jump in timestamps caused mpeg2dec to skip frames to make QoS happy. This patch just make diff unsigned to avoid negative values. Signed-off-by: Alexey Fisher <bug-track@fisher-privat.net> https://bugzilla.gnome.org/show_bug.cgi?id=656115
I have updated the summary to be hopefully more concise, please update it or change it back if I misunderstood the gist of these changes.
I am probably missing something here, but his is how the diff is computed if (G_UNLIKELY (old_scr > scr_adjusted)) diff = old_scr - scr_adjusted; else diff = scr_adjusted - old_scr; what difference does it make whether it's signed or not?
in some cases where old_scr is really big and scr_adjusted really small, then you will get really big _unsigned_ diff. If the diff is signed then you will get negative diff.
That's the part I don't quite get. SCR is in mpeg time, so it has original resolution of 33 bits. How can it get so big it overflows gint64 ?
i do not know. I didn't had time to research this problem deeper. May be diff is just top of the iceberg.
That's what I was thinking. If it overflows the problem might be somewhere else.
if you wont to investigate this problem i can check if i have some samples now, or if i will get it in the future. if you just curios, then i do not really have time for it now.
If you can provide some samples I'd like to have a look. At least to see if the samples are broken or there is something wrong with the demuxer.
Just checked: the samples i currently have are not affected. The problem with this bug is, the jump in video is "invisible", except you know what is there. So to track it down is good to send a big error message at least to console. I will keep my eyes open, if i will not forget to keep my warning patch in the source. Or you can add some warning patch to the git source... :)