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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #3 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
(In reply to Tim-Philipp Müller from comment #2)
> 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.

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.

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