GNOME Bugzilla – Bug 784002
rtpbin: ntp-sync temporarily causes old timestamps
Last modified: 2018-02-15 08:06:15 UTC
When enabling ntp-sync on the rtpbin the sender report is used to adjust the timestamps. When this happens a 'large' time offset might be applied at once to adjust the timestamps. The result can be that the rtpbin produces buffers with timestamps that are older then previously produced buffers.
Created attachment 354104 [details] [review] Make sure PTS is always increasing Suggested solution that ensures that the PTS of buffers are always increasing.
Fixing the timestamps like in the first patch does not solve the problem good enough. We don't get old timestamps any more but the video can still be jerky when applying a large offset at once. I will instead try a solution where changes to ts-offset are applied gradually frame by frame.
Created attachment 354614 [details] [review] Adds new property, ts_offset_rate, to control the change rate of ts_offset Added a new property ts_offset_rate that lets the application control the rate of change of ts_offset. This property can be used to solve both the issue with old timestamps as well as solving the issue with visible effects when applying large offsets at once.
Review of attachment 354614 [details] [review]: Generally looks good to me With old timestamps, you mean timestamps going backwards? That should be prevented in any case in rtpjitterbuffer and would be a bug that must be fixed. There is code already to prevent that in the calculations for the actual PTS (before ts-offset adjustment), but it would make sense one layer above additionally too ::: gst/rtpmanager/gstrtpbin.c @@ +2497,3 @@ + * GstRtpBin:ts-offset-rate + * + * Syncing time stamps to either NTP or RFC7273 clocks adds a time offset. RFC7273 does not go via ts-offset though. The offset comes from the SDP (or otherwise via caps) at the beginning and does not change. What can change however is the underlying network clock synchronization, but that would not be affected here. @@ +2505,3 @@ + g_param_spec_uint64 ("ts-offset-rate", "Timestamp Offset Rate", + "The rate in nanoseconds per frame to apply adjustments to " + "time stamps (0 = apply all at once) ", 0, G_MAXUINT64, This is not really a rate as implemented, but a maximum step size per "group of packets with same RTP timestamp". Maybe should be called "max-ts-offset-adjustment"? ::: gst/rtpmanager/gstrtpjitterbuffer.c @@ +3391,3 @@ update_estimated_eos (jitterbuffer, item); + priv->last_pts = pts; This one here could also be used for detecting backwards timestamps caused by the ts-offset
Yes, I meant that the timestamps are going backwards. But I have only seen this as a direct result of applying a to large ts-offset. I will update the property name to max-ts-offset-adjustment About the last comment regarding priv->last_pts = pts; Do you mean that we should add an extra check and compare priv->last_out_time to GST_BUFFER_PTS (outbuf), like I did in my first patch (in Comment 1)?
(In reply to Patrick Radizi from comment #5) > About the last comment regarding > priv->last_pts = pts; > Do you mean that we should add an extra check and compare > priv->last_out_time to GST_BUFFER_PTS (outbuf), like I did in my first patch > (in Comment 1)? Similar to that, yes. If the PTS goes backwards, set it to the same PTS as before (don't add a millisecond, just make it the same).
Created attachment 358853 [details] [review] Add new property max-ts-offset-adjustment to control ts_offset Changed name of property to max-ts-offset-adjustment and handles the case described in Comment #6
Review of attachment 358853 [details] [review]: ::: gst/rtpmanager/gstrtpjitterbuffer.c @@ +3451,3 @@ + /* verify that an offset has not caused time stamps to go backwards, if so + * handle by reusing the previous timestamp */ + if (priv->last_out_time > 0 && Please fix up the indentation here :) Also is it 0 when unset? I would've expected GST_CLOCK_TIME_NONE instead. 0 seems like a valid value. And before it was initialized to -1 (aka GST_CLOCK_TIME_NONE) but you changed it to 0 instead
Created attachment 359772 [details] [review] Add new property max-ts-offset-adjustment to control ts_offset - Fixed indentation - Restored default value of last_out_time to GST_CLOCK_TIME_NONE - Yes, I meant that 0 was disable the max_ts_offset_adjustment feature. I had forgotten to mention that in the jitterbuffer property declaration (it was there for rtpbin, rtspsrc) The reason for not choosing -1 was that if 0 would be allowed that would in effect disable the ts_offset property since no time would ever be added to the offset. If you want this behaviour I will change it but this patch kept the original behaviour.
commit 23f7739ba48f67fe55e6e068c9185155ae0c6539 (HEAD -> master) Author: Patrick Radizi <patrickr@axis.com> Date: Thu Sep 14 11:20:17 2017 +0200 rtpbin: add option for increasing ts_offset gradually Instant large changes to ts_offset may cause timestamps to move backwards and also cause visible effects in media playback. The new option max-ts-offset-adjustment lets the application control the rate to apply changes to ts_offset. https://bugzilla.gnome.org/show_bug.cgi?id=784002
This break the timestamps in VOD streaming of AVC H.264 streams where rtp timestamps can go backwards. Specifically, from https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtpmanager/gstrtpjitterbuffer.c?id=23f7739ba48f67fe55e6e068c9185155ae0c6539#n3451 to 3464 inclusive.
Basically whenever you have frame reordering, this is causing problems.
Patrick, are you planning to look at this? Otherwise, I'm going to probably revert it and the dependant commits.
Yes we should revert before 1.13.1 (~Wednesday) and then can revisit it at a later time.
I did not know that timestamps where allowed to go backwards. But for my use case that check is actually not important. I only need the other functionality that comes with the new max-ts-offset-adjustment parameter. So, if I understand you correctly then what is causing this problem is the if statement at gstrtpjitterbuffer.c:3480 that checks if GST_BUFFER_PTS (outbuf) < priv->last_out_time. That if statement can be removed without affecting my use case. Would that be an ok solution for you?
That should be sufficient yes. Should also double-check if there's not another place in the jitterbuffer that assumes (RTP & PTS) timestamps are always increasing.
Created attachment 368346 [details] [review] Allow timestamps to go backwards I don't know of any other places that does that kind of assumption. But at least the following patch should fix the problem introduced with the original solution
Comment on attachment 368346 [details] [review] Allow timestamps to go backwards Looks good to me. Matthew, does this solve your problem?
That's good for me.
commit 364dbb5fc7b9b4c68f87c943e573e7292c350a14 (HEAD -> master) Author: Patrick Radizi <patrickr@axis.com> Date: Wed Feb 14 16:38:07 2018 +0100 rtpjitterbuffer: allow timestamps to move backwards The original solution for #784002 incorrectly assumed that timestamps may not move backwards and changed timestamps that did so. https://bugzilla.gnome.org/show_bug.cgi?id=784002