GNOME Bugzilla – Bug 719383
rtpbasepayload: Perfect timestamps confusingly explained
Last modified: 2014-01-28 13:45:36 UTC
In gst_rtp_base_payload_prepare_push() if perfect_rtptime == TRUE and data.offset and base_offset != GST_BUFFER_OFFSET_NONE perfect timestamps will be used to compute the rtptime of each buffer based on its offset compared to the first buffer passed through the payloader. Now, the expression looks like this: rtptime = ts_base + base_rtime + data.offset - base_offset; ts_base is the random rtp offset expressed in media clock hz. base_rtime is the first buffers running time expressed in hz (it is sampled for the first buffer in the else clause). base_offset is the first buffer's data.offset expressed in ns. data.offset is the current buffer's offset expressed in ns. This means that the above expression is a mix of hz and ns. The attached patchs fixes this and does some cosmetic changes to make it less probable that a similar mistake takes place in the future.
Created attachment 262900 [details] [review] Proposed cosmetic patch, preparing for the real one
Created attachment 262901 [details] [review] Proposed patch expresng all terms in hz
Patches look good, but we should have a unit test for this.
Agreed, I'll try to put some effort into uploading one here soon.
(In reply to comment #0) > > rtptime = ts_base + base_rtime + data.offset - base_offset; > > ts_base is the random rtp offset expressed in media clock hz. > base_rtime is the first buffers running time expressed in hz (it is sampled for > the first buffer in the else clause). > base_offset is the first buffer's data.offset expressed in ns. > data.offset is the current buffer's offset expressed in ns. not really. data.offset if the media specific offset (not expressed in nanoseconds, for audio this is the number of samples) and base_offset is the sample offset since we took base_rtime. > > This means that the above expression is a mix of hz and ns. For audio they are both in Hz.
Comment on attachment 262901 [details] [review] Proposed patch expresng all terms in hz >From 98da63a53ccb970b8aec3de500acadb145d5b056 Mon Sep 17 00:00:00 2001 >From: Sebastian Rasmussen <sebrn@axis.com> >Date: Mon, 25 Nov 2013 23:53:19 +0100 >Subject: [PATCH 2/2] rtpbasepayload: Fix wrongly computed perfect RTP > timestamps > >Previously the rtptimes were a mix of media clock hz and ns, >now all terms used to compute rtptime is expressed in hz. > >Fixes https://bugzilla.gnome.org/show_bug.cgi?id=719383 >--- > gst-libs/gst/rtp/gstrtpbasepayload.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > >diff --git a/gst-libs/gst/rtp/gstrtpbasepayload.c b/gst-libs/gst/rtp/gstrtpbasepayload.c >index a824a78..7587f4c 100644 >--- a/gst-libs/gst/rtp/gstrtpbasepayload.c >+++ b/gst-libs/gst/rtp/gstrtpbasepayload.c >@@ -834,11 +834,19 @@ gst_rtp_base_payload_prepare_push (GstRTPBasePayload * payload, > /* convert to RTP time */ > if (priv->perfect_rtptime && data.offset != GST_BUFFER_OFFSET_NONE && > priv->base_offset != GST_BUFFER_OFFSET_NONE) { >+ guint64 rtime_ns; >+ guint64 rtime_hz; >+ > /* if we have an offset, use that for making an RTP timestamp */ >- data.rtptime = payload->ts_base + priv->base_rtime + >- data.offset - priv->base_offset; > GST_LOG_OBJECT (payload, > "Using offset %" G_GUINT64_FORMAT " for RTP timestamp", data.offset); >+ rtime_ns = data.offset - priv->base_offset; This is not nanoseconds, this is elapsed number of samples for audio. >+ rtime_hz = >+ gst_util_uint64_scale_int (rtime_ns, payload->clock_rate, GST_SECOND); This is then wrong. Are you trynig to use perfect timestamps for video?
No, I was trying to understand how to handle perfect timestamping in the light of https://bugzilla.gnome.org/show_bug.cgi?id=719415, as discussed om #gstreamer. I'll get back to this patchset soon.
Ok, so I got back to this now. I have after further discussions with wtay on #gstreamer finally understood what perfect-rtptime really means to the point where I was able to improve the documentation. I therefore renamed this bug as the timestamps _aren't_ incorrectly computed, but instead confusingly described. Hence I provide you with documentation patches that improves the documentation.
Created attachment 266999 [details] [review] Proposed patch fixing typos.
Created attachment 267000 [details] [review] Proposed patch improving documentation for perfect-rtptime.
Created attachment 267001 [details] [review] Proposed cosmetic patch.
Created attachment 267367 [details] [review] Proposed patch fixing typos.
Created attachment 267368 [details] [review] Proposed patch improving documentation for perfect-rtptime.
Created attachment 267369 [details] [review] Proposed cosmetic patch.
Rebased this patch series to apply after 719415 has been committed.
Created attachment 267373 [details] [review] Proposed cosmetic patch. Some day I will be able to clearly distinguish between GST_TIME_FORMAT and GST_FORMAT_TIME. Today is not that day...
commit 125b9c19c0e23864ac8723e30bc1662701f43f32 Author: Sebastian Rasmussen <sebrn@axis.com> Date: Tue Nov 26 01:13:45 2013 +0100 rtpbasepayload: Do cosmetic changes to rtptime calculations * Change running time type to guint64 * Use GST_CLOCK_TIME_NONE() to check for invalid timestamps * Name variables so ns-based and hz-based timestamps are evident Fixes https://bugzilla.gnome.org/show_bug.cgi?id=719383 commit 865a5d1c8fb8107da53bb3ea943f2def2782f12d Author: Sebastian Rasmussen <sebrn@axis.com> Date: Wed Jan 22 17:47:02 2014 +0100 rtpbasepayload: Improve documentation for perfect-rtptime Fixes https://bugzilla.gnome.org/show_bug.cgi?id=719383 commit 713dfe0d704917e7af6df53dbbd1967a09d5b13f Author: Sebastian Rasmussen <sebrn@axis.com> Date: Thu Jan 16 16:58:43 2014 +0100 rtpbasepayload: Fix typos in documentation for properties Fixes https://bugzilla.gnome.org/show_bug.cgi?id=719383