gst-libs/gst/rtp/gstrtpbuffer.c: Fix up GstRTPHeader helper struct so that compilers will not under any circumstances...

Original commit message from CVS:
Based on patch by: Haakon Sporsheim  <haakon.sporsheim at tandberg com>
* gst-libs/gst/rtp/gstrtpbuffer.c:
Fix up GstRTPHeader helper struct so that compilers will not under
any circumstances add padding in between our fields, as currently
happens with MSVC on win32, because that would lead to us sending
out RTP payloads with broken RTP headers (#471194).
Fix assertion guards for gst_rtp_buffer_get_csrc() and _set_csrc().
* tests/check/Makefile.am:
* tests/check/libs/.cvsignore:
* tests/check/libs/rtp.c:
Add some simple unit tests for GstRTPBuffer. Some are disabled
because the code tested still needs fixing (set_csrc() does not work).

ChangeLog

@@ -1,3 +1,20 @@
+2007-09-07  Tim-Philipp Müller  <tim at centricular dot net>
+
+	Based on patch by: Haakon Sporsheim  <haakon.sporsheim at tandberg com>
+
+	* gst-libs/gst/rtp/gstrtpbuffer.c:
+	  Fix up GstRTPHeader helper struct so that compilers will not under
+	  any circumstances add padding in between our fields, as currently
+          happens with MSVC on win32, because that would lead to us sending
+	  out RTP payloads with broken RTP headers (#471194).
+	  Fix assertion guards for gst_rtp_buffer_get_csrc() and _set_csrc().
+	  
+	* tests/check/Makefile.am:
+	* tests/check/libs/.cvsignore:
+	* tests/check/libs/rtp.c:
+	  Add some simple unit tests for GstRTPBuffer. Some are disabled
+	  because the code tested still needs fixing (set_csrc() does not work).
+
 2007-09-07  Tim-Philipp Müller  <tim at centricular dot net>
 
 	* win32/MANIFEST:

gst-libs/gst/rtp/gstrtpbuffer.c

@@ -40,6 +40,9 @@
 
 #define GST_RTP_HEADER_LEN 12
 
+/* Note: we use bitfields here to make sure the compiler doesn't add padding
+ * between fields on certain architectures; can't assume aligned access either
+ */
 typedef struct _GstRTPHeader
 {
 #if G_BYTE_ORDER == G_LITTLE_ENDIAN
@@ -59,10 +62,10 @@ typedef struct _GstRTPHeader
 #else
 #error "G_BYTE_ORDER should be big or little endian."
 #endif
-  guint16 seq;                  /* sequence number */
-  guint32 timestamp;            /* timestamp */
-  guint32 ssrc;                 /* synchronization source */
-  guint32 csrc[1];              /* optional CSRC list */
+  unsigned int seq:16;          /* sequence number */
+  unsigned int timestamp:32;    /* timestamp */
+  unsigned int ssrc:32;         /* synchronization source */
+  guint8 csrclist[4];           /* optional CSRC list, 32 bits each */
 } GstRTPHeader;
 
 #define GST_RTP_HEADER_VERSION(buf)     (((GstRTPHeader *)(GST_BUFFER_DATA (buf)))->version)
@@ -74,8 +77,10 @@ typedef struct _GstRTPHeader
 #define GST_RTP_HEADER_SEQ(buf)         (((GstRTPHeader *)(GST_BUFFER_DATA (buf)))->seq)
 #define GST_RTP_HEADER_TIMESTAMP(buf)   (((GstRTPHeader *)(GST_BUFFER_DATA (buf)))->timestamp)
 #define GST_RTP_HEADER_SSRC(buf)        (((GstRTPHeader *)(GST_BUFFER_DATA (buf)))->ssrc)
-#define GST_RTP_HEADER_CSRC(buf,i)      (((GstRTPHeader *)(GST_BUFFER_DATA (buf)))->csrc[i])
-
+#define GST_RTP_HEADER_CSRC_LIST_OFFSET(buf,i)  \
+    GST_BUFFER_DATA (buf) +                     \
+    G_STRUCT_OFFSET(GstRTPHeader, csrclist) +   \
+    ((i) * sizeof(guint32))
 #define GST_RTP_HEADER_CSRC_SIZE(buf)   (GST_RTP_HEADER_CSRC_COUNT(buf) * sizeof (guint32))
 
 /**
@@ -110,7 +115,7 @@ gst_rtp_buffer_allocate_data (GstBuffer * buffer, guint payload_len,
   GST_RTP_HEADER_VERSION (buffer) = GST_RTP_VERSION;
   GST_RTP_HEADER_PADDING (buffer) = FALSE;
   GST_RTP_HEADER_EXTENSION (buffer) = FALSE;
-  GST_RTP_HEADER_CSRC_COUNT (buffer) = 0;
+  GST_RTP_HEADER_CSRC_COUNT (buffer) = 0;       /* FIXME: not csrc_count? */
   GST_RTP_HEADER_MARKER (buffer) = FALSE;
   GST_RTP_HEADER_PAYLOAD_TYPE (buffer) = 0;
   GST_RTP_HEADER_SEQ (buffer) = 0;
@@ -687,9 +692,9 @@ gst_rtp_buffer_get_csrc (GstBuffer * buffer, guint8 idx)
 {
   g_return_val_if_fail (GST_IS_BUFFER (buffer), 0);
   g_return_val_if_fail (GST_BUFFER_DATA (buffer) != NULL, 0);
-  g_return_val_if_fail (GST_RTP_HEADER_CSRC_COUNT (buffer) < idx, 0);
+  g_return_val_if_fail (idx < GST_RTP_HEADER_CSRC_COUNT (buffer), 0);
 
-  return g_ntohl (GST_RTP_HEADER_CSRC (buffer, idx));
+  return GST_READ_UINT32_BE (GST_RTP_HEADER_CSRC_LIST_OFFSET (buffer, idx));
 }
 
 /**
@@ -705,9 +710,9 @@ gst_rtp_buffer_set_csrc (GstBuffer * buffer, guint8 idx, guint32 csrc)
 {
   g_return_if_fail (GST_IS_BUFFER (buffer));
   g_return_if_fail (GST_BUFFER_DATA (buffer) != NULL);
-  g_return_if_fail (GST_RTP_HEADER_CSRC_COUNT (buffer) < idx);
+  g_return_if_fail (idx < GST_RTP_HEADER_CSRC_COUNT (buffer));
 
-  GST_RTP_HEADER_CSRC (buffer, idx) = g_htonl (csrc);
+  GST_WRITE_UINT32_BE (GST_RTP_HEADER_CSRC_LIST_OFFSET (buffer, idx), csrc);
 }
 
 /**

tests/check/Makefile.am

@@ -93,6 +93,7 @@ check_PROGRAMS = \
 	libs/mixer \
 	libs/netbuffer \
 	libs/pbutils \
+	libs/rtp \
 	libs/tag \
 	libs/video \
 	pipelines/simple-launch-lines \
@@ -167,6 +168,12 @@ libs_netbuffer_LDADD = \
 	$(top_builddir)/gst-libs/gst/netbuffer/libgstnetbuffer-@GST_MAJORMINOR@.la \
 	$(LDADD)
 
+libs_rtp_CFLAGS = \
+	$(GST_PLUGINS_BASE_CFLAGS) \
+	$(AM_CFLAGS)
+libs_rtp_LDADD = \
+	$(top_builddir)/gst-libs/gst/rtp/libgstrtp-@GST_MAJORMINOR@.la $(LDADD)
+
 libs_tag_CFLAGS = \
 	$(GST_PLUGINS_BASE_CFLAGS) \
 	$(AM_CFLAGS)

tests/check/libs/.gitignore

@@ -2,9 +2,11 @@
 audio
 cddabasesrc
 fft
+mixer
 netbuffer
+pbutils
+rtp
 tag
 utils
 video
-pbutils
-mixer
+

tests/check/libs/rtp.c

@@ -0,0 +1,140 @@
+/* GStreamer unit tests for the RTP support library
+ *
+ * Copyright (C) 2007 Tim-Philipp Müller <tim centricular net>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 02111-1307, USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <gst/check/gstcheck.h>
+
+#include <gst/rtp/gstrtpbuffer.h>
+#include <string.h>
+
+#define RTP_HEADER_LEN 12
+
+GST_START_TEST (test_rtp_buffer)
+{
+  GstBuffer *buf;
+  guint8 *data;
+
+  /* check GstRTPHeader structure alignment and packing */
+  buf = gst_rtp_buffer_new_allocate (16, 4, 3);
+  fail_unless (buf != NULL);
+  fail_unless_equals_int (GST_BUFFER_SIZE (buf),
+      RTP_HEADER_LEN + 16 + 4 + 4 * 3);
+  data = GST_BUFFER_DATA (buf);
+
+  /* check version in bitfield */
+  gst_rtp_buffer_set_version (buf, 3);
+  fail_unless_equals_int (gst_rtp_buffer_get_version (buf), 3);
+  fail_unless_equals_int ((data[0] & 0xC0) >> 6, 3);
+  gst_rtp_buffer_set_version (buf, 0);
+  fail_unless_equals_int (gst_rtp_buffer_get_version (buf), 0);
+  fail_unless_equals_int ((data[0] & 0xC0) >> 6, 0);
+
+  /* check sequence offset */
+  gst_rtp_buffer_set_seq (buf, 0xF2C9);
+  fail_unless_equals_int (gst_rtp_buffer_get_seq (buf), 0xF2C9);
+  fail_unless_equals_int (GST_READ_UINT16_BE (data + 2), 0xF2C9);
+  gst_rtp_buffer_set_seq (buf, 0);
+  fail_unless_equals_int (gst_rtp_buffer_get_seq (buf), 0);
+  fail_unless_equals_int (GST_READ_UINT16_BE (data + 2), 0);
+
+  /* check timestamp offset */
+  gst_rtp_buffer_set_timestamp (buf, 432191);
+  fail_unless_equals_int (GST_READ_UINT32_BE (data + 4), 432191);
+  fail_unless_equals_int (gst_rtp_buffer_get_timestamp (buf), 432191);
+  gst_rtp_buffer_set_timestamp (buf, 0);
+  fail_unless_equals_int (gst_rtp_buffer_get_timestamp (buf), 0);
+  fail_unless_equals_int (GST_READ_UINT32_BE (data + 4), 0);
+
+  /* check ssrc offset */
+  gst_rtp_buffer_set_ssrc (buf, 0xf04043C2);
+  fail_unless_equals_int (gst_rtp_buffer_get_ssrc (buf), 0xf04043c2);
+  fail_unless_equals_int (GST_READ_UINT32_BE (data + 4 + 4), 0xf04043c2);
+  gst_rtp_buffer_set_ssrc (buf, 0);
+  fail_unless_equals_int (gst_rtp_buffer_get_ssrc (buf), 0);
+  fail_unless_equals_int (GST_READ_UINT32_BE (data + 4 + 4), 0);
+  gst_buffer_unref (buf);
+
+  /* FIXME: this is broken, the _set_csrc doesn't work because the csrc count
+   * is initialised to 0 and _set_csrc() then has an assertion to make sure
+   * index is < the value in the struct ... */
+#if 0
+  /* and again, this time with CSRCs */
+  {
+    volatile guint32 ret;
+
+    buf = gst_rtp_buffer_new_allocate (16, 4, 3);
+    fail_unless (buf != NULL);
+    fail_unless_equals_int (GST_BUFFER_SIZE (buf),
+        RTP_HEADER_LEN + 16 + 4 + 4 * 3);
+
+    data = GST_BUFFER_DATA (buf);
+
+    /* the default value is 0, because we haven't set any yet (FIXME?) */
+    fail_unless_equals_int (gst_rtp_buffer_get_csrc_count (buf), 0);
+    ASSERT_CRITICAL (ret = gst_rtp_buffer_get_csrc (buf, 0));
+
+    data += RTP_HEADER_LEN;     /* skip the other header stuff */
+    gst_rtp_buffer_set_csrc (buf, 0, 0xf7c0);
+    fail_unless_equals_int (GST_READ_UINT32_BE (data + 0 * 4), 0xf7c0);
+    fail_unless_equals_int (gst_rtp_buffer_get_csrc_count (buf), 1);    /* FIXME? */
+    gst_rtp_buffer_set_csrc (buf, 1, 0xf7c1);
+    fail_unless_equals_int (GST_READ_UINT32_BE (data + 1 * 4), 0xf7c1);
+    fail_unless_equals_int (gst_rtp_buffer_get_csrc_count (buf), 2);    /* FIXME? */
+    gst_rtp_buffer_set_csrc (buf, 2, 0xf7c2);
+    fail_unless_equals_int (GST_READ_UINT32_BE (data + 2 * 4), 0xf7c2);
+    fail_unless_equals_int (gst_rtp_buffer_get_csrc_count (buf), 3);    /* FIXME? */
+    ASSERT_CRITICAL (gst_rtp_buffer_set_csrc (buf, 3, 0xf123));
+    gst_buffer_unref (buf);
+  }
+#endif
+}
+
+GST_END_TEST;
+
+static Suite *
+rtp_suite (void)
+{
+  Suite *s = suite_create ("rtp support library");
+  TCase *tc_chain = tcase_create ("general");
+
+  suite_add_tcase (s, tc_chain);
+  tcase_add_test (tc_chain, test_rtp_buffer);
+  return s;
+}
+
+int
+main (int argc, char **argv)
+{
+  int nf;
+
+  Suite *s = rtp_suite ();
+  SRunner *sr = srunner_create (s);
+
+  gst_check_init (&argc, &argv);
+
+  srunner_run_all (sr, CK_NORMAL);
+  nf = srunner_ntests_failed (sr);
+  srunner_free (sr);
+
+  return nf;
+}