[Bug 700977] Add colour image enhancement element based on Retinex algorithm

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #1 from Sebastian Dröge <slomo at circular-chaos.org> 2013-05-25 13:21:40 UTC ---
Review of attachment 245277:
 --> (https://bugzilla.gnome.org/review?bug=700977&attachment=245277)

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

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