GNOME Bugzilla – Bug 762715
mpegtsmux: alignment property and timestamps for Live UDP streaming
Last modified: 2016-06-07 12:13:02 UTC
Hi, Related with https://bugzilla.gnome.org/show_bug.cgi?id=722129 with status "RESOLVED FIXED". The alignment property on mpegtsmux seems to have a bug, as it doesn't timestamp all output when combining multiple packets into one buffer. In Gstreamer 1.4.4 the udpsink plugin with sync=true works like a live source. It means to stream a 2 minutes video file it takes 2 minutes (like a live source) In Gstreamer 1.7.1 (also in 1.6.0) the udpsink plugin with sync=true works similar to sync=false. It means to stream a 2 minutes video file it takes 5 seconds (like a on demand source-best effort) I used the next pipelines: 1) Create a sample file gst-launch-1.0 -e videotestsrc ! video/x-raw, framerate=10/1, width=320, height=240, format=I420 ! timeoverlay font-desc="sans bold 40" halignment=0 valignment=2 ! x264enc bitrate=200 key-int-max=10 ! h264parse config-interval=1 ! mpegtsmux ! filesink location=test.ts 2) Stream the file ("alignment=7" for UDP streaming) gst-launch-1.0 filesrc location=test.ts ! tsparse ! tsdemux ! h264parse ! mpegtsmux alignment=7 ! udpsink port=5014 host=127.0.0.1 sync=true enable-last-sample=false send-duplicates=false 3) Play the Stream gst-launch-1.0 udpsrc uri=udp://127.0.0.1:5014 ! video/mpegts ! tsdemux name=demux ! video/x-h264 ! queue ! decodebin ! autovideosink When using ("alignment=1") it works as a live source <- wanted behavior gst-launch-1.0 filesrc location=test.ts ! tsparse ! tsdemux ! h264parse ! mpegtsmux alignment=1 ! udpsink port=5014 host=127.0.0.1 sync=true enable-last-sample=false send-duplicates=false When using rndbuffersize workaround (it worked in 1.4.4 but in 1.7.1 it does not work) gst-launch-1.0 filesrc location=test.ts ! tsparse ! tsdemux ! h264parse ! mpegtsmux alignment=7 ! rndbuffersize min=1316 max=1316 ! udpsink port=5014 host=127.0.0.1 sync=true enable-last-sample=false send-duplicates=false Best, Angel
I have tested with wireshark that the alignment property has effect in the UDP packet size. This way, alignment=7 produces 1358 UDP packet size. This is as expected, but the sync problem is related to timestamps when combining multiple packets into one buffer.
This is the evidence. This produces buffers of 188 bytes with PTS gst-launch-1.0 videotestsrc ! x264enc ! mpegtsmux alignment=1 ! fakesink silent=false sync=true -v /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (188 bytes, dts: none, pts: 0:00:02.266666666, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f7bc0ba8690 This produces buffers of 1316 bytes without PTS gst-launch-1.0 videotestsrc ! x264enc ! mpegtsmux alignment=7 ! fakesink silent=false sync=true -v /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (1316 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00000000 ) 0x7f357cb0e950
Created attachment 322455 [details] [review] Fix timestamps when alignment parameter is set With this fix the mpegtsmux plugin adds timestamp (the same way 1.4.4 did) to all output when combining multiple packets into one buffer
Review of attachment 322455 [details] [review]: ::: /gst-plugins-bad/gst/mpegtsmux/mpegtsmux.c @@ +1526,3 @@ + buf = gst_adapter_take_buffer (mux->out_adapter, align); + GST_BUFFER_PTS (buf) = ts; + gst_buffer_list_add (buffer_list, buf); Can't you use gst_adapter_prev_pts_at_offset() ? Also, this is encoded data, shouldn't you first look for a DTS and if none then pick the PTS ? Specially in your example, you have a B-Frame enabled H264 stream, so PTS is not monotonic, hence not suited for synchronization.
Also, unit tests should be enhanced to cover this imho.
Created attachment 322640 [details] [review] Fix timestamps when alignment parameter is set v2 I have followed your advice and created a patch looking first for DTS and taking into account the offset suggestion.
Review of attachment 322640 [details] [review]: Looks like the way forward, though I'd like to see a unit test to go with it. Also, are you sure the timestamp code is right? gst_adapter_take_buffer() will flush the buffers, possibly removing the timestamp you where looking for. I would have expected the timestamp to be read before flushing the data form the adapter. As this shrink the adapter, I would expected a fixed offset from start (align or align - 1, not sure if prev is exclusive). ::: /a/gst/mpegtsmux/mpegtsmux.c @@ +1524,3 @@ + /* first sync on DTS, else use PTS */ + ts = gst_adapter_prev_dts_at_offset (mux->out_adapter, offset, NULL); + if (!GST_CLOCK_TIME_IS_VALID (ts)) { This code is correct, but as I got confused reading it, maybe it's better to avoid the not (!) here and flip and cases ? @@ +1528,3 @@ + GST_BUFFER_PTS (buf) = ts; + } + else { Wrong style, should be "} else {" for GStreamer code. Do you have indent installed properly ? We normally have commit hooks that catches these.
Hi, Using Comment #2 as unit test shows the following results: gst-launch-1.0 videotestsrc ! x264enc ! mpegtsmux alignment=7 ! fakesink silent=false sync=true -v Likewise, without patch we obtain; /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (1316 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00000000 ) 0x7f2d3cb0c910 And with patch: /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (1316 bytes, dts: none, pts: 0:00:04.200000000, duration: none, offset: -1, offset_end: -1, flags: 00000000 ) 0x7fb030b0e840 is the result. Another test can be performed, trying the initial description test with the sample file number #1 and streaming it with stream pipeline #2. When the plugin is patched, live stream is performed ( sync = true ). However, without the patch an EOS is received instantaneously, which proves that the property "sync = true " does not work correctly, as it worked in version 1.4.4 Finally, just a note about the code comments: The DTS/PTS order is exactly the same as gstbasesink.c does. With respect to the second paragraph, I did not realize until now about that rule in gstreamer. Well, I think that's all. Thanks for your help. It was of great value for me. Best Regards Josu.
Let me rephrase, by unit test, I mean a peace of software that validate the behaviour, and is ran by "make check".
See also https://bugzilla.gnome.org/show_bug.cgi?id=765926 which has an alternative patch and the same amount of lack of unit test.
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but we are happy to tell you that the problem has already been fixed in the code repository. *** This bug has been marked as a duplicate of bug 765926 ***