Created attachment 354324 [details] [review]
rtpbasepayload

Review of attachment 354324 [details] [review]:

No full review yet, just some comments. It seems useful to (also) add this in rtpbin somewhere if the payloader did not do it yet

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +1285,3 @@
+  if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (priv->base_ntptime))) {
+    guint64 running_time;
+    get_current_times (payload, &running_time, &priv->base_ntptime);

This seems wrong for running_time/clock_time. You probably want to calculate it based on the running time of the buffer PTS instead of taking the clock directly.

Review of attachment 354323 [details] [review]:

Also not a real review yet, just some comments/questions.

In rtpjitterbuffer this could probably also be used for some of the sync modes instead of interpolating timestamps. There already is code that calculates timestamps based on the NTP timestamp, which could directly use this then.

::: gst/rtpmanager/gstrtpbin.c
@@ +1633,3 @@
+
+  if (gst_structure_has_field (s, "ntp-time")) {
+    /* perform in-band synchronization for this stream */

Now you would do sync based on this, and based on RTCP. Is that a problem?
Also this now would only work if the first RTP packet has the header extension, or not? Or will this be called for every single RTP packet (that would be a lot of overhead)?

::: gst/rtpmanager/rtpsession.c
@@ +1964,3 @@
+    if (G_LIKELY (gst_rtp_hdrext_get_ntp_64 (data, size, &ntptime)))
+      return ntptime;
+  } else if (gst_rtp_buffer_get_extension_onebyte_header (rtp, 0xB, 0, &data,

0xA / 0xB are not really reserved for this but could also be used for other header extensions, or not? Isn't this usually negotiated over the SDP?

Thanks for the patches, that's very useful :) Especially because it could now work also completely without RTCP.

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 354324 [details] [review] [review]:
> 
> No full review yet, just some comments. It seems useful to (also) add this
> in rtpbin somewhere if the payloader did not do it yet
> 
> ::: gst-libs/gst/rtp/gstrtpbasepayload.c
> @@ +1285,3 @@
> +  if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (priv->base_ntptime))) {
> +    guint64 running_time;
> +    get_current_times (payload, &running_time, &priv->base_ntptime);
> 
> This seems wrong for running_time/clock_time. You probably want to calculate
> it based on the running time of the buffer PTS instead of taking the clock
> directly.

I'm not sure what you mean.  I do need an actual timestamp provided by a clock (ntp).  I cannot derive a "ntp" time from the PTS.  I then take these timestamps and use those in the RTCP SR packets rather than querying the clock there, which ensures that the direct relationship between ntp timestamp and rtp timestamp is maintained.

(In reply to Sebastian Dröge (slomo) from comment #3)
> In rtpjitterbuffer this could probably also be used for some of the sync
> modes instead of interpolating timestamps. There already is code that
> calculates timestamps based on the NTP timestamp, which could directly use
> this then.

I think the interpolation there is still needed.

> ::: gst/rtpmanager/gstrtpbin.c
> @@ +1633,3 @@
> +
> +  if (gst_structure_has_field (s, "ntp-time")) {
> +    /* perform in-band synchronization for this stream */
> 
> Now you would do sync based on this, and based on RTCP. Is that a problem?
> Also this now would only work if the first RTP packet has the header
> extension, or not? Or will this be called for every single RTP packet (that
> would be a lot of overhead)?

No, the ntp-time field only exists for in-band synchronization.  Currently, this is only called when the first in-band timestamp is provided, as per spec RTCP is still required (in fact the 56 bit mode won't work until the first RTCP).  If we wanted to work entirely without RTCP we should set a timer or something to trigger in-band synchronization periodically.

> 0xA / 0xB are not really reserved for this but could also be used for other
> header extensions, or not? Isn't this usually negotiated over the SDP?

I'm not sure.  I assumed so, at least looking at section 3.3.  I don't know if these values can be configured through SDP, section 3.3 only mentions urn:ietf:params:rtp-hdrext:ntp-64 and urn:ietf:params:rtp-hdrext:ntp-56

(In reply to Sebastian Dröge (slomo) from comment #4)
> Thanks for the patches, that's very useful :) Especially because it could
> now work also completely without RTCP.

As mentioned in comment #5, if we wanted to use ntp-64 mode without RTCP at all, we should probably add a timer so that we periodically perform synchronization rather than only on the first packet.

I think you're right about the 0xA/0xB.  I think I mis-read the spec.  Not sure where these values are defined or how that should work.  I'm not as familiar with SDP and how it ties in with RTP.

What if I add a property "inband-sync-hdr-id" to rtpbasepayload and rtpbin to define the in-band header extension id, and then rtpbin would also need the same property "inband-sync" as rtpbasepayload?

Created attachment 354341 [details] [review]
Added inband-sync-hdr-id property to rtpbasepayload

Created attachment 354342 [details] [review]
Added inband-sync and inband-sync-hdr-id properties to rtpbin

-- 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/383.