diff --git a/ChangeLog b/ChangeLog index deb10ad455..568a32f715 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,25 @@ +2006-11-09 Jan Schmidt + + * 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 * docs/random/draft-missing-plugins.txt: diff --git a/Makefile.am b/Makefile.am index 7472ca77db..86539d5af9 100644 --- a/Makefile.am +++ b/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"` ; \ diff --git a/configure.ac b/configure.ac index 82e03e6af9..462a05b427 100644 --- a/configure.ac +++ b/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 diff --git a/libs/gst/base/gstadapter.c b/libs/gst/base/gstadapter.c index 30ad670fa8..df1a080584 100644 --- a/libs/gst/base/gstadapter.c +++ b/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) diff --git a/libs/gst/base/gstadapter.h b/libs/gst/base/gstadapter.h index 36492fbf43..1ae993b627 100644 --- a/libs/gst/base/gstadapter.h +++ b/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 { diff --git a/tests/check/libs/adapter.c b/tests/check/libs/adapter.c index 2fe65731be..e236b63780 100644 --- a/tests/check/libs/adapter.c +++ b/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; } diff --git a/tests/examples/Makefile.am b/tests/examples/Makefile.am index 25c625e839..e27f19d246 100644 --- a/tests/examples/Makefile.am +++ b/tests/examples/Makefile.am @@ -15,7 +15,8 @@ always_dirs = \ helloworld \ manual \ metadata \ - queue + queue \ + adapter #appreader #cutter