After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 757961 - baseparse: do not overwrite header buffer timestamps
baseparse: do not overwrite header buffer timestamps
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-11 19:06 UTC by Thibault Saunier
Modified: 2018-11-03 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
collectpads: handle buffer with dts-only when mapping to running time (4.25 KB, patch)
2015-11-13 23:50 UTC, Thiago Sousa Santos
needs-work Details | Review
baseparse: do not overwrite header buffer timestamps (2.86 KB, patch)
2015-11-14 01:04 UTC, Thiago Sousa Santos
needs-work Details | Review

Description Thibault Saunier 2015-11-11 19:06:11 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.
Comment 1 Thiago Sousa Santos 2015-11-13 16:01:32 UTC
Not sure why but with flacparse it seems that the file gets poorly interleaved and it stalls on prerolling because video queues get full.
Comment 2 Sebastian Dröge (slomo) 2015-11-13 16:30:55 UTC
flacparse probably breaks timestamps somehow
Comment 3 Thiago Sousa Santos 2015-11-13 23:50:46 UTC
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.
Comment 4 Thiago Sousa Santos 2015-11-14 00:05:23 UTC
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.
Comment 5 Thiago Sousa Santos 2015-11-14 01:04:59 UTC
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
Comment 6 Thiago Sousa Santos 2015-11-14 15:25:09 UTC
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
Comment 7 Nicolas Dufresne (ndufresne) 2015-11-14 16:27:56 UTC
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.
Comment 8 Nicolas Dufresne (ndufresne) 2015-11-14 16:35:08 UTC
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.
Comment 9 Sebastian Dröge (slomo) 2015-11-14 17:30:20 UTC
Thiago, I think this should also go into 1.6.2 :)
Comment 10 Thibault Saunier 2015-11-14 22:33:03 UTC
Thanks for that thiagos :)
Comment 11 Thiago Sousa Santos 2015-11-16 12:01:34 UTC
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
Comment 12 Nicola 2015-11-17 10:33:35 UTC
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
Comment 13 Tim-Philipp Müller 2015-11-17 11:15:28 UTC
This should probably be reverted in 1.6 given all the problems, unless we find a better fix.
Comment 14 Nicola 2015-11-17 11:16:47 UTC
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
Comment 15 Nicola 2015-11-17 11:19:52 UTC
the first keyframe seems to have the header flag
Comment 16 Thiago Sousa Santos 2015-11-17 12:23:05 UTC
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.
Comment 17 Thiago Sousa Santos 2015-11-17 15:16:18 UTC
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.
Comment 18 Nicola 2015-11-17 15:47:12 UTC
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
Comment 19 Thiago Sousa Santos 2015-11-19 04:15:01 UTC
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.
Comment 20 GStreamer system administrator 2018-11-03 15:05:44 UTC
-- 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.