[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 #303341|none                        |needs-work
             status|                            |

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

::: gst/videofilters/gstsharpen.c
@@ +113,3 @@
+  sharp_kernel.h = 3;
+  sharp_kernel.va = sharp;
+  gst_video_base_convolve_filter_set_kernel (video_filter, &sharp_kernel);

This is independent of the info, right? Why not just set it in
gst_video_sharpen_filter_init() then?

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

That's a weird set of default video formats :)

Can you abstract this a bit? E.g. all RGB(A/X) variants should be possible to
write once and then automatically generate variants via macros for the specific
order. For all planar formats you should be able to just have a single
implementation, for the semi-planar formats another one

@@ +94,3 @@
+
+  pData = (guint8 *) pInput->data[0];
+  pOutp = (guint8 *) pOutput->data[0];

This is not correcet for NV12/NV21. You need to get the pointer to the second
plane explicitly, it's not guaranteed to be right after the first plane. Mighjt
be a completely different memory region.

@@ +102,3 @@
+  }
+
+  stride = pInput->info.stride[0];

The stride can be different per plane too, also should consider looking at the
component_width/height for multiplanar formats

::: gst/videofilters/gstvideobaseconvolvefilter.h
@@ +39,3 @@
+
+
+typedef struct _gstVideoKernel

Uppercast G :)

@@ +44,3 @@
+  gint h; /* Kernel Height */
+  gint *va; /* Kernel Values */
+}GstVideoKernel;

GstVideoConvolutionKernel?

@@ +47,3 @@
+
+/**
+ * GstVideoBaseConvolveFilter:

GstVideoBaseConvolutionFilter, not Convolve

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