GNOME Bugzilla – Bug 793513
rtpbin, jitterbuffer: ts-offset on RFC7273 synchronized streams
Last modified: 2018-11-03 15:26:22 UTC
The rtpbin sets the ts-offset on rtpjitterbuffer to synchronize multiple SSRC for one CNAME based on the timestamps in the SR. The offset is added to the buffer pts after the pts has been calculated from the rtptime. However, I use RFC7273 to already synchronize RTP streams between multiple clients and the rtpjitterbuffers already calculate a synchronized pts. The additional offset breaks the synchronization again. Therefore, I don't want the rtpbin to add a ts-offset to the streams if the pts is already in sync. I added a check in the rtpbin, if rfc7273-sync is set and skip the synchronization in these cases and with this change, my streams are in sync. However this is not really a fix, because the rfc7273-sync can be also set if the SDP does not announce an RFC7273 clock. In this case, the rtpbin should synchronize the streams. If there is only one SSRC, there not an issue, because then the rtpbin does not add the offset, but I want separate streams for audio and video to avoid the additional latency for muxing and demuxing. Maybe we can add a property to the rtpjitterbuffer if the produced pts are already synchronous to a wall clock or if the rtpbin should take care of synchronization. Any thoughts?
The ts-offset setting from RTCP you mean? That should never be applied for RFC7273 *if* a proper clock is available and the media-offset is given.
Do you want to provide a patch for that?
The rtpbin does not know if the rtpjitterbuffer has a proper clock and media-offset and does not know if it should set the ts-offset or if not. Should the rtpjitterbuffer always ignore the ts-offset if it uses RFC7273? This would include situations when some application explicitly sets the offset.
Created attachment 368718 [details] [review] 0001-rtpjitterbuffer-do-not-apply-ts-offset-if-streams-ar.patch The attached patch skips all adjustments using ts-offset if the pts is calculated from an RFC7273 synchronized clock. Is still keeps and updates the property to fall back if synchronization is lost or disabled.
Review of attachment 368718 [details] [review]: This patch looks fine, but another option is to have a readable property on the jitterbuffer to know if it is indeed using RFC7273 sync and have rtpbin read that.
(In reply to Olivier Crête from comment #5) > This patch looks fine, but another option is to have a readable property on > the jitterbuffer to know if it is indeed using RFC7273 sync and have rtpbin > read that. Yes, but if the RFC7273 synchronization is lost, the jitterbuffer itself can do a fallback and use the ts-offset for synchronization. If the rtpbin is responsible for checking RFC7273 it either has to wait until the next SR or listen for changes of the property and keep track of the ts-offset itself.
Review of attachment 368718 [details] [review]: ::: gst/rtpmanager/gstrtpjitterbuffer.c @@ +1346,3 @@ if ((item = rtp_jitter_buffer_peek (priv->jbuf))) { /* head buffer timestamp and offset gives our output time */ + if (!rtp_jitter_buffer_is_rfc7273_synced (priv->jbuf)) Do you thin you could add a comment explaining why the offset is not apply in RFC7273 mode? The Jitterbuffer code is already super hard to follow. @@ +2056,3 @@ return -1; + if (rtp_jitter_buffer_is_rfc7273_synced (priv->jbuf)) Same here
Created attachment 369086 [details] [review] 0001-rtpjitterbuffer-do-not-adjust-pts-if-streams-are-rfc.patch (In reply to Olivier Crête from comment #7) > Review of attachment 368718 [details] [review] [review]: > > ::: gst/rtpmanager/gstrtpjitterbuffer.c > @@ +1346,3 @@ > if ((item = rtp_jitter_buffer_peek (priv->jbuf))) { > /* head buffer timestamp and offset gives our output time */ > + if (!rtp_jitter_buffer_is_rfc7273_synced (priv->jbuf)) > > Do you thin you could add a comment explaining why the offset is not apply > in RFC7273 mode? The Jitterbuffer code is already super hard to follow. I know:) I added comments in which I try to explain why the offset does not apply. It's very hard to come up with a concise explanation, though. > > @@ +2056,3 @@ > return -1; > > + if (rtp_jitter_buffer_is_rfc7273_synced (priv->jbuf)) > > Same here
Do the comments help?
-- 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/441.