GNOME Bugzilla – Bug 700977
opencv: add colour image enhancement element based on Retinex algorithm
Last modified: 2013-07-09 09:19:47 UTC
Created attachment 245277 [details] [review] Retinex element full patch. Add colour image enhancement element based on Retinex algorithm. Two types exist, namely basic and multiscale; both are described in this article: Rahman, Zia-ur, Daniel J. Jobson, and Glenn A. Woodell. "Multi-scale retinex for color image enhancement." Image Processing, 1996. Proceedings., International Conference on. Vol. 3. IEEE, 1996 Visually speaking the result looks a bit funny, but is pretty invariable to lightning changes, which is good for some applications, like image segmentation. I hope I learned enough in previous upload (skin detection) to get a better quality patch this time :)
Review of attachment 245277 [details] [review]: ::: ext/opencv/gstretinex.c @@ +104,3 @@ +} + +G_DEFINE_TYPE (GstRetinex, gst_retinex, GST_TYPE_VIDEO_FILTER); Why plain GstVideoFilter and not GstOpenCvVideoFilter? @@ +179,3 @@ +{ + filter->method = METHOD_BASIC; + filter->scales = 3; Above you said default is 4, but you initialize to 3. Best is to create some #defines for the default values at the very top @@ +250,3 @@ + if (retinex->scales != 0) { + retinex->weights = (double *) malloc (sizeof (double) * retinex->scales); + retinex->sigmas = (double *) malloc (sizeof (double) * retinex->scales); These are not freed anywhere. Should probably be handled separately from set_caps In transform you could check if the value of retinex->scales has changed since last time, and if so free() the memory and reallocate. And in stop always free the memory. @@ +298,3 @@ + + if (TRUE != gst_buffer_map (buf, &info, GST_MAP_READWRITE)) + return GST_FLOW_ERROR; This error path leaks the outbuf you've mapped above already. Also you probably want to map the outbuf WRITE-only and the inbuf READ-only? Or do in-place transformation as your result is in RGBin anyway
New patch coming. (In reply to comment #1) > Review of attachment 245277 [details] [review]: > > ::: ext/opencv/gstretinex.c > @@ +104,3 @@ > +} > + > +G_DEFINE_TYPE (GstRetinex, gst_retinex, GST_TYPE_VIDEO_FILTER); > > Why plain GstVideoFilter and not GstOpenCvVideoFilter? I have developed this in the (near) past and used VideoFilter. Should I change it? > > @@ +179,3 @@ > +{ > + filter->method = METHOD_BASIC; > + filter->scales = 3; > > Above you said default is 4, but you initialize to 3. Best is to create some > #defines for the default values at the very top Indeed, done. > > @@ +250,3 @@ > + if (retinex->scales != 0) { > + retinex->weights = (double *) malloc (sizeof (double) * retinex->scales); > + retinex->sigmas = (double *) malloc (sizeof (double) * retinex->scales); > > These are not freed anywhere. Should probably be handled separately from > set_caps > > In transform you could check if the value of retinex->scales has changed since > last time, and if so free() the memory and reallocate. And in stop always free > the memory. > stop() releases them, and then I allocated via realloc() in set_property(), since any change of this parameter should go via this function, as far as I know. > @@ +298,3 @@ > + > + if (TRUE != gst_buffer_map (buf, &info, GST_MAP_READWRITE)) > + return GST_FLOW_ERROR; > > This error path leaks the outbuf you've mapped above already. Also you probably > want to map the outbuf WRITE-only and the inbuf READ-only? Or do in-place > transformation as your result is in RGBin anyway Changed to in_place and hopefully sorted out.
Created attachment 245300 [details] [review] Second iteration full retinex as patch Comments addressed - except VideoFilter vs OpencvVideoFilter.
Review of attachment 245300 [details] [review]: ::: ext/opencv/gstretinex.c @@ +184,3 @@ + filter->method = DEFAULT_METHOD; + filter->scales = DEFAULT_SCALES; + gst_base_transform_set_in_place (GST_BASE_TRANSFORM (filter), FALSE); Should be TRUE, not FALSE @@ +206,3 @@ + retinex->sigmas = + (double *) realloc (retinex->sigmas, + sizeof (double) * retinex->scales); This all needs locking then, as set_property() can be called while transform() is called. That's why I proposed to just do the allocation/initialization in transform() if the scales value has changed Best also to use g_realloc() here for portability @@ +277,3 @@ + + free (filter->weights); + free (filter->sigmas); Use g_free(), and set to NULL after freeing @@ +304,3 @@ + int filter_size; + + if (TRUE != gst_buffer_map (buf, &info, GST_MAP_READWRITE)) { Don't do comparisons for boolean checks, just "if (foo)" and "if (!foo)"
(In reply to comment #2) > New patch coming. > > (In reply to comment #1) > > Review of attachment 245277 [details] [review] [details]: > > > > ::: ext/opencv/gstretinex.c > > @@ +104,3 @@ > > +} > > + > > +G_DEFINE_TYPE (GstRetinex, gst_retinex, GST_TYPE_VIDEO_FILTER); > > > > Why plain GstVideoFilter and not GstOpenCvVideoFilter? > I have developed this in the (near) past and used VideoFilter. Should I change > it? I don't know, I assume GstOpenCvVideoFilter already handles some OpenCv related things for you and thus makes your life easier and the code shorter :) Does it?
(In reply to comment #5) > (In reply to comment #2) > > New patch coming. > > > > (In reply to comment #1) > > > Review of attachment 245277 [details] [review] [details] [details]: > > > > > > ::: ext/opencv/gstretinex.c > > > @@ +104,3 @@ > > > +} > > > + > > > +G_DEFINE_TYPE (GstRetinex, gst_retinex, GST_TYPE_VIDEO_FILTER); > > > > > > Why plain GstVideoFilter and not GstOpenCvVideoFilter? > > I have developed this in the (near) past and used VideoFilter. Should I change > > it? > > I don't know, I assume GstOpenCvVideoFilter already handles some OpenCv related > things for you and thus makes your life easier and the code shorter :) Does it? Mmm perhaps but I'm not fully familiar with that "class", f.i. in line 210, transform_ip, the input buffer is mapped and the return of the function is not checked. That said, in my previous patch for skin detection I used it :) In other words, you're more experienced than me with Gstreamer so I'll take your recommendation.
(In reply to comment #4) > Review of attachment 245300 [details] [review]: > > ::: ext/opencv/gstretinex.c > @@ +184,3 @@ > + filter->method = DEFAULT_METHOD; > + filter->scales = DEFAULT_SCALES; > + gst_base_transform_set_in_place (GST_BASE_TRANSFORM (filter), FALSE); > > Should be TRUE, not FALSE > ok done > @@ +206,3 @@ > + retinex->sigmas = > + (double *) realloc (retinex->sigmas, > + sizeof (double) * retinex->scales); > > This all needs locking then, as set_property() can be called while transform() > is called. That's why I proposed to just do the allocation/initialization in > transform() if the scales value has changed > I didn't know that =-0, I moved then the allocation to the transform function, but then I added a static int to keep track of the last "scales" value, to compare with the one from the filter struct.. > Best also to use g_realloc() here for portability > > @@ +277,3 @@ > + > + free (filter->weights); > + free (filter->sigmas); > > Use g_free(), and set to NULL after freeing > done. > @@ +304,3 @@ > + int filter_size; > + > + if (TRUE != gst_buffer_map (buf, &info, GST_MAP_READWRITE)) { > > Don't do comparisons for boolean checks, just "if (foo)" and "if (!foo)" done too. New patch on its way!
Created attachment 245347 [details] [review] Third iteration full gstretinex Comments from Sebastian addressed.
Created attachment 245348 [details] [review] Rebased third retinex element full patch Forgot to rebase :)
Review of attachment 245348 [details] [review]: ::: ext/opencv/gstretinex.c @@ +292,3 @@ + int offset = 128; + int filter_size; + static int scales = 0; That's not going to work if there are multiple instances of the element. Store it inside the instance struct (e.g. current_scales for the currently used value, scales for the value of the property).
(In reply to comment #6) > (In reply to comment #5) > > I don't know, I assume GstOpenCvVideoFilter already handles some OpenCv related > > things for you and thus makes your life easier and the code shorter :) Does it? > > Mmm perhaps but I'm not fully familiar with that "class", f.i. in line 210, > transform_ip, the input buffer is mapped and the return of the function is not > checked. That said, in my previous patch for skin detection I used it :) In > other words, you're more experienced than me with Gstreamer so I'll take your > recommendation. I might know more about GStreamer, but definitely not about OpenCV and the OpenCV plugin... especially not about the base class :) What do you think from looking at it, would it do some things for you that you currently have to do yourself? Like mapping the GstBuffer to an OpenCV image, or in set_caps do the mapping of caps to OpenCV things?
Created attachment 245354 [details] [review] Fourth full retinex patch changed the static gint scales with filter->current_scales.
(Added a minor fix, checking for retinex->scales being NULL too for deciding if it needs to be reallocated) commit 407f3e1856e3d7e79c9d743e4ae92538b6dad290 Author: Miguel Casas-Sanchez <miguelecasassanchez@gmail.com> Date: Mon May 27 11:20:07 2013 +0200 opencv: Add colour image enhancement element based on Retinex algorithm Add colour image enhancement element based on Retinex algorithm. Two types exist, namely basic and multiscale; both are described in this article: Rahman, Zia-ur, Daniel J. Jobson, and Glenn A. Woodell. "Multi-scale retinex for color image enhancement." Image Processing, 1996. Proceedings., International Conference on. Vol. 3. IEEE, 1996 Visually speaking the result looks a bit funny, but is pretty invariable to lightning changes, which is good for some applications, like image segmentation. https://bugzilla.gnome.org/show_bug.cgi?id=700977