Created attachment 370811 [details] [review]
adapter: port the buffer list from GSList to GstQueueArray

Significantly reduces the amount of memory allocation operations.

Comment on attachment 370811 [details] [review]
adapter: port the buffer list from GSList to GstQueueArray

Nice!

I think this also makes the code nicer to read in many places.

Would be good to also run a full make check/valgrind test suite on the plugin modules if you haven't done so yet.

Couple of nitpicks:


>@@ -160,7 +160,7 @@ struct _GstAdapter
> 
>+  guint scan_entry_idx;

Maybe add a comment here that G_MAXUINT means invalid/unset.


>@@ -661,20 +659,18 @@ gst_adapter_flush_unchecked (GstAdapter * adapter, gsize flush)
>     flush -= size;
> 
>     gst_buffer_unref (cur);
>-    g = g_slist_delete_link (g, g);
>     --adapter->count;
> 
>-    if (G_UNLIKELY (g == NULL)) {
>+    gst_queue_array_pop_head (adapter->bufqueue);

I think it would be good to move the pop_head before the buffer unref here, and/or even unref the return value directly for clarity. With the current code we basically have a dangling pointer in the queue array for a few lines. It shouldn't matter of course, but it's nice if it can be avoided and will help avoid problems later when people make additional changes.


>@@ -661,20 +659,18 @@ gst_adapter_flush_unchecked (GstAdapter * adapter, gsize flush)
>
>-    if (G_UNLIKELY (g == NULL)) {
>+    gst_queue_array_pop_head (adapter->bufqueue);
>+    if (G_UNLIKELY (gst_queue_array_is_empty (adapter->bufqueue))) {
>       GST_LOG_OBJECT (adapter, "adapter empty now");
>-      adapter->buflist_end = NULL;
>       break;
>     }

I think we might just as well drop the G_UNLIKELY_LIKELY here while we're at it, I don't think they actually do much other than affect code readability.


>@@ -839,10 +835,11 @@ gst_adapter_get_buffer_fast (GstAdapter * adapter, gsize nbytes)
> 
>-  for (item = adapter->buflist; item && left > 0; item = item->next) {
>+  for (idx = 0;
>+      idx < gst_queue_array_get_length (adapter->bufqueue) && left > 0; idx++) {
>     gsize size, cur_size;

The _get_length() should be moved out of the loop here. Get the length once, especially if it doesn't change during the iteration. (And when it changes, update the local var as you go along).

There are multiple places where this applies.

(In reply to Tim-Philipp Müller from comment #2)
> Comment on attachment 370811 [details] [review] [review]
> adapter: port the buffer list from GSList to GstQueueArray
> 
> Nice!
> 
> I think this also makes the code nicer to read in many places.

agreed :D

> 
> Would be good to also run a full make check/valgrind test suite on the
> plugin modules if you haven't done so yet.
> 

Done in -base and -good, check-valgrind shows no regression with this patch

> Couple of nitpicks:
> 
> 
> >@@ -160,7 +160,7 @@ struct _GstAdapter
> > 
> >+  guint scan_entry_idx;
> 
> Maybe add a comment here that G_MAXUINT means invalid/unset.

Done

> 
> 
> >@@ -661,20 +659,18 @@ gst_adapter_flush_unchecked (GstAdapter * adapter, gsize flush)
> >     flush -= size;
> > 
> >     gst_buffer_unref (cur);
> >-    g = g_slist_delete_link (g, g);
> >     --adapter->count;
> > 
> >-    if (G_UNLIKELY (g == NULL)) {
> >+    gst_queue_array_pop_head (adapter->bufqueue);
> 
> I think it would be good to move the pop_head before the buffer unref here,
> and/or even unref the return value directly for clarity. With the current
> code we basically have a dangling pointer in the queue array for a few
> lines. It shouldn't matter of course, but it's nice if it can be avoided and
> will help avoid problems later when people make additional changes.
> 

Done, this indeed didn't feel completely right, tnx

> 
> >@@ -661,20 +659,18 @@ gst_adapter_flush_unchecked (GstAdapter * adapter, gsize flush)
> >
> >-    if (G_UNLIKELY (g == NULL)) {
> >+    gst_queue_array_pop_head (adapter->bufqueue);
> >+    if (G_UNLIKELY (gst_queue_array_is_empty (adapter->bufqueue))) {
> >       GST_LOG_OBJECT (adapter, "adapter empty now");
> >-      adapter->buflist_end = NULL;
> >       break;
> >     }
> 
> I think we might just as well drop the G_UNLIKELY_LIKELY here while we're at
> it, I don't think they actually do much other than affect code readability.

Done, I don't really know how much a positive effect improving branch prediction had here, probably minor.

> 
> 
> >@@ -839,10 +835,11 @@ gst_adapter_get_buffer_fast (GstAdapter * adapter, gsize nbytes)
> > 
> >-  for (item = adapter->buflist; item && left > 0; item = item->next) {
> >+  for (idx = 0;
> >+      idx < gst_queue_array_get_length (adapter->bufqueue) && left > 0; idx++) {
> >     gsize size, cur_size;
> 
> The _get_length() should be moved out of the loop here. Get the length once,
> especially if it doesn't change during the iteration. (And when it changes,
> update the local var as you go along).

Done, I planned to do that while porting then forgot :) The length never changes in any of these places, because the only access wrapped by the for loops is peeking, not pushing or popping.

> 
> There are multiple places where this applies.

Created attachment 370916 [details] [review]
adapter: port the buffer list from GSList to GstQueueArray

Significantly reduces the amount of memory allocation operations.

Attachment 370916 [details] pushed as 4456792 - adapter: port the buffer list from GSList to GstQueueArray