[Bug 649542] element ranking customization

GStreamer (bugzilla.gnome.org) bugzilla at gnome.org
Mon May 9 00:46:40 PDT 2011


https://bugzilla.gnome.org/show_bug.cgi?id=649542
  GStreamer | gstreamer (core) | 0.10.32

Sebastian Dröge <slomo> changed:

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

--- Comment #6 from Sebastian Dröge <slomo at circular-chaos.org> 2011-05-09 07:46:35 UTC ---
Review of attachment 187346:
 --> (https://bugzilla.gnome.org/review?bug=649542&attachment=187346)

First a general comment, I would prefer a generic "configuration" system over
something that is specialized to changing ranks of elements. I'll still add
some comments about the current implementation though, maybe others disagree
and would like to have a special rank configuration system :)

::: docs/design/draft-customizeranking.txt
@@ +2,3 @@
+--------------------
+
+Base class to change the default element ranking by a custom one.

Not really a base class but that's nitpicking ;)

::: gst/Makefile.am
@@ +17,3 @@
+else
+GST_CUSTOMIZE_RANKING_SRC = gstcustomizeranking.c
+endif

You're not adding the header here, also instead of not including the API when
disabled you should make the API just do nothing. There are only few things
that are worse than an API that changes depending on configure parameters ;)

Also include the header from gst.h

::: gst/gstcustomizeranking.c
@@ +39,3 @@
+G_DEFINE_TYPE (GstCustomizeRankingEntry, gst_customize_ranking_entry,
+    GST_TYPE_OBJECT);
+static GstObjectClass *entry_parent_class = NULL;

G_DEFINE_TYPE does this already, but as
gst_customize_ranking_entry_parent_class. It also sets the pointer in
class_init already

@@ +121,3 @@
+/* customize ranking functions */
+G_DEFINE_TYPE (GstCustomizeRanking, gst_customize_ranking, GST_TYPE_OBJECT);
+static GstObjectClass *parent_class = NULL;

Same comment as above for G_DEFINE_TYPE

@@ +290,3 @@
+  if (custom->file == NULL) {
+    custom->file = g_build_filename (g_get_home_dir (),
+        ".gstreamer-" GST_MAJORMINOR, "custom_ranking." HOST_CPU ".bin",
NULL);

For 0.10 this is fine but for 0.11 please use g_get_system_config_dirs() in
combination with g_get_user_config_dir().

::: gst/gstcustomizeranking.h
@@ +26,3 @@
+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#endif

Don't do this in public headers, it would require that code using gstreamer
also has configure checks for unistd.h, uses the same #define for this, etc.
Stuff from config.h should only ever be used in source files, not headers 
(also you're not including config.h here but that wouldn't make it better)

@@ +55,3 @@
+    /* private fiels */
+    gchar *name;
+    gint rank;

Why not make this a GstMiniObject for example? Or a plain struct? Also, please
add padding to this and the class struct

@@ +66,3 @@
+
+void gst_customize_ranking_entry_set_name (GstCustomizeRankingEntry * entry,
gchar *name);
+void gst_customize_ranking_entry_set_rank (GstCustomizeRankingEntry * entry,
gint rank);

Instead of these two functions maybe just a constructor that sets both fields?
Also you have no constructor at all

@@ +85,3 @@
+    GKeyFile *key_file;
+    gchar *file;
+    GList *entries;

Padding and really GstObject? What about GObject here, I don't think you need
floating refs and all that

::: tools/gst-rank.c
@@ +31,3 @@
+#include <glib/gprintf.h>
+
+#include "gst/gstcustomizeranking.h"

<gst/gstcustomizeranking.h> or even better <gst/gst.h>

-- 
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.


More information about the gstreamer-bugs mailing list