[Bug 745151] videofilter: Added a new plugin to enhance video in NV format

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=745151

Edward Hervey <bilboed at bilboed.com> changed:

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

--- Comment #6 from Edward Hervey <bilboed at bilboed.com> ---
Review of attachment 297960:
 --> (https://bugzilla.gnome.org/review?bug=745151&attachment=297960)

Definitely something nice, but still some cleanup/fixes to be done. The
convolve logic could later be generalized quite easily.

BTW, Looks like there's a stray file in your patch (second file in the patch).

::: gst/videofilters/gstconvolve.c
@@ +35,3 @@
+  guint8 *pData;
+  guint8 *pOutp;
+

Shouldn't we also check that the kernel width/height is odd-numbered ?
Otherwise it makes no sense whatsoever (what would the 'middle' point of the
kernel be ?)

@@ +36,3 @@
+  guint8 *pOutp;
+
+  if ((pInput == NULL) || (pOutput == NULL) || (pFilter == NULL))

This should never really happen, I'd use a g_return_val_if_fail() instead (so
it shows it in big when debugging and can also be trapped with
G_DEBUG=fatal_warnings)

@@ +38,3 @@
+  if ((pInput == NULL) || (pOutput == NULL) || (pFilter == NULL))
+    return GST_FLOW_ERROR;
+

Another check that could be done is ensuring the input and output frames are of
identical size/type/stride/...

@@ +46,3 @@
+  h = pInput->info.height;
+
+  if (w < fw)

might want to check the same thing for height also ?

@@ +57,3 @@
+      for (i = 0; i < h; i++) {
+        for (j = 0; j < w; j++) {
+          fw = pFilter->w;

No need to repeat the fw/fh assignments, they are done at the top of the
function and never change

@@ +62,3 @@
+          inc = 0;
+          for (k = 0; k < fh; k++) {
+            if ((i < ((fh + 1) / 2)) || ((i + k) >= (h - 1)))

Those fh+1/2 and fw+1/2 should be put as a global variable for this function.
It never changes. And it could be named middle_point_x (or something else more
explicit)

@@ +80,3 @@
+            cnt = 1;
+          outc = (inc / cnt);
+          index = (i * w + j);

that index calculation is assuming w == stride.

@@ +81,3 @@
+          outc = (inc / cnt);
+          index = (i * w + j);
+          outc = outc > 255 ? 255 : (outc < 0) ? 0 : outc;

Just use the CLAMP() glib macro :)

::: gst/videofilters/gstsharpcontrast.c
@@ +20,3 @@
+ * SECTION:element-gstsharpcontrast
+ *
+ * Enhances the input frame

"Increases the contrast and sharpens the image" as opposed to "enhance" ?

@@ +54,3 @@
+/* pad templates */
+#define VIDEO_CAPS GST_VIDEO_CAPS_MAKE( \
+    "{ NV12, NV21, RGB }")

Just remove RGB from the caps template if you don't support it

@@ +93,3 @@
+}
+
+static gboolean

Remove the start/stop implementations if they don't do anything

@@ +165,3 @@
+    for (j = 0; j < w; j++) {
+      val = (pData[index] - min) * 255 / (max - min);
+      pOutp[index] = (val > 255) ? 255 : val;

Just use the MAX() macro instead

@@ +180,3 @@
+{
+  GstEnhance *sharpcontrast = GST_ENHANCE (filter);
+  Kernel kernel;

Maybe that kernel can just be a global static const structure (since it doesn't
change) ?

::: gst/videofilters/gstsharpcontrast.h
@@ +25,3 @@
+
+G_BEGIN_DECLS
+#define GST_TYPE_ENHANCE   (gst_sharpcontrast_get_type())

You forgot to rename a few more macros and structure names here (from enhance
to sharp contrast)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.