[Bug 795167] adapter: port the buffer list from GSList to GstQueueArray

[GStreamer (GNOME Bugzilla)]

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

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #370811|none                        |needs-work
             status|                            |

--- Comment #2 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 370811
  --> https://bugzilla.gnome.org/attachment.cgi?id=370811
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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.