Do some optimisation work in GstAdapter to avoid copies in more cases.

Original commit message from CVS:
* Makefile.am:
* configure.ac:
* libs/gst/base/gstadapter.c: (gst_adapter_clear),
(gst_adapter_push), (gst_adapter_peek_into), (gst_adapter_peek),
(gst_adapter_flush), (gst_adapter_take), (gst_adapter_take_buffer):
* libs/gst/base/gstadapter.h:
* tests/check/libs/adapter.c: (create_and_fill_adapter),
(GST_START_TEST), (gst_adapter_suite):
* tests/examples/Makefile.am:
Do some optimisation work in GstAdapter to avoid copies in more cases.
It could still do slightly better by merging buffers when
gst_buffer_is_span_fast is true, but is already faster.
Also, avoid traversing a single-linked list to append each incoming
buffer inside the adapter.
Add simple test app that times the adapter behaviour in different
situations, and extend the unit test to check that bytes enter and
exit the adapter in their original order.

ChangeLog

@@ -1,3 +1,25 @@
+2006-11-09  Jan Schmidt  <thaytan@mad.scientist.com>
+
+	* Makefile.am:
+	* configure.ac:
+	* libs/gst/base/gstadapter.c: (gst_adapter_clear),
+	(gst_adapter_push), (gst_adapter_peek_into), (gst_adapter_peek),
+	(gst_adapter_flush), (gst_adapter_take), (gst_adapter_take_buffer):
+	* libs/gst/base/gstadapter.h:
+	* tests/check/libs/adapter.c: (create_and_fill_adapter),
+	(GST_START_TEST), (gst_adapter_suite):
+	* tests/examples/Makefile.am:
+	Do some optimisation work in GstAdapter to avoid copies in more cases.
+	It could still do slightly better by merging buffers when
+	gst_buffer_is_span_fast is true, but is already faster. 
+
+	Also, avoid traversing a single-linked list to append each incoming 
+	buffer inside the adapter.
+
+	Add simple test app that times the adapter behaviour in different
+	situations, and extend the unit test to check that bytes enter and
+	exit the adapter in their original order.
+
 2006-11-08  Tim-Philipp Müller  <tim at centricular dot net>
 
 	* docs/random/draft-missing-plugins.txt:

Makefile.am

@@ -50,7 +50,7 @@ clean-bbg:
 GCOV_DIRS=gst libs
 
 ## .PHONY so it always rebuilds it
-.PHONY: coverage-report.txt test-coverage-report.html
+.PHONY: coverage-report.txt test-coverage-report.html lcov
 
 coverage-report.txt:
 	BBG_FILES=`find $(GCOV_DIRS) -name "*.bbg"` ;			  \

configure.ac

@@ -499,6 +499,7 @@ tests/benchmarks/Makefile
 tests/check/Makefile
 tests/misc/Makefile
 tests/examples/Makefile
+tests/examples/adapter/Makefile
 tests/examples/controller/Makefile
 tests/examples/helloworld/Makefile
 tests/examples/launch/Makefile

libs/gst/base/gstadapter.c

@@ -180,6 +180,7 @@ gst_adapter_clear (GstAdapter * adapter)
   g_slist_foreach (adapter->buflist, (GFunc) gst_mini_object_unref, NULL);
   g_slist_free (adapter->buflist);
   adapter->buflist = NULL;
+  adapter->buflist_end = NULL;
   adapter->size = 0;
   adapter->skip = 0;
   adapter->assembled_len = 0;
@@ -200,9 +201,43 @@ gst_adapter_push (GstAdapter * adapter, GstBuffer * buf)
   g_return_if_fail (GST_IS_BUFFER (buf));
 
   adapter->size += GST_BUFFER_SIZE (buf);
-  /* FIXME, _append does not scale. Note: merging buffers at this
-   * point is premature. */
-  adapter->buflist = g_slist_append (adapter->buflist, buf);
+
+  /* Note: merging buffers at this point is premature. */
+  if (G_UNLIKELY (adapter->buflist == NULL)) {
+    adapter->buflist = adapter->buflist_end = g_slist_append (NULL, buf);
+  } else {
+    /* Otherwise append to the end, and advance our end pointer */
+    adapter->buflist_end = g_slist_append (adapter->buflist_end, buf);
+    adapter->buflist_end = g_slist_next (adapter->buflist_end);
+  }
+}
+
+/* Internal function that copies data into the given buffer, size must be 
+ * bigger than the first buffer */
+static void
+gst_adapter_peek_into (GstAdapter * adapter, guint8 * data, guint size)
+{
+  GstBuffer *cur;
+  GSList *cur_list;
+  guint copied, to_copy;
+
+  /* The first buffer might be partly consumed, so need to handle
+   * 'skipped' bytes. */
+  cur = adapter->buflist->data;
+  copied = to_copy = MIN (GST_BUFFER_SIZE (cur) - adapter->skip, size);
+  memcpy (data, GST_BUFFER_DATA (cur) + adapter->skip, copied);
+  data += copied;
+
+  cur_list = g_slist_next (adapter->buflist);
+  while (copied < size) {
+    g_assert (cur_list);
+    cur = cur_list->data;
+    cur_list = g_slist_next (cur_list);
+    to_copy = MIN (GST_BUFFER_SIZE (cur), size - copied);
+    memcpy (data, GST_BUFFER_DATA (cur), to_copy);
+    data += to_copy;
+    copied += to_copy;
+  }
 }
 
 /**
@@ -230,8 +265,6 @@ const guint8 *
 gst_adapter_peek (GstAdapter * adapter, guint size)
 {
   GstBuffer *cur;
-  GSList *cur_list;
-  guint copied;
 
   g_return_val_if_fail (GST_IS_ADAPTER (adapter), NULL);
   g_return_val_if_fail (size > 0, NULL);
@@ -251,27 +284,20 @@ gst_adapter_peek (GstAdapter * adapter, guint size)
   if (GST_BUFFER_SIZE (cur) >= size + adapter->skip)
     return GST_BUFFER_DATA (cur) + adapter->skip;
 
+  /* FIXME, optimize further - we may be able to efficiently
+   * merge buffers in our pool to gather a big enough chunk to 
+   * return it from the head buffer directly */
+
   if (adapter->assembled_size < size) {
     adapter->assembled_size = (size / DEFAULT_SIZE + 1) * DEFAULT_SIZE;
     GST_DEBUG_OBJECT (adapter, "setting size of internal buffer to %u",
         adapter->assembled_size);
-    adapter->assembled_data =
-        g_realloc (adapter->assembled_data, adapter->assembled_size);
+    g_free (adapter->assembled_data);
+    adapter->assembled_data = g_malloc (adapter->assembled_size);
   }
   adapter->assembled_len = size;
-  copied = GST_BUFFER_SIZE (cur) - adapter->skip;
-  memcpy (adapter->assembled_data, GST_BUFFER_DATA (cur) + adapter->skip,
-      copied);
 
-  cur_list = g_slist_next (adapter->buflist);
-  while (copied < size) {
-    g_assert (cur_list);
-    cur = cur_list->data;
-    cur_list = g_slist_next (cur_list);
-    memcpy (adapter->assembled_data + copied, GST_BUFFER_DATA (cur),
-        MIN (GST_BUFFER_SIZE (cur), size - copied));
-    copied = MIN (size, copied + GST_BUFFER_SIZE (cur));
-  }
+  gst_adapter_peek_into (adapter, adapter->assembled_data, size);
 
   return adapter->assembled_data;
 }
@@ -304,7 +330,10 @@ gst_adapter_flush (GstAdapter * adapter, guint flush)
       /* can skip whole buffer */
       flush -= GST_BUFFER_SIZE (cur) - adapter->skip;
       adapter->skip = 0;
-      adapter->buflist = g_slist_remove (adapter->buflist, cur);
+      adapter->buflist =
+          g_slist_delete_link (adapter->buflist, adapter->buflist);
+      if (G_UNLIKELY (adapter->buflist == NULL))
+        adapter->buflist_end = NULL;
       gst_buffer_unref (cur);
     } else {
       adapter->skip += flush;
@@ -328,20 +357,27 @@ gst_adapter_flush (GstAdapter * adapter, guint flush)
 guint8 *
 gst_adapter_take (GstAdapter * adapter, guint nbytes)
 {
-  const guint8 *cdata;
   guint8 *data;
 
   g_return_val_if_fail (GST_IS_ADAPTER (adapter), NULL);
   g_return_val_if_fail (nbytes > 0, NULL);
 
-  GST_LOG_OBJECT (adapter, "taking %u bytes", nbytes);
-
-  cdata = gst_adapter_peek (adapter, nbytes);
-  if (!cdata)
+  /* we don't have enough data, return NULL. This is unlikely
+   * as one usually does an _available() first instead of peeking a
+   * random size. */
+  if (G_UNLIKELY (nbytes > adapter->size))
     return NULL;
 
   data = g_malloc (nbytes);
-  memcpy (data, cdata, nbytes);
+
+  /* we have enough assembled data, copy from there */
+  if (adapter->assembled_len >= nbytes) {
+    GST_LOG_OBJECT (adapter, "taking %u bytes already assembled", nbytes);
+    memcpy (data, adapter->assembled_data, nbytes);
+  } else {
+    GST_LOG_OBJECT (adapter, "taking %u bytes by collection", nbytes);
+    gst_adapter_peek_into (adapter, data, nbytes);
+  }
 
   gst_adapter_flush (adapter, nbytes);
 
@@ -355,8 +391,8 @@ gst_adapter_take (GstAdapter * adapter, guint nbytes)
  *
  * Returns a #GstBuffer containing the first @nbytes bytes of the
  * @adapter. The returned bytes will be flushed from the adapter.
- * This function is potentially more performant that gst_adapter_take() 
- * since it can reuse the memory in the pushed buffer by subbuffering
+ * This function is potentially more performant than gst_adapter_take() 
+ * since it can reuse the memory in pushed buffers by subbuffering
  * or merging.
  *
  * Caller owns returned value. gst_buffer_unref() after usage.
@@ -370,6 +406,7 @@ GstBuffer *
 gst_adapter_take_buffer (GstAdapter * adapter, guint nbytes)
 {
   GstBuffer *buffer;
+  GstBuffer *cur;
   guint8 *data;
 
   g_return_val_if_fail (GST_IS_ADAPTER (adapter), NULL);
@@ -377,7 +414,23 @@ gst_adapter_take_buffer (GstAdapter * adapter, guint nbytes)
 
   GST_LOG_OBJECT (adapter, "taking buffer of %u bytes", nbytes);
 
-  /* FIXME, optimize me */
+  /* we don't have enough data, return NULL. This is unlikely
+   * as one usually does an _available() first instead of peeking a
+   * random size. */
+  if (G_UNLIKELY (nbytes > adapter->size))
+    return NULL;
+
+  /* our head buffer has enough data left, return it */
+  cur = adapter->buflist->data;
+  if (GST_BUFFER_SIZE (cur) >= nbytes + adapter->skip) {
+    GST_LOG_OBJECT (adapter, "providing buffer via sub-buffer", nbytes);
+    buffer = gst_buffer_create_sub (cur, adapter->skip, nbytes);
+
+    gst_adapter_flush (adapter, nbytes);
+
+    return buffer;
+  }
+
   data = gst_adapter_take (adapter, nbytes);
   if (data == NULL)
     return NULL;
@@ -412,10 +465,12 @@ gst_adapter_available (GstAdapter * adapter)
  * gst_adapter_available_fast:
  * @adapter: a #GstAdapter
  *
- * Gets the maximum number of bytes available without the need to do expensive
- * operations (like copying the data into a temporary buffer).
+ * Gets the maximum number of bytes that are immediately available without
+ * requiring any expensive operations (like copying the data into a 
+ * temporary buffer).
  *
- * Returns: number of bytes available in @adapter without expensive operations
+ * Returns: number of bytes that are available in @adapter without expensive 
+ * operations
  */
 guint
 gst_adapter_available_fast (GstAdapter * adapter)

libs/gst/base/gstadapter.h

@@ -60,7 +60,10 @@ struct _GstAdapter {
   guint		assembled_size;
   guint		assembled_len;
 
-  gpointer _gst_reserved[GST_PADDING];
+  /* Remember where the end of our buffer list is to
+   * speed up the push */
+  GSList *buflist_end;
+  gpointer _gst_reserved[GST_PADDING - 1];
 };
 
 struct _GstAdapterClass {

tests/check/libs/adapter.c

@@ -171,6 +171,79 @@ GST_START_TEST (test_take3)
 
 GST_END_TEST;
 
+static GstAdapter *
+create_and_fill_adapter ()
+{
+  GstAdapter *adapter;
+  gint i, j;
+
+  adapter = gst_adapter_new ();
+  fail_unless (adapter != NULL);
+
+  for (i = 0; i < 10000; i += 4) {
+    GstBuffer *buf = gst_buffer_new_and_alloc (sizeof (guint32) * 4);
+    guint8 *data;
+
+    fail_unless (buf != NULL);
+    data = GST_BUFFER_DATA (buf);
+
+    for (j = 0; j < 4; j++) {
+      GST_WRITE_UINT32_LE (data, i + j);
+      data += sizeof (guint32);
+    }
+    gst_adapter_push (adapter, buf);
+  }
+
+  return adapter;
+}
+
+/* Fill a buffer with a sequence of 32 bit ints and read them back out,
+ * checking that they're still in the right order */
+GST_START_TEST (test_take_order)
+{
+  GstAdapter *adapter;
+  int i = 0;
+
+  adapter = create_and_fill_adapter ();
+  while (gst_adapter_available (adapter) >= sizeof (guint32)) {
+    guint8 *data = gst_adapter_take (adapter, sizeof (guint32));
+
+    fail_unless (GST_READ_UINT32_LE (data) == i);
+    i++;
+    g_free (data);
+  }
+  fail_unless (gst_adapter_available (adapter) == 0,
+      "Data was left in the adapter");
+
+  g_object_unref (adapter);
+}
+
+GST_END_TEST;
+
+/* Fill a buffer with a sequence of 32 bit ints and read them back out
+ * using take_buffer, checking that they're still in the right order */
+GST_START_TEST (test_take_buf_order)
+{
+  GstAdapter *adapter;
+  int i = 0;
+
+  adapter = create_and_fill_adapter ();
+  while (gst_adapter_available (adapter) >= sizeof (guint32)) {
+    GstBuffer *buf = gst_adapter_take_buffer (adapter, sizeof (guint32));
+
+    fail_unless (GST_READ_UINT32_LE (GST_BUFFER_DATA (buf)) == i);
+    i++;
+
+    gst_buffer_unref (buf);
+  }
+  fail_unless (gst_adapter_available (adapter) == 0,
+      "Data was left in the adapter");
+
+  g_object_unref (adapter);
+}
+
+GST_END_TEST;
+
 Suite *
 gst_adapter_suite (void)
 {
@@ -184,6 +257,8 @@ gst_adapter_suite (void)
   tcase_add_test (tc_chain, test_take1);
   tcase_add_test (tc_chain, test_take2);
   tcase_add_test (tc_chain, test_take3);
+  tcase_add_test (tc_chain, test_take_order);
+  tcase_add_test (tc_chain, test_take_buf_order);
 
   return s;
 }

tests/examples/Makefile.am

@@ -15,7 +15,8 @@ always_dirs = \
 	helloworld			\
 	manual                          \
 	metadata                        \
-	queue
+	queue				\
+	adapter
 
 #appreader
 #cutter