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 698748 - mpegtsmux: improper timestamping of output packets
mpegtsmux: improper timestamping of output packets
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.x
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 720724 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-04-24 15:08 UTC by Greg Rutz
Modified: 2014-01-08 09:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test stream that exhibits the problem (1.30 MB, text/vnd.trolltech.linguist)
2013-04-24 15:29 UTC, Greg Rutz
  Details
Proposed fix (1.03 KB, patch)
2013-04-24 15:43 UTC, Greg Rutz
none Details | Review
mpegtsmux: Don't disrupt buffer state in the clip function (4.49 KB, patch)
2013-12-19 12:04 UTC, Jan Schmidt
committed Details | Review

Description Greg Rutz 2013-04-24 15:08:54 UTC
Using the following gst-launch (and the attached stream), mpegtsmux is timestamping every output buffer with a value of 0.  I am using multifilesink to segment the resulting transport stream by key-frames.  multifilesink looks for key-frames every 10 seconds to know when to start a new file, but if the buffer timestamps are not increasing, a new file will never be opened.

gst-launch-1.0 -e -v --gst-debug-level=3 --gst-debug=multifilesink:5 \
    filesrc location=test2.ts ! \
    tsdemux name=demux  \
        demux.video_0840 ! mpeg2dec ! queue ! tee name=videotee \
        demux.audio_0841 ! tee name=audiotee1 \
        demux.audio_0842 ! tee name=audiotee2 \
    mpegtsmux name=mux_320x240 ! \
        multifilesink max-files=100 index=1 next-file=2 \
            location=test_320x240_200kb_%06d.ts \
    mpegtsmux name=mux_640x480 ! \
        multifilesink max-files=100 index=1 next-file=2 \
            location=test_640x480_1000kb_%06d.ts \
    videotee. ! queue ! \
        videoscale ! video/x-raw, width=320, height=240 ! queue ! \
        avenc_mpeg2video bitrate=200000 ! mux_320x240. \
    videotee. ! queue ! \
        videoscale ! video/x-raw, width=640, height=480 ! queue ! \
        avenc_mpeg2video bitrate=1000000 ! mux_640x480. \
    audiotee1. ! queue ! mux_320x240. \
    audiotee2. ! queue ! mux_320x240. \
    audiotee1. ! queue ! mux_640x480. \
    audiotee2. ! queue ! mux_640x480.

It is possible that a simpler pipeline will also produce this result, but this is one that does it for me.  I have identified the code in mpegtsmux that is causing the problem (will post in a future comment), but I'm not sure if the problem is with mpegtsmux or something further upstream in the pipeline.
Comment 1 Greg Rutz 2013-04-24 15:14:46 UTC
In mpegtsmux_clip_inc_running_time(), the last dts and pts of incoming packets are saved to the MpegTsPadData.  In the case of my test stream, the first buffer to come through the video sink pad always have DTS = PTS = 0.  This sets pad_data->last_dts = pad_data->last_pts = 0.

After that first video buffer, all subsequent buffers only have valid DTS.  Finally, in mpegtsmux_collected_buffer, the timestamping of the outgoing buffer is applied by this code:

  /* outgoing ts follows ts of PCR program stream */
  if (prog->pcr_stream == best->stream) {
    /* prefer DTS if present for PCR as it should be monotone */
    mux->last_ts =
        GST_CLOCK_TIME_IS_VALID (best->last_dts) ? best->last_dts : best->
        last_pts;
  }

Since DTS is preferred and since the last dts is valid (its 0, not -1), every buffer gets timestamped with the value 0.
Comment 2 Greg Rutz 2013-04-24 15:16:14 UTC
I should also mention that this happens on 1.x only.  Does not happen (with the same test stream) on 1.0
Comment 3 Greg Rutz 2013-04-24 15:29:10 UTC
Created attachment 242334 [details]
Test stream that exhibits the problem

My original test stream was too big to attach.  For this test stream, the pids have changed so you need to change the demux pad names to:

demux.video_0800
demux.audio_0801
demux.audio_0802
Comment 4 Greg Rutz 2013-04-24 15:43:54 UTC
Created attachment 242339 [details] [review]
Proposed fix

I don't have high confidence that this is the correct fix, I just wanted to put something out there that works for me.
Comment 5 Edward Hervey 2013-08-21 09:01:25 UTC
Having only DTS set (and not PTS) seems like a bug, no ?

You either have only PTS set (and then DTS == PTS)
Or you have PTS and DTS set (and then they have their respective values)

What produces only DTS ?
Comment 6 Greg Rutz 2013-08-21 13:59:41 UTC
I'll try to dig into avenc_mpeg2video to identify why its output buffers have only DTS.
Comment 7 Edward Hervey 2013-08-21 14:13:31 UTC
FWIW, I tried videotestsrc ! avenc_mpeg2video  and it produces both PTS and DTS. Tried latest master ?

I'll add a warning in the meantime if incoming data has DTS without PTS
Comment 8 Greg Rutz 2013-08-21 14:15:14 UTC
I haven't tried latest master.  I will do that as well.  I have been using my fix in our fork of plugins-bad.
Comment 9 RajuB 2013-12-18 10:32:22 UTC
I am using gstreamer 1.2.1, this issue is seen in the latest version. The above given patch resolves this. Can anybody help me understand this issue. What I found is best->last_pts and best->last_dts initial values sent to mpegtsmux_collected_buffer function are set to 0, and GST_CLOCK_TIME_IS_VALID infers it as valid time. This is leading to zero timestamps. Am I right here??

You can reproduce this issue with below given pipelines:

This produces zero timestamps:
gst-launch-1.0 filesrc location=<file path> ! tee ! queue ! decodebin name=demux ! deinterlace mode=0 fields=1 method=4 tff=0 ! videoconvert ! tee ! queue ! videoscale ! video/x-raw ! x264enc ! mpegtsmux name=mux ! checksumsink demux.! tee ! queue ! audioconvert ! audioresample ! volume ! voaacenc ! mux.

This produces proper timestamps:
gst-launch-1.0 filesrc location=<file path> ! tee ! queue ! decodebin ! deinterlace mode=0 fields=1 method=4 tff=0 ! videoconvert ! tee ! queue ! videoscale ! video/x-raw ! x264enc ! mpegtsmux ! checksumsink

This produces zero timestamps: 
gst-launch-1.0 filesrc location=<file path> ! tee ! queue ! decodebin ! audioconvert ! audioresample ! volume ! voaacenc ! mpegtsmux ! checksumsink

This produces proper timestamps: 
gst-launch-1.0 filesrc location=<file path> ! tee ! queue ! decodebin ! audioconvert ! audioresample ! volume ! voaacenc ! checksumsink

I tried with other muxers (like mp4mux, flvmux), where I didn't see this issue. Please let me know if you require more details.
Comment 10 Jan Schmidt 2013-12-18 11:56:23 UTC
The larger problem here seems to be that DTS is always 0, and not incrementing as it should. That'd be a bug in the encoder afaics.
Comment 11 RajuB 2013-12-18 14:38:25 UTC
(In reply to comment #10)
> The larger problem here seems to be that DTS is always 0, and not incrementing
> as it should. That'd be a bug in the encoder afaics.

I tapped both audio and video PTS/DTS values before muxing and I am getting Proper PTS/DTS values from Video encode pipeline. From audio pipeline I am getting proper PTS values but DTS values are 99:99.999999999. I don't see 0 time timestamps anywhere. Could you please given me the pipeline you used and if possible the input file used. Thank you for quick response.
Comment 12 Jan Schmidt 2013-12-19 00:27:45 UTC
I was just going off what's been reported in the bug. Actually testing, with git master, I can't see the problem:

gst-launch-1.0 audiotestsrc ! audioconvert ! audioresample ! volume ! voaacenc ! mpegtsmux ! fakesink silent=false -v |less

produces a stream with incrementing timestamps.

gst-launch-1.0 audiotestsrc ! audioconvert ! audioresample ! volume ! voaacenc ! fakesink silent=false -v |less

shows voaacenc putting out a nicely timestamped stream with pts == dts.
Comment 13 Greg Rutz 2013-12-19 03:59:49 UTC
Either use my original pipeline given in the bug description (with the attached content) or try a pipeline that uses avenc_mpeg2 and mpegtsmux.  I'm pretty sure the problem is either in avenc_mpeg2 or in mpegtsmux when muxing video encoded with avenc_mpeg2.
Comment 14 Jan Schmidt 2013-12-19 06:46:38 UTC
Using either your supplied test file or videotestsrc, the only thing I really see wrong with avenc_mpeg2video is that all output buffers have PTS == DTS, which isn't right. Otherwise though, all buffers do have timestamps, and mpegtsmux produces a stream with incrementing output timestamps.

I get the same behaviour with both 1.2.1 and git master.

gst-launch-1.0 filesrc location=./bug-698748.ts ! tsdemux ! mpeg2dec ! queue ! avenc_mpeg2video ! fakesink silent=false -v 2>&1 |less
  -> all buffers have incrementing output timestamps

gst-launch-1.0 filesrc location=./bug-698748.ts ! tsdemux ! mpeg2dec ! queue ! avenc_mpeg2video ! mpegtsmux ! fakesink silent=false -v 2>&1 |less
  -> all buffers have incrementing output timestamps

This however, does generate 0 output timestamps:

gst-launch-1.0 -e -v filesrc location=./bug-698748.ts !     tsdemux name=demux  demux. ! audio/x-ac3 ! tee name=audiotee1         demux. ! audio/x-ac3 ! tee name=audiotee2     mpegtsmux name=mux_320x240 ! fakesink silent=false  audiotee1. ! queue ! mux_320x240.     audiotee2. ! queue ! mux_320x240.  2>&1 |less

Eventually, I tracked it to this collectpads commit in core:

commit f671bd27548f2e5f7db0e475aeb450d794ba697d
Author: David Schleef <ds@schleef.org>
Date:   Fri Feb 22 14:56:49 2013 -0800

    collectpads: take DTS into account
    
    Importantly, this patch converts DTS to running time.  Less importantly,
    and possibly a problem for some muxers, is that it orders buffers by
    DTS (if it is valid, otherwise PTS).  This is generally correct, but
    might be somewhat surprising to muxers.
    
    Also note that once converted to running time, DTS can end up negative.

That commit, for some reason, calls the clip function, passing a buffer with PTS = DTS = segment start time, thus confusing and disrupting mpegtsmux's timestamp tracking when none of the incoming buffers have a DTS.
Comment 15 Jan Schmidt 2013-12-19 11:12:21 UTC
*** Bug 720724 has been marked as a duplicate of this bug. ***
Comment 16 RajuB 2013-12-19 11:22:58 UTC
(In reply to comment #15)
> *** Bug 720724 has been marked as a duplicate of this bug. ***

Here I have given links to detailed logs of different test-cases, which might be helpful for you to analyze.
Comment 17 Jan Schmidt 2013-12-19 12:04:28 UTC
Created attachment 264533 [details] [review]
mpegtsmux: Don't disrupt buffer state in the clip function

Collectpads assumes that it can pass any buffer to the clip function
for adjustment, some of which are artificially injected - so don't
adjust global timestamp tracking there. Instead, only adjust the
buffer timestamps and use them directly in the collection function.

Fixes
Comment 18 RajuB 2013-12-20 05:19:44 UTC
(In reply to comment #17)
> Created an attachment (id=264533) [details] [review]
> mpegtsmux: Don't disrupt buffer state in the clip function
> 
> Collectpads assumes that it can pass any buffer to the clip function
> for adjustment, some of which are artificially injected - so don't
> adjust global timestamp tracking there. Instead, only adjust the
> buffer timestamps and use them directly in the collection function.
> 
> Fixes


Thank you Jan, Can I apply this patch 1.2.1 version.??
Comment 19 Jan Schmidt 2013-12-20 05:46:52 UTC
It should apply cleanly to 1.2.1, yes.
Comment 20 Jan Schmidt 2013-12-31 12:26:06 UTC
Pushed to master, since it seems to work in my tests. Please re-open if you see further trouble.
Comment 21 RajuB 2014-01-08 05:07:58 UTC
Thank you Jan, I will post here if I find further issue. Good day.
Comment 22 Tim-Philipp Müller 2014-01-08 09:49:17 UTC
commit 14a56b6964b87e66bc2e242ace03ac385bfe1e12
Author: Jan Schmidt <jan@centricular.com>
Date:   Thu Dec 19 23:00:12 2013 +1100

    mpegtsmux: Don't disrupt buffer state in the clip function
    
    Collectpads assumes that it can pass any buffer to the clip function
    for adjustment, some of which are artificially injected - so don't
    adjust global timestamp tracking there. Instead, only adjust the
    buffer timestamps and use them directly in the collection function.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=698748