[Bug 700654] opencv skin colour detection plugin

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=700654
  GStreamer | gst-plugins-bad | 1.x

Sebastian Dröge <slomo> changed:

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

--- Comment #5 from Sebastian Dröge <slomo at circular-chaos.org> 2013-05-21 07:02:59 UTC ---
Review of attachment 244859:
 --> (https://bugzilla.gnome.org/review?bug=700654&attachment=244859)

::: ext/opencv/gstskindetect.c
@@ +3,3 @@
+ * Copyright (C) 2005 Thomas Vander Stichele <thomas at apestaart.org>
+ * Copyright (C) 2005 Ronald S. Bultje <rbultje at ronald.bitfreak.net>
+ * Copyright (C) 2008 Michael Sheldon <mike at mikeasoft.com>

You probably want to add your own copyright notice there and remove these (I
don't see any code you copied from elsewhere except boilerplate code)

@@ +52,3 @@
+ * <title>Example launch line</title>
+ * |[
+ * gst-launch-0.10 videotestsrc ! decodebin ! videoconvert ! skindetect !
videoconvert ! xvimagesink

gst-launch-1.0

@@ +85,3 @@
+
+#define  METHOD_HSV 0
+#define  METHOD_RGB 1

Make this a C enum and register a GEnum GType for that
https://developer.gnome.org/gobject/unstable/gobject-Enumeration-and-Flag-Types.html

That allows to introspect the possible values at runtime, which is not possible
with an integer

@@ +119,3 @@
+    cvReleaseImage (&filter->cvRGB);
+    cvReleaseImage (&filter->cvSkin);
+    cvReleaseImage (&filter->cvChA);

Ideally this should be in GstBaseTransform::stop() and not in
GObject::finalize().

@@ +148,3 @@
+          (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)));
+  g_object_class_install_property (gobject_class, PROP_METHOD,
+      g_param_spec_int ("method",

And here then use g_param_spec_enum() for the reasons mentioned above

@@ +230,3 @@
+  GstSkinDetect *filter = GST_SKIN_DETECT (base);
+
+  if (filter->cvRGB == NULL) {

You should do this initialization (and freeing of the images if already there)
in the GstCvVideoFilter::cv_set_caps() method. See gsthanddetect.c for an
example

If you do it in transform you won't be able to detect caps changes (e.g.
different width/height) during runtime.

@@ +376,3 @@
+
+  IplImage *imageSkinPixels = cvCreateImage (cvGetSize (imageRGB),
IPL_DEPTH_32F, 1);   /*  Greyscale output image. */
+  IplImage *draft = cvCreateImage (cvGetSize (imageRGB), IPL_DEPTH_8U, 1);    
 /*  Greyscale output image. */

Would it be possible to cache all this many images and only create them
together with cvRGB and the other two?

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