GNOME Bugzilla – Bug 757961
baseparse: do not overwrite header buffer timestamps
Last modified: 2018-11-03 15:05:44 UTC
Test cases: $ wget --user="pitivisamples@ecchi.ca" --password="pitivisamples" ftp://ecchi.ca/00159.MTS $ gst-launch-1.0 uridecodebin uri=file:///$PWD/00159.MTS name=d ! \ queue ! videoconvert ! videoscale ! videoconvert ! videorate ! \ jpegenc quality=100 ! matroskamux name=m ! queue ! filesink \ location=/tmp/test.bad.mkv d. ! queue ! audioconvert ! \ flacenc ! flacparse ! m. now: $ gst-launch-1.0 playbin uri=file:///tmp/test.bad.mkv -> Fails to preroll $ gst-launch-1.0 uridecodebin uri=file:///$PWD/00159.MTS name=d ! \ queue ! videoconvert ! videoscale ! videoconvert ! videorate ! \ jpegenc quality=100 ! matroskamux name=m ! queue ! filesink \ location=/tmp/test.good.no.flacparse.mkv d. ! queue ! audioconvert ! \ flacenc ! m. then: $ gst-launch-1.0 playbin uri=file:///tmp/test.good.no.flacparse.mkv Works and: $ gst-launch-1.0 uridecodebin uri=file:///$PWD/00159.MTS name=d ! \ queue ! videoconvert ! videoscale ! videoconvert ! videorate ! \ jpegenc quality=85 ! matroskamux name=m ! queue ! filesink \ location=/tmp/test.good.quality85.mkv d. ! queue ! audioconvert ! \ flacenc ! flacparse ! m. Then: $ gst-launch-1.0 playbin uri=file:///tmp/test.good.quality85.mkv works.
Not sure why but with flacparse it seems that the file gets poorly interleaved and it stalls on prerolling because video queues get full.
flacparse probably breaks timestamps somehow
Created attachment 315440 [details] [review] collectpads: handle buffer with dts-only when mapping to running time flacparse does some bad timestamping by putting dts on a header buffer and then collect pads does not help by using the times on those values wrongly. This fixes the issue but we also need to fix flacparse.
The bug in flacparse is actually in baseparse. When it receives a segment it sets the 'next_dts' to segment.start. Then it overwrites the first buffers DTS with this value. As the first few buffers are headers they don't have pts and dts but get the dts set from the base class, leading to an unusual scenario.
Created attachment 315441 [details] [review] baseparse: do not overwrite header buffer timestamps I remember there was a bug about baseparse retimestamping behavior but I coudln't find it. Anyway, this is a fix specific to this scenario of header buffers timestamping
commit 42d45a0f4031478d1043f8b096f4cedba4fd562f Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Nov 13 20:44:57 2015 -0300 collectpads: handle buffer with dts-only when mapping to running time Otherwise the buffer was left with the original values and later would be compared with other buffers that were converted to runninn time, leading to bad interleaving of multiple streams. https://bugzilla.gnome.org/show_bug.cgi?id=757961 commit 971ac61c36b1405ce57cecdf8dceaf525fe9b908 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Nov 13 16:31:06 2015 -0300 baseparse: do not overwrite header buffer timestamps baseparse tries to preserve timestamps from upstream if it is running on a time segment and write that to output buffers. It assumes the first DTS is going to be segment.start and sets that to the first buffers. In case the buffer is a header buffer, it had no timestamps and will have only the DTS set due to this mechanism. This patch prevents this by skipping this behavior for header buffers. https://bugzilla.gnome.org/show_bug.cgi?id=757961
Review of attachment 315440 [details] [review]: ::: libs/gst/base/gstcollectpads.c @@ +518,3 @@ + if (GST_CLOCK_TIME_IS_VALID (time)) { + time = + gst_segment_to_running_time (&cdata->segment, GST_FORMAT_TIME, time); DTS_OR_PTS returns DTS first. If you clip on DTS you are doing it wrong, and breaking H264 stream with B-Frames again.
Review of attachment 315440 [details] [review]: ::: libs/gst/base/gstcollectpads.c @@ +518,3 @@ + if (GST_CLOCK_TIME_IS_VALID (time)) { + time = + gst_segment_to_running_time (&cdata->segment, GST_FORMAT_TIME, time); Ok, nevermind, I miss-read.
Thiago, I think this should also go into 1.6.2 :)
Thanks for that thiagos :)
Merged to 1.6 commit 288280fb0cea8ecf4f2b170f790b9c814925d017 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Nov 13 20:44:57 2015 -0300 collectpads: handle buffer with dts-only when mapping to running time Otherwise the buffer was left with the original values and later would be compared with other buffers that were converted to runninn time, leading to bad interleaving of multiple streams. https://bugzilla.gnome.org/show_bug.cgi?id=757961 commit 2c475a035543efc0202ecdc52070295a421ed4b4 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Nov 13 16:31:06 2015 -0300 baseparse: do not overwrite header buffer timestamps baseparse tries to preserve timestamps from upstream if it is running on a time segment and write that to output buffers. It assumes the first DTS is going to be segment.start and sets that to the first buffers. In case the buffer is a header buffer, it had no timestamps and will have only the DTS set due to this mechanism. This patch prevents this by skipping this behavior for header buffers. https://bugzilla.gnome.org/show_bug.cgi?id=757961
this patch seems to break my app, please take a look to these logs 0:00:00.157222961 15683 0x277a400 LOG baseparse gstbaseparse.c:1994:gst_base_parse_prepare_frame:<video_muxer_parser> preparing frame at offset 18446744073709551615 (0xffffffffffffffff) of size 16981 0:00:00.157255415 15683 0x277a400 DEBUG baseparse gstbaseparse.c:789:gst_base_parse_update_frame:<video_muxer_parser> marking DISCONT 0:00:00.157276147 15683 0x277a400 LOG baseparse gstbaseparse.c:794:gst_base_parse_update_frame:<video_muxer_parser> marking as new frame 0:00:00.157417421 15683 0x277a400 DEBUG baseparse gstbaseparse.c:3710:gst_base_parse_set_frame_rate:<video_muxer_parser> invalid fps (0/1), ignoring parameters 0:00:00.157452776 15683 0x277a400 LOG baseparse gstbaseparse.c:3732:gst_base_parse_set_frame_rate:<video_muxer_parser> set fps: 0/0 => duration: 18446744073709 ms 0:00:00.157476917 15683 0x277a400 LOG baseparse gstbaseparse.c:3736:gst_base_parse_set_frame_rate:<video_muxer_parser> set lead in: 0 frames = 0 ms, lead out: 0 frames = 0 ms 0:00:00.157585896 15683 0x277a400 LOG baseparse gstbaseparse.c:2505:gst_base_parse_finish_frame:<video_muxer_parser> finished frame at offset 0, flushing size 16981 0:00:00.157649401 15683 0x277a400 DEBUG baseparse gstbaseparse.c:1871:gst_base_parse_check_seekability:<video_muxer_parser> seeking query failed 0:00:00.157675616 15683 0x277a400 DEBUG baseparse gstbaseparse.c:1912:gst_base_parse_check_seekability:<video_muxer_parser> seekable: 0 (18446744073709551615 - 18446744073709551615) 0:00:00.157706211 15683 0x277a400 DEBUG baseparse gstbaseparse.c:1916:gst_base_parse_check_seekability:<video_muxer_parser> idx_interval: 0ms 0:00:00.157748558 15683 0x277a400 DEBUG baseparse gstbaseparse.c:1935:gst_base_parse_check_upstream:<video_muxer_parser> upstream_has_duration: 0 0:00:00.157778256 15683 0x277a400 DEBUG baseparse gstbaseparse.c:2211:gst_base_parse_handle_and_push_frame:<video_muxer_parser> no next fallback timestamp 0:00:00.157803564 15683 0x277a400 LOG baseparse gstbaseparse.c:2270:gst_base_parse_push_frame:<video_muxer_parser> processing buffer of size 16981 with dts 99:99:99.999999999, pts 99:99:99.999999999, duration 99:99:99.999999999 0:00:00.157861822 15683 0x277a400 DEBUG baseparse gstbaseparse.c:1958:gst_base_parse_check_media:<video_muxer_parser> media is video: 1 as you can see there is a frame of size 16981 (first keyframe) that enter with a valid pts and is pushed with invalid timestamp. In my app this frame is passed to matroskamux that drop it since has no pts/dts set and so the resulting video miss the first keyframe. I'll do some more investigations later today and try to reproduce with gst-launch
This should probably be reverted in 1.6 given all the problems, unless we find a better fix.
please download the file here: http://195.250.34.59/temp/2015_11_17_09_52_03_432.mkv and try this pipeline: gst-launch-1.0 -v filesrc location=/tmp/2015_11_17_09_52_03_432.mkv ! matroskademux ! h264parse ! fakesink silent=false you have something like this for the first frame: /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (16981 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000440 discont header ) 0x7f6084013b30 if you revert the changes you get this: /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (16981 bytes, dts: 24:43:55.673000000, pts: 24:43:55.673000000, duration: none, offset: 0, offset_end: -1, flags: 00000440 discont header ) 0x7fdb58013aa0 please fix, thanks Nicola
the first keyframe seems to have the header flag
yes, h264parse seems to mark a lot of things as headers. We've been discussing if it is possible to fix the original bug in some other way.
The original bug: 1) flacparse receives a header buffer with dts=none, pts=none 2) baseparse has next-dts and next-pts variables that it gets from the incoming buffers and tries to preserve them on the output. next-dts is initialized as segment.start. As the first buffer has no dts, the next-dts is kept as segment.start 3) the output from flacparse (the header buffer) has its dts set to segment.start and is pushed This was confusing collectpads interleaving (now fixed) The patch was to ignore this re-timestamping done by baseparse on the hope that header buffers would keep their timestamp=None from upstream. But this introduced a regression because h264parse uses the header flag on buffers that have timestamps (SPS/PPS/SEI) and the new behavior of skipping retimestamping of header buffers breaks the whole stream timestamping. I agree with Tim that we should revert this for 1.6 and keep only the collectpads one that also fixes the issue with flacparse. For master I'm unsure of what to do until now. I think the original behavior of having header buffers with dts=segment.start or next-dts is better than the current one, considering what it made h264parse do. We should also revisit h264parse header flag usage to make sure it isn't marking too much as headers.
Ok so these two commits should be reverted http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?h=1.6&id=2c475a035543efc0202ecdc52070295a421ed4b4 http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?h=1.6&id=921816400bf3ad65f8978a50569a7d87ef05c806 I can confirm that my the test file I provided works as expected in this way. Can you please push to 1.6? thanks
Reverted in master and 1.6. The original bug is still fixed in collectpads so the use case works. We can now try again to figure out how to sanely fix this in baseparse, h264parse and flacparse at the same time.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/236.