[Bug 745151] videofilter: add sharpening filter

[GStreamer (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

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

--- Comment #21 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 314955:
 --> (https://bugzilla.gnome.org/review?bug=745151&attachment=314955)

Thanks, just some further comments but we're getting there :)

::: gst/videofilters/gstvideobaseconvolvefilter.c
@@ +28,3 @@
+
+#define ALLOWED_CAPS \
+    "{ NV12, NV21, Y444, RGB, xRGB, ARGB }"

If you have support for Y444, you can easily add support for I420/YV12, Y42B,
Y41B, YUV9/YV9 (the planes just have different sizes). Also A420, which just
has one plane more.
If you have support for NV12, you can easily add support for NV16/NV61 and NV24
(just different plane sizes).

If you have support for RGB, you can support BGR and v308, with the same code.
If you have support for ARGB, you can support RGBA, BGRA, ABGR, AYUV, xRGB,
xBGR, BGRx and xBGR with the same code.

You have implementations for GRAY8 below but it's missing from the caps.

@@ +97,3 @@
+
+  switch (input->info.finfo->format) {
+      /* Gray Scale, YUV Data */

You are only running convolution over the Y plane. Why?

@@ +152,3 @@
+
+        default:               /* Not Handled */
+          break;

g_assert_not_reached() ?

@@ +234,3 @@
+          pOutp[index] = outcR;
+          pOutp[index + 1] = outcG;
+          pOutp[index + 2] = outcB;

What about the A channel? And you're here running it over all 3 components,
while for YUV you only did Y.

@@ +356,3 @@
+    return gst_video_base_convolve_RGB (video_filter, input, output);
+  } else if (GST_VIDEO_INFO_IS_GRAY (&input->info)) {
+    return gst_video_base_convolve_GRAY (video_filter, input, output);

Here better a switch over input->info.finfo->format and cases for everything
you explicitly handle

::: gst/videofilters/gstvideobaseconvolvefilter.h
@@ +55,3 @@
+ 
+  /* properties */
+  const GstVideoKernel *kernel; /* filter kernel */

Why const? Wouldn't it be nicer for the subclass if the base class takes
ownership of the kernel and frees it when needed itself?

@@ +65,3 @@
+void gst_video_base_convolution_filter_set_kernel (GstVideoFilter *filter,
const GstVideoKernel *kernel);
+GstFlowReturn
+gst_video_base_convolve (GstVideoFilter *filter, GstVideoFrame * pIn,
GstVideoFrame * pOut);

Why is this public in the header?

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