GNOME Bugzilla – Bug 632222
mpegvideoparse: missing timestamps
Last modified: 2018-05-04 09:43:14 UTC
mpegvideoparse is missing timestamps on mpeg frames. This is needed if you want to send this unstamped mpeg2 streams via rtp. (rtp timing goes corrupt) Attached file can be used to demonstrate this problem. Decoders can succesfully decode (and timestamp) this stream, so all necesary information is available. gst-launch filesrc location=mpegvideoparse.mpeg ! mpegvideoparse ! fakesink -v | grep chain | sed 's/^.* (\([0-9]*.*\), d.*$/\1/' 86 bytes, timestamp: none 41622 bytes, timestamp: 0:00:00.000000000 41709 bytes, timestamp: none 41708 bytes, timestamp: none 41708 bytes, timestamp: none 41708 bytes, timestamp: none 86 bytes, timestamp: none 41623 bytes, timestamp: none 41708 bytes, timestamp: none 41708 bytes, timestamp: none 41708 bytes, timestamp: none 41709 bytes, timestamp: none 86 bytes, timestamp: none gst-launch filesrc location=mpegvideoparse.mpeg ! mpegvideoparse ! mpeg2dec ! fakesink -v | grep chain | sed 's/^.* (\([0-9]*.*\), d.*$/\1/' 518400 bytes, timestamp: 0:00:00.000000000 518400 bytes, timestamp: 0:00:00.033366666 518400 bytes, timestamp: 0:00:00.066733332 518400 bytes, timestamp: 0:00:00.100099998 518400 bytes, timestamp: 0:00:00.133466664 518400 bytes, timestamp: 0:00:00.166833330 518400 bytes, timestamp: 0:00:00.200199996 518400 bytes, timestamp: 0:00:00.233566662
Created attachment 172430 [details] mpegvideoparse.mpeg
Created attachment 172610 [details] [review] mpegvideoparse: fix timestamp generation Use information from the gop header and picture header to calculate the picture timestamp. (time_code and temporal_reference)
Timestamps now look like expected: gst-launch filesrc location=mpegvideoparse.mpeg ! mpegvideoparse ! fakesink -v | grep chain | sed 's/^.* (\([0-9]*.*\), d.*$/\1/' 86 bytes, timestamp: none 41622 bytes, timestamp: 0:00:00.033366666 41709 bytes, timestamp: 0:00:00.000000000 41708 bytes, timestamp: 0:00:00.100100000 41708 bytes, timestamp: 0:00:00.066733333 41708 bytes, timestamp: 0:00:00.133466666 86 bytes, timestamp: none 41623 bytes, timestamp: 0:00:00.199199999 41708 bytes, timestamp: 0:00:00.165833333 41708 bytes, timestamp: 0:00:00.265933333 41708 bytes, timestamp: 0:00:00.232566666 41709 bytes, timestamp: 0:00:00.299299999 86 bytes, timestamp: none
Created attachment 172618 [details] [review] mpegvideoparse: fix timestamp generation Use information from the gop header and picture header to calculate the picture timestamp. (time_code and temporal_reference) and adapt to upstream timestamps if provided.
commit 2271608c4314d6d0a685c18c5c47d55495586159 Author: Thijs Vermeir <thijsvermeir@gmail.com> Date: Mon Oct 18 15:32:14 2010 +0200 mpegvideoparse: fix timestamp generation Use information from the gop header and picture header to calculate the picture timestamp. (time_code and temporal_reference) and adapt to upstream timestamps if provided. https://bugzilla.gnome.org/show_bug.cgi?id=632222
This seems to have caused a regression, see bug #636279.
commit e5f1cdd0e9362c14784c4e1e8c762623537af4c7 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Fri Jan 7 18:49:02 2011 +0000 Revert "mpegvideoparse: fix timestamp generation" This reverts commit 2271608c4314d6d0a685c18c5c47d55495586159. This patch needs more work so it doesn't cause grave playback regressions (multi-second freezes) with some files that have slightly broken timestamps but play fine everywhere else. https://bugzilla.gnome.org/show_bug.cgi?id=636279 https://bugzilla.gnome.org/show_bug.cgi?id=632222
So, if I get it correctly; the code gets broken again because faulty files are used as a reference? This doesn't seem quite right IMO.
No, the patch got reverted for the release because it breaks something that used to work before and that works everywhere else, whereas the thing it fixes didn't work before (correct me if I'm wrong). I don't know for sure if the file is faulty or not, and it doesn't really matter that much given that other players handle it fine. I'm working mainly on the principle that it is more desirable to avoid a playback regression than to add a new fix or feature. Surely with some tweaking this can be made to work properly for both cases?
Any progress on this?
Comment on attachment 172618 [details] [review] mpegvideoparse: fix timestamp generation Setting to needs-work because it causes regressions
Hi, Is this bug still exist?? Any updates or patch or this doesnt occur with latest git master?? regards JasonP
For the 0.10, this is an issue for sure. On the 1.0; I would need to check our internal git repo; but I believe we ported the patch.
Thanks Marc. I hope this is resolved in 1.x.x. JasonP
It's not fixed yet in 1.x because the patch here causes regressions but nobody had the time yet to fix it.
I tried to reproduce this issue using pipeline mentioned in description. I couldn't get expected results. Below given are the results. gst-launch --version gst-launch-1.0 version 1.0.4 GStreamer 1.0.4 Unknown package origin gst-launch filesrc location=mpegvideoparse.mpeg ! mpegvideoparse ! fakesink -v Setting pipeline to PAUSED ... Pipeline is PREROLLING ... /GstPipeline:pipeline0/GstMpegvParse:mpegvparse0.GstPad:src: caps = video/mpeg, mpegversion=(int)2, systemstream=(boolean)false, parsed=(boolean)true, width=(int)720, height=(int)480, framerate=(fraction)30000/1001, pixel-aspect-ratio=(fraction)8/9, codec_data=(buffer)000001b32d01e024249f228110111112121213131313141414141415151515151516161616161616171717171717171718181819181818191a1a1a1a191b1b1b1b1b1c1c1c1c1e1e1e1f1f21000001b5148200010000, profile=(string)main, level=(string)main, interlace-mode=(string)mixed /GstPipeline:pipeline0/GstFakeSink:fakesink0.GstPad:sink: caps = video/mpeg, mpegversion=(int)2, systemstream=(boolean)false, parsed=(boolean)true, width=(int)720, height=(int)480, framerate=(fraction)30000/1001, pixel-aspect-ratio=(fraction)8/9, codec_data=(buffer)000001b32d01e024249f228110111112121213131313141414141415151515151516161616161616171717171717171718181819181818191a1a1a1a191b1b1b1b1b1c1c1c1c1e1e1e1f1f21000001b5148200010000, profile=(string)main, level=(string)main, interlace-mode=(string)mixed Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstSystemClock Got EOS from element "pipeline0". Execution ended after 2160984 ns. Setting pipeline to PAUSED ... Setting pipeline to READY ... Setting pipeline to NULL ... Freeing pipeline ...
gst-launch --version gst-launch-1.0 version 1.0.4 GStreamer 1.0.4 test1: gst-launch filesrc location=/root/Raju/long_duration/mpegvideoparse.mpeg ! mpegvideoparse ! checksumsink -v 0:00:00.000000000 bf3bbe5910b40f2d43a2b05fb109e1d68bd7a3d2 99:99:99.999999999 01a72c33826b1803cae0e05acb5ba23061e2b74c 99:99:99.999999999 b9786afee2b0539089177ef9a3c63f08ee2623ab 99:99:99.999999999 d4a627cd9450f738314dae639bd00da04b989204 99:99:99.999999999 4c605ad96528cbf428e723c30ba0eb9657b64737 99:99:99.999999999 93273fd1a7985154daa3a6b808e3a9a28df77b1b 99:99:99.999999999 c139db4895bbb6c18821539eec9fa54f4affea5f 99:99:99.999999999 5aa3ce0de520ee464d7ff5f5aaba4253a712e627 99:99:99.999999999 ba0aab6703317e0717b57dbb9557c9f0a0095408 99:99:99.999999999 7b08df99c8da6e03bd4f62326d2039c2bceaee98 99:99:99.999999999 5370105280abf271a2b5e0ab34ba626373410949 Test2: gst-launch filesrc location=/root/Raju/long_duration/mpegvideoparse.mpeg ! mpegvideoparse ! avdec_mpeg2video ! checksumsink -v 0:00:00.000000000 c1cd864312d25733b1227615695bcf5acd602a59 0:00:00.033366666 c6412ddad67db29668199621a335ab8c8d17a52d 0:00:00.066733332 4b3cd5d191056ba2bd8bffcc8bb63a0eded3ffce 0:00:00.100099998 e5988c9cc1e60dc459be5868b68aab8a0a6514d2 0:00:00.133466664 3b005dda7d949dcba3cf5ef5c41d118322f5b8aa 0:00:00.166833330 4961b4900335ca9749579dd9b9728139a4828269 0:00:00.200199996 42c3e26337ff4220ecaf12df2f381a1c36202840 0:00:00.233566662 d3adfbcb3860cf8d6a205dfa307a614c24bedab1 0:00:00.233566662 d3adfbcb3860cf8d6a205dfa307a614c24bedab1 0:00:00.233566662 60bc3c3a55417c1363de72fd687472c5f2a03d40
I think there is an change with the -v option in fakesink that does not print the headers anymore in the 1.0
(In reply to comment #18) > I think there is an change with the -v option in fakesink that does not print > the headers anymore in the 1.0 Ya you r right Marc, thats why I re-ran experiment with checksumsink which dumps timestamp.
or you could just set "silent=False" on fakesink ...
(In reply to comment #20) > or you could just set "silent=False" on fakesink ... It worked. Thanks Edward. The bug still exist in gstreaemr-1.0.4 version. Here are the results. gst-launch-1.0 filesrc location=/root/Raju/long_duration/mpegvideoparse.mpeg ! mpegvideoparse ! fakesink silent=false -v | grep chain | sed 's/^.* (\([0-9]*.*\), d.*$/\1/' 41708 bytes, dts: 0:00:00.000000000, pts: 0:00:00.000000000 41709 bytes, dts: 0:00:00.033366666, pts: none 41708 bytes, dts: 0:00:00.066733332, pts: none 41708 bytes, dts: 0:00:00.100099998, pts: none 41708 bytes, dts: 0:00:00.133466664, pts: none 41709 bytes, dts: 0:00:00.166833330, pts: none 41708 bytes, dts: 0:00:00.200199996, pts: none 41708 bytes, dts: 0:00:00.233566662, pts: none 41708 bytes, dts: 0:00:00.266933328, pts: none 41709 bytes, dts: 0:00:00.300299994, pts: none 86 bytes, dts: 0:00:00.333666660, pts: none gst-launch-1.0 filesrc location=/root/Raju/long_duration/mpegvideoparse.mpeg ! mpegvideoparse ! avdec_mpeg2video ! fakesink silent=false -v | grep chain | sed 's/^.* (\([0-9]*.*\), d.*$/\1/' 518400 bytes, dts: 0:00:00.000000000, pts: 0:00:00.000000000 518400 bytes, dts: 0:00:00.033366666, pts: 0:00:00.033366666 518400 bytes, dts: 0:00:00.066733332, pts: 0:00:00.066733332 518400 bytes, dts: 0:00:00.100099998, pts: 0:00:00.100099998 518400 bytes, dts: 0:00:00.133466664, pts: 0:00:00.133466664 518400 bytes, dts: 0:00:00.166833330, pts: 0:00:00.166833330 518400 bytes, dts: 0:00:00.200199996, pts: 0:00:00.200199996 518400 bytes, dts: 0:00:00.233566662, pts: 0:00:00.233566662 518400 bytes, dts: 0:00:00.233566662, pts: 0:00:00.233566662 518400 bytes, dts: 0:00:00.233566662, pts: 0:00:00.233566662
Created attachment 250359 [details] [review] mpegvideoparse: Extract timestamps from the MPEG GOP header Use the MPEG GOP header as the source of timestamps if possible Based on 0.10 patch by Thijs Vermeir <thijsvermeir@gmail.com>
Created attachment 250360 [details] [review] mpegvideoparse: Add option to override upstream timestamps with MPEG ones
*** Bug 705066 has been marked as a duplicate of this bug. ***
The approach of calculating frame PTS by multiplying TSN with frame duration will not work for telecine, where the frame duration is variable (also there are only 24 frames but the framerate is 29.97)
(In reply to comment #26) > The approach of calculating frame PTS by multiplying TSN with frame duration > will not work for telecine, where the frame duration is variable (also there > are only 24 frames but the framerate is 29.97) No, the framerate *IS* 29.97 interlaced AND it has 29.97 frames per second. The decoder will also output 29.97 interlaced, but with the proper TFF/RFF set, which will result in a 24000/1001 progressive output (if you put it through a deinterlacer). The whole point of telecine is to carry a progressive (film) stream in a interlaced (tele) format. It should be transparent to the parser, decoder and display.
You get 24 buffers. Some of them longer (depending on repeat_count), some regular. To know the duration of frame you need to parse it first.
Sorry, got confused with yet-another-painful-variant which has applied telecine.
What's the point of adding a property for this? The parser should do the right thing automatically, we can't expect people to get the parser from inside decodebin and set properties on it ;)
Otherwise this looks good I guess, except for the problem in comment 28.
(In reply to comment #30) > What's the point of adding a property for this? The problem is that we have some mutually incompatible use-cases 1. Upstream timestamps come from tsdemux/rtp/etc and have been clock-skew corrected, so we don't want to alter them. 2. Upstream timestamps come from udpsrc and need to be corrected (but we prefer ero-latency) 3. Upstream timestamps come from a demuxer, but are missing from some buffers.
(In reply to comment #31) > Otherwise this looks good I guess, except for the problem in comment 28. I really don't think tsn should generally be used to calculate timestamps. I attached a filter to #705066 that fills in missing timestamps, handles telecine correctly and only adds latency if you have missing timestamps (which is necessary, because you can't know durations of future frames).
So any ideas what we should do here now? This seems like we need more information somewhere to decide what to do. Maybe information in the segment event about how to interpret timestamps? Case 3. sounds like something that can be detected now already though. Missing timestamps should just be interpolated, no matter what.
Olivier? :)
I think this ties in to Edward's idea to not have jitterbuffers modify timestamps, but instead to push a downstream event so the sinks can adjust themselves. If we have this event, then we will know better the value of upstream timestamps. That would help us differentiate cases 1 and 2, but not detect case 3.
Marking as duplicate of the forward-clock issue *** This bug has been marked as a duplicate of bug 704331 ***