GNOME Bugzilla – Bug 700654
opencv skin colour detection plugin
Last modified: 2013-05-23 09:31:29 UTC
I'm trying to add a skin-colour detection plugin element to the opencv elements in gst-plugins-bad. My first bug so please whoever reviews this be very careful :) The input is an RGB image and the output is an RGB black and white with white being skin-colour identified pixels and black the rest. Originally it was based on an RGBA input to RGBA, perhaps some of this remains although I tried cleaning it up.
Created attachment 244730 [details] [review] full patch with the proposed skindetect opencv element Contains gstskindetect.{c,h} and a tiny modification to Makefile.am and gstopencv.c
Review of attachment 244730 [details] [review]: Why isn't this based on GstVideoFilter or at least GstBaseTransform? Same goes for many other of the OpenCV elements... ::: ext/opencv/gstskindetect.c @@ +157,3 @@ + (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS))); + g_object_class_install_property (gobject_class, PROP_ENABLE, + g_param_spec_boolean ("enable", "Enable", For the other OpenCV filters there is no such property @@ +162,3 @@ + (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS))); + g_object_class_install_property (gobject_class, PROP_METHOD, + g_param_spec_int ("method", Please use a GEnum type here and g_param_spec_enum() @@ +305,3 @@ + + buf = gst_buffer_make_writable (buf); + gst_buffer_map (buf, &in_info, GST_MAP_WRITE); Should be GST_MAP_READ as this is only read, not written to. @@ +318,3 @@ + } + } + ////////////////////////////////////////////////////////////////////////////// Please use C comments (/* blabla */)
(In reply to comment #2) > Review of attachment 244730 [details] [review]: > > Why isn't this based on GstVideoFilter or at least GstBaseTransform? Same goes > for many other of the OpenCV elements... > I made it based on GstOpencvVideoFilter, which in turn is based on GstVideoFilter. Before, I started with a copy of edgedetect, which was not based on any VideoFilter, hence my mistake. > ::: ext/opencv/gstskindetect.c > @@ +157,3 @@ > + (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS))); > + g_object_class_install_property (gobject_class, PROP_ENABLE, > + g_param_spec_boolean ("enable", "Enable", > > For the other OpenCV filters there is no such property Originally it was there to solve large pipeline caps negotiation problems, but I removed it indeed. > > @@ +162,3 @@ > + (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS))); > + g_object_class_install_property (gobject_class, PROP_METHOD, > + g_param_spec_int ("method", > > Please use a GEnum type here and g_param_spec_enum() > Not really needed, I put them as #define's. > @@ +305,3 @@ > + > + buf = gst_buffer_make_writable (buf); > + gst_buffer_map (buf, &in_info, GST_MAP_WRITE); > > Should be GST_MAP_READ as this is only read, not written to. > > @@ +318,3 @@ > + } > + } > + > ////////////////////////////////////////////////////////////////////////////// > > Please use C comments (/* blabla */) Done.
Created attachment 244859 [details] [review] Second full patch with skindetect based on opencvvideofilter etc.
Review of attachment 244859 [details] [review]: ::: ext/opencv/gstskindetect.c @@ +3,3 @@ + * Copyright (C) 2005 Thomas Vander Stichele <thomas@apestaart.org> + * Copyright (C) 2005 Ronald S. Bultje <rbultje@ronald.bitfreak.net> + * Copyright (C) 2008 Michael Sheldon <mike@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?
Created attachment 244988 [details] [review] Third full patch with skindetect based on opencvvideofilter etc. Notice the manually inlined previous functions for calculation of skin colour in HSV and RGB.
Sorry, I noticed I remove the previous copyrights when adding mine, I'll re-add them in the evening. I took the rest of the comments into account and added/modified them.
Review of attachment 244988 [details] [review]: Almost there :) ::: ext/opencv/gstskindetect.c @@ +1,3 @@ +/* + * GStreamer + * Copyright (C) 2013 Miguel Casas-Sanchez <miguelecasassanchez@gmail.com> Just having your copyright here is fine, I don't see any code copied from elsewhere except boilerplate code @@ +95,3 @@ + static const GEnumValue values[] = { + {HSV, "Classic HSV thresholding", "HSV"}, + {RGB, "RGB-colorspace thresholds", "RGB"}, The last elements here ("HSV", "RGB") should be lower-case for consistency with other code @@ +205,3 @@ + filter->cvGp2 = NULL; + filter->cvSkinPixels2 = NULL; + filter->cvdraft = NULL; Setting all this to NULL is not necessary, GObject does that for you already @@ +222,3 @@ + break; + case PROP_METHOD: + filter->method = g_value_get_int (value); g_value_get_enum() @@ +241,3 @@ + break; + case PROP_METHOD: + g_value_set_int (value, filter->method); g_value_set_enum() @@ +259,3 @@ + CvSize size = cvSize (in_width, in_height); + + filter->cvRGB = cvCreateImageHeader (size, IPL_DEPTH_8U, 3); You need to check here if these images already exist, in if so free them before creating new ones. The caps can change at any time
Created attachment 245078 [details] [review] Fourth full patch Addressed all comments from before. "Display" parameter changed to "postprocess" which is actually what it does :) it tries to blob skin patches together by using morphological open-close operations.
commit ac4efd2914df0b23144ffba5644f354a962e24ec Author: Miguel Casas-Sanchez <miguelecasassanchez@gmail.com> Date: Thu May 23 11:04:57 2013 +0200 opencv: Add skin color detection element https://bugzilla.gnome.org/show_bug.cgi?id=700654