GNOME Bugzilla – Bug 767900
multipartmux is not clearing dts timestamp.
Last modified: 2016-07-08 13:59:48 UTC
Created attachment 330116 [details] [review] gst-plugins-good patch The multipartmux is just handling TIMESTAMP ie PTS_TIMESTAMP and not clearing the DTS timestamp. We have a pipeline source->multipartmux->sink. The source is producing both PTS and DTS timestamp normally. But after seek it produce rather many packets that are to early. The multipartmux is setting the PTS_TIMESTAMP(BUFFER_TIMESTAMP)on this early packets to GST_CLOCK_TIME_NONE and leave the DTS_TIMESTAMP as they are. The sink will now see a stream that seams to have no PTS_TIMESTAMP but have DTS_TIMESTAMP and treys to sync on this DTS_TIMESTAMP. Solution: Let the multipartmux set the DTS_TIMESTAMP to GST_CLOCK_TIME_NONE.
Review of attachment 330116 [details] [review]: Makes sense, but OTOH we might want to pass through DTS together with the PTS? ::: gst/multipart/multipartmux.c @@ +506,2 @@ GST_BUFFER_DURATION (headerbuf) = 0; GST_BUFFER_OFFSET (headerbuf) = mux->offset; Why don't yet set DTS here @@ +543,3 @@ /* the footer has the same timestamp as the data buffer and has a * duration of 0 */ + GST_BUFFER_PTS (footerbuf) = best->timestamp; and here?
Created attachment 330120 [details] [review] gst-plugins-good patch New patch.
Review of attachment 330120 [details] [review]: ::: gst/multipart/multipartmux.c @@ +504,3 @@ * below) and has a duration of 0 */ + GST_BUFFER_PTS (headerbuf) = best->timestamp; + GST_BUFFER_DTS (headerbuf) = best->timestamp; I meant passing through PTS and DTS independently, not setting them to the same value (which is generally wrong). Just implement the same logic as for the PTS (->timestamp) also for DTS :)
Created attachment 330132 [details] Logfile with probe1 and prob2 . Probe1 before multipartmux and probe 2 after Attach logfile with probes.
Created attachment 330189 [details] [review] patch for gst-plugins-good New patch
Review of attachment 330189 [details] [review]: Generally looks like going in the right direction, thanks :) ::: gst/multipart/multipartmux.c @@ +318,2 @@ /* no timestamp on new buffer, it must go first */ + newtime = new->pts_timestamp; All this should probably use the DTS of both, and only if there is not DTS on both fall back to PTS. Should also use the running time, not the buffer timestamp @@ +357,3 @@ + /* Store timestamps with segment_start and preroll */ + if (buf && GST_BUFFER_PTS_IS_VALID (buf) && GST_BUFFER_DTS_IS_VALID (buf)) { Set PTS if PTS is valid, DTS if DTS is valid. No need to only set both if both are valid
You wrote: "All this should probably use the DTS of both, and only if there is not DTS on both fall back to PTS. Should also use the running time, not the buffer timestamp" Comment: Both pts_timestamp and dts_timestamp are converted to running time. line 362 pad->pts_timestamp = gst_segment_to_running_time (&data->segment, GST_FORMAT_TIME, GST_BUFFER_PTS (buf)); pad->dts_timestamp = gst_segment_to_running_time (&data->segment, GST_FORMAT_TIME, GST_BUFFER_DTS (buf)); I will fix the rest in a new patch.
Created attachment 330239 [details] [review] gst-plugins-good patch Add a new patch.
Review of attachment 330239 [details] [review]: ::: gst/multipart/multipartmux.c @@ +312,3 @@ + if (old->dts_timestamp != GST_CLOCK_TIME_NONE && + new->dts_timestamp != GST_CLOCK_TIME_NONE) { Basically this comparison here should do something like GST_BUFFER_DTS_OR_PTS(), that is using the one that is not NONE @@ +316,3 @@ + /* no timestamp on old buffer, it must go first */ + oldtime = old->dts_timestamp; + if (oldtime == GST_CLOCK_TIME_NONE) This does not make sense: above you check that it is != NONE @@ +496,3 @@ + if (best->pts_timestamp != -1) + time = best->pts_timestamp; Should this maybe be time = dts != NONE ? dts : (pts != NONE ? pts : 0)
Hi, I am filling in for Göran while he is on vacation. Could you please elaborate on the expression: "time = dts != NONE ? dts : (pts != NONE ? pts : 0)" I am not sure that I understand what it evaluates to and I am not sure why time should be set to dts or pts. Cheers!
It evaluates to DTS if set, otherwise to PTS if set, otherwise to 0. That seems to be the intention of the code, to have some kind of time available.
Created attachment 331078 [details] [review] multipartmux: Use PTS and DTS instead of timestamp New patch uploaded based on Görans changes. 0001-multipartmux-Use-PTS-and-DTS-instead-of-timestamp.patch Cheers!
Attachment 331078 [details] pushed as 6fe88d8 - multipartmux: Use PTS and DTS instead of timestamp