rtp: fix gst_rtp_buffer_ext_timestamp taking into account backwards

If timestamp goes forwards more than allowed, we consider that the
timestamp belongs to the previous counting, so the extended timestamp
is unwrapped.

https://bugzilla.gnome.org/show_bug.cgi?id=783443

diff --git a/gst-libs/gst/rtp/gstrtpbuffer.c b/gst-libs/gst/rtp/gstrtpbuffer.c
index 40392f3d571837b079a610a3f9c88c4117078a46..916b6479544c6785de987be1af25848d6401b7ef 100644 (file)
--- a/gst-libs/gst/rtp/gstrtpbuffer.c
+++ b/gst-libs/gst/rtp/gstrtpbuffer.c
@@ -1252,18 +1252,21 @@ gst_rtp_buffer_compare_seqnum (guint16 seqnum1, guint16 seqnum2)
  * @exttimestamp: a previous extended timestamp
  * @timestamp: a new timestamp
  *
- * Update the @exttimestamp field with @timestamp. For the first call of the
- * method, @exttimestamp should point to a location with a value of -1.
+ * Update the @exttimestamp field with the extended timestamp of @timestamp
+ * For the first call of the method, @exttimestamp should point to a location
+ * with a value of -1.
  *
- * This function makes sure that the returned value is a constantly increasing
- * value even in the case where there is a timestamp wraparound.
+ * This function is able to handle both forward and backward timestamps taking
+ * into account:
+ *   - timestamp wraparound making sure that the returned value is properly increased.
+ *   - timestamp unwraparound making sure that the returned value is properly decreased.
  *
- * Returns: The extended timestamp of @timestamp.
+ * Returns: The extended timestamp of @timestamp or 0 if the result can't go anywhere backwards.
  */
 guint64
 gst_rtp_buffer_ext_timestamp (guint64 * exttimestamp, guint32 timestamp)
 {
-  guint64 result, diff, ext;
+  guint64 result, ext;
 
   g_return_val_if_fail (exttimestamp != NULL, -1);
 
@@ -1276,17 +1279,33 @@ gst_rtp_buffer_ext_timestamp (guint64 * exttimestamp, guint32 timestamp)
     result = timestamp + (ext & ~(G_GUINT64_CONSTANT (0xffffffff)));
 
     /* check for timestamp wraparound */
-    if (result < ext)
-      diff = ext - result;
-    else
-      diff = result - ext;
-
-    if (diff > G_MAXINT32) {
-      /* timestamp went backwards more than allowed, we wrap around and get
-       * updated extended timestamp. */
-      result += (G_GUINT64_CONSTANT (1) << 32);
+    if (result < ext) {
+      guint64 diff = ext - result;
+
+      if (diff > G_MAXINT32) {
+        /* timestamp went backwards more than allowed, we wrap around and get
+         * updated extended timestamp. */
+        result += (G_GUINT64_CONSTANT (1) << 32);
+      }
+    } else {
+      guint64 diff = result - ext;
+
+      if (diff > G_MAXINT32) {
+        if (result < (G_GUINT64_CONSTANT (1) << 32)) {
+          GST_WARNING
+              ("Cannot unwrap, any wrapping took place yet. Returning 0 without updating extended timestamp.");
+          return 0;
+        } else {
+          /* timestamp went forwards more than allowed, we unwrap around and get
+           * updated extended timestamp. */
+          result -= (G_GUINT64_CONSTANT (1) << 32);
+          /* We don't want the extended timestamp storage to go back, ever */
+          return result;
+        }
+      }
     }
   }
+
   *exttimestamp = result;
 
   return result;

diff --git a/tests/check/Makefile.am b/tests/check/Makefile.am
index d4e0536562341628caf2b4cc71778c2d83d4fbd0..e90619dfd017523e2dfeda853ecf247aa78f723f 100644 (file)
--- a/tests/check/Makefile.am
+++ b/tests/check/Makefile.am
@@ -311,6 +311,10 @@ VALGRIND_TESTS_DISABLE = $(VALGRIND_TO_FIX)
 
 SUPPRESSIONS = $(top_srcdir)/common/gst.supp $(srcdir)/gst-plugins-base.supp
 
+generic_rtp_LDADD = \
+        $(top_builddir)/gst-libs/gst/rtp/libgstrtp-@GST_API_VERSION@.la \
+        $(LDADD)
+
 libs_libsabi_CFLAGS = \
        $(GST_PLUGINS_BASE_CFLAGS) \
        $(GST_BASE_CFLAGS) \

diff --git a/tests/check/libs/rtp.c b/tests/check/libs/rtp.c
index 20c01ab860c99d703ef71115033b89570b0faa8a..72d526ab94f9c56ccace3cf48874363e1ffa1a8c 100644 (file)
--- a/tests/check/libs/rtp.c
+++ b/tests/check/libs/rtp.c
@@ -1353,6 +1353,90 @@ GST_START_TEST (test_rtp_buffer_empty_payload)
 
 GST_END_TEST;
 
+
+GST_START_TEST (test_ext_timestamp_basic)
+{
+  guint64 exttimestamp = -1;
+  guint64 result = 0;
+
+  /* no wraparound when timestamps are increasing */
+  result = gst_rtp_buffer_ext_timestamp (&exttimestamp, 0);
+  fail_unless_equals_uint64 (result, 0);
+  result = gst_rtp_buffer_ext_timestamp (&exttimestamp, 10);
+  fail_unless_equals_uint64 (result, 10);
+  result = gst_rtp_buffer_ext_timestamp (&exttimestamp, 10);
+  fail_unless_equals_uint64 (result, 10);
+  result = gst_rtp_buffer_ext_timestamp (&exttimestamp,
+      G_GUINT64_CONSTANT (1) + G_MAXINT32);
+  fail_unless_equals_uint64 (result, G_GUINT64_CONSTANT (1) + G_MAXINT32);
+
+  /* Even big bumps under G_MAXINT32 don't result in wrap-around */
+  exttimestamp = -1;
+  result = gst_rtp_buffer_ext_timestamp (&exttimestamp, 1087500);
+  fail_unless_equals_uint64 (result, 1087500);
+  result = gst_rtp_buffer_ext_timestamp (&exttimestamp, 24);
+  fail_unless_equals_uint64 (result, 24);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_ext_timestamp_wraparound)
+{
+  guint64 ext_ts = -1;
+
+  fail_unless_equals_uint64 (gst_rtp_buffer_ext_timestamp (&ext_ts,
+          G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1)),
+      (G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1)));
+
+  fail_unless_equals_uint64 (gst_rtp_buffer_ext_timestamp (&ext_ts, 0),
+      G_MAXUINT32 + G_GUINT64_CONSTANT (1));
+
+  fail_unless_equals_uint64 (gst_rtp_buffer_ext_timestamp (&ext_ts, 90000),
+      (G_MAXUINT32 + G_GUINT64_CONSTANT (1) + 90000));
+}
+
+GST_END_TEST;
+
+
+GST_START_TEST (test_ext_timestamp_wraparound_disordered)
+{
+  guint64 ext_ts = -1;
+
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts,
+          G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1))
+      == (G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1)));
+
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts, 0)
+      == G_MAXUINT32 + G_GUINT64_CONSTANT (1));
+
+  /* Unwrapping around */
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts,
+          G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1))
+      == (G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1)));
+
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts, 90000)
+      == (G_MAXUINT32 + G_GUINT64_CONSTANT (1) + 90000));
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_ext_timestamp_wraparound_disordered_cannot_unwrap)
+{
+  guint64 ext_ts = -1;
+
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts, 90000)
+      == 90000);
+
+  /* Cannot unwrapping around */
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts,
+          G_MAXUINT32 - 90000 + G_GUINT64_CONSTANT (1)) == 0);
+
+  fail_unless (gst_rtp_buffer_ext_timestamp (&ext_ts, 90000)
+      == 90000);
+}
+
+GST_END_TEST;
+
 static Suite *
 rtp_suite (void)
 {
@@ -1389,6 +1473,12 @@ rtp_suite (void)
 
   //tcase_add_test (tc_chain, test_rtp_buffer_list);
 
+  tcase_add_test (tc_chain, test_ext_timestamp_basic);
+  tcase_add_test (tc_chain, test_ext_timestamp_wraparound);
+  tcase_add_test (tc_chain, test_ext_timestamp_wraparound_disordered);
+  tcase_add_test (tc_chain,
+      test_ext_timestamp_wraparound_disordered_cannot_unwrap);
+
   return s;
 }