[gstreamer-bugs] [Bug 599018] [PATCH] Cleanups and fixes for libcaca plugin

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=599018
  GStreamer | gst-plugins-good | git

--- Comment #4 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2009-10-22 01:16:58 UTC ---
Review of attachment 145856:
 --> (https://bugzilla.gnome.org/review?bug=599018&attachment=145856)

::: ext/libcaca/gstcacasink.c
@@ +149,3 @@
+
+  list = caca_get_display_driver_list ();
+

Why is this code in the _get_type() function? You're basically rebuildilng the
driver list on every type check, so possibly even multiple times for each
buffer that arrives. Is that really what you want?

But even if, this needs locking really, so that two different threads calling
_get_type() don't trample over each other.

If the driver list is assumed to be static during the runtime of the program,
you could just put a _build_driver_list() call into the plugin_init function or
so, or call it from class_init.

@@ +161,3 @@
+    driver[i].value_nick = g_strdup (list[2 * i]);
+    driver[i].value_name = g_strdup (list[2 * i + 1]);
+  }

Since you're buildling an enum table here: will the drivers always be
enumerated with the same integer/enum values, or only be addressed by string
nick/name? If the latter, then maybe this should be a string property instead,
since the assumption for enum properties is that you could theoretically
hard-code numbers and still always get what you expect.

@@ +358,3 @@
+    case ARG_DRIVER:{
+      cacasink->driver = g_value_get_enum (value);
+    }

A break here woudl be nice, even if it's not stricly needed.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.