Created attachment 297861 [details] [review]
videofilter: Added a new plugin to enhance video in NV format

Attaching the Patch

Perhaps we could find a less generic name than 'enhance' for this element?

Please don't use C++ comments (// foo) in the code. And it would be good if you could either test the RGB path as well, or remove the code and add it later once it's been tested.

Thanks for the comments.
I will change the comment style and remove RGB for now.

Can you please suggest a name to change from 'enhance' ?

Few other options I could think are
normalize / autocorrect

(In reply to Sanjay NM from comment #3)
> Few other options I could think are
> normalize / autocorrect

This is has generic or even more as the previous one. Usually filters try to reflect the media type (video vs audio) and the actual action being done (type of enhancement).

Created attachment 297960 [details] [review]
videofilter: sharpcontrast Filter to improve visual quality of video

Thanks for the inputs.

I have updated the patch by

1. Removing unused RGB code, which could be reintroduced later when its supported
2. Changed comment to /* */ style from //
3. Updated the name of the plugin to sharpcontrast as this patch is doing a contrast stretching to use the complete dynamic Range and then doing bit of sharpening.

I am ok to change the name to any that is suggested.

Review of attachment 297960 [details] [review]:

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)

Created attachment 302626 [details] [review]
videofilter: Added sharpen filter based on convolution

Added convolution based sharpen filter.
This supports NV and RGB format

Have taken care of all the review comments given.
Removed contrast stretch as this is not really based on convolution.

Will make a separate patch for it later (as an enhancement filter)

Request to please review. Based on this patch's acceptance other pending patches will be resubmitted along with new filters :-)

Review of attachment 302626 [details] [review]:

Definitely cleaner :) But still some issues.

::: gst/videofilters/gstsharpen.c
@@ +112,3 @@
+  GstVideoSharpenFilter *sharpen = GST_VIDEO_SHARPEN_FILTER (video_filter);
+
+  g_mutex_lock (&sharpen->lock);

This lock is not needed. GstVideoFilter::set_info() is only called when caps are being set in the parent class, and that's guaranteed to be atomic.

::: gst/videofilters/gstvideobaseconvolvefilter.c
@@ +77,3 @@
+  GstVideoKernel *kernel = self->kernel;
+
+  g_mutex_lock (&self->lock);

That lock isn't needed either (and wasn't initialized!). the filter transform method is guaranteed to be called atomically within one filter instance.

@@ +84,3 @@
+  if (kernel == NULL) {
+    g_mutex_unlock (&self->lock);
+    return GST_FLOW_ERROR;

Whenever you return a GST_FLOW_ERROR, please include a GST_ERROR_OBJECT() debugging message so one can trace the issues. And in general, you should put a bit more debugging so people can trace what's going on.

::: gst/videofilters/gstvideobaseconvolvefilter.h
@@ +65,3 @@
+struct _GstVideoBaseConvolveFilterClass {
+  GstVideoFilterClass parent_class;
+  gboolean (*set_kernel)(GstVideoKernel **kernel);

Not needed/used anywhere ?

@@ +69,3 @@
+
+GType gst_video_base_convolve_filter_get_type (void);
+void gst_video_base_convolve_filter_set_kernel (GstVideoKernel *kernel);

This method is not implemented anywhere ?

It would be nice to have it implemented and take a "const GstVideoKernel *kernel". That method would copy the provided argument.

Like that, the caller (subclass) could just have a local static kernel (no need to do copy/free in the subclass), and call the method with _set_kernel(convolve, &mykernel);

Created attachment 302645 [details] [review]
videofilter: Added support for convolution based sharpen filter

Thanks for the quick review.
Incorporated review comments.

This supports convolution based sharpen filter which works for NV, RGB format

Review of attachment 302645 [details] [review]:

Apart from that small issue, looks good to commit imho

::: gst/videofilters/gstsharpen.c
@@ +91,3 @@
+gst_video_sharpen_filter_init (GstVideoSharpenFilter * self)
+{
+  g_mutex_init (&self->lock);

still not needed

Created attachment 302977 [details] [review]
videofilter: Added support for convolution based sharpening filter

As per the review comment, removed mutex from sharpening filter.

Created attachment 303341 [details] [review]
videofilters: Added convolution based sharpen filter

Added support for Sharpening which is convolution based.
Currently it is implemented and tested for NV and RGB data
Below are the pipelines used for testing

gst-launch-1.0 filesrc location=pm.mp4 ! decodebin ! videoconvert ! sharpen ! videoconvert ! ximagesink
gst-launch-1.0 filesrc location=pm.mp4 ! decodebin ! videoconvert ! "video/x-raw, format={RGB}" ! sharpen ! videoconvert ! ximages

Fixed an issue in RGB format index calculation. 

Request to please review the patch

Request to please review this patch.

I have few more patches including filters and special effects which depend on this patch.

Few more patches which are already submitted and are for review are:

746006: videofilters: Added filter to remove speckle noise
746007: videofilters: Added support for BoxBlur filter 	
746008: videofilters: Added support for convolution based motionblur filter 
746009: videofilters: Added support for Laplacian filter

Review of attachment 303341 [details] [review]:

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

I will make the changes and resubmit it soon

Created attachment 309131 [details] [review]
videofilters: Added support for convolution based sharpen filter

Incorporated all review comments except color format comment.

There are few more patches pending on this original patch.
Request to please review this. Once this patch is done then I will make the color format generic as per the suggestion and also will resubmit the other pending patches.

Review of attachment 309131 [details] [review]:

Can you split this into two patches, one to add the base class and one for the sharpen filter?

::: gst/videofilters/gstsharpen.c
@@ +89,3 @@
+    -1, -1, -1,
+    -1, 18, -1,
+    -1, -1, -1

Should the "strength" of the filter be configurable by property?

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

You can easily support all other RGB/xRGB/ARGB (in all orders) formats, and also I420/Y444/Y42B/Y41B/YV12. Can you add support for these?

@@ +62,3 @@
+GstFlowReturn
+gst_video_base_convolve (GstVideoFilter * video_filter, GstVideoFrame * pInput,
+    GstVideoFrame * pOutput)

We use a different style to name variables: lower-case, underscore separated words and the type (p / pointer) is not part of the name

@@ +109,3 @@
+  }
+
+  switch (pInput->info.finfo->format) {

Try to move the format specific processing code into separate functions

@@ +112,3 @@
+      /* Gray Scale, YUV Data */
+    case GST_VIDEO_FORMAT_NV12:
+    case GST_VIDEO_FORMAT_NV21:

The two planes might have different strides

@@ +143,3 @@
+      /* Get Pointer for UV plane and memcpy remaining data */
+      pData = (guint8 *) pInput->data[1];
+      pOutp = (guint8 *) pOutput->data[1];

Use the accessor macros (here and elsewhere) to access the fields of the GstVideoFrame

Created attachment 314905 [details] [review]
videofilters: Added Convolution support

Supported RGB color formats
Incorporated review comments on
- Variables
- Macros
- Adding color models
- Splitting into separate patch

Once this patch is through then will resubmit all other patches
Request to please review

Review of attachment 314905 [details] [review]:

Minor formatting comments

::: gst/videofilters/gstvideobaseconvolvefilter.c
@@ +106,3 @@
+          inc = 0;
+          for (k = 0; k < fh; k++) {
+            if ((i < fh_mid) || ((i + k) >= (h - 1)))

Drop the unneeded parenthesis

@@ +107,3 @@
+          for (k = 0; k < fh; k++) {
+            if ((i < fh_mid) || ((i + k) >= (h - 1)))
+              index = (i * stride + j);

ditto

@@ +114,3 @@
+              fIndex = k * kernel->w + l;
+              inc += (pData[index + c] * kernel->va[fIndex]);
+              cnt += (kernel->va[fIndex]);

ditto

@@ +206,3 @@
+          incR = incG = incB = 0;
+          for (k = 0; k < fh; k++) {
+            if ((i < fh_mid) || ((i + k) >= (h - 1)))

here too

@@ +207,3 @@
+          for (k = 0; k < fh; k++) {
+            if ((i < fh_mid) || ((i + k) >= (h - 1)))
+              index = (i * stride + j * components);

ditto

@@ +215,3 @@
+              incR += (pData[index + c] * kernel->va[fIndex]);
+              incG += (pData[index + c + 1] * kernel->va[fIndex]);
+              incB += (pData[index + c + 2] * kernel->va[fIndex]);

again

@@ +217,3 @@
+              incB += (pData[index + c + 2] * kernel->va[fIndex]);
+              cnt += (kernel->va[fIndex]);
+              if ((j < fw_mid) || ((j + l) >= (w - 1)))

and again

@@ +227,3 @@
+          outcR = (incR / cnt);
+          outcG = (incG / cnt);
+          outcB = (incB / cnt);

and again

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

yet another time

@@ +283,3 @@
+          index = (i * stride + j);
+        else
+          index = ((i + k) * stride + j);

again

@@ +289,3 @@
+          inc += (pData[index + c] * kernel->va[fIndex]);
+          cnt += (kernel->va[fIndex]);
+          if ((j < fw_mid) || ((j + l) >= (w - 1)))

again

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

feel free to drop the braces for single statements

Created attachment 314955 [details] [review]
videofilters: Added support for convolution

Removed unwanted braces

Review of attachment 314955 [details] [review]:

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?

Thanks for the review.

I will make the changes for the possible color formats and other review comments and resubmit.

I have avoided running convolution on U, V intentionally as YUV, YCbCr is not absolute color space. So my concern is running convolution on that may impact the colors negatively.

If you suggest to include them then I will include them in the processing.

Maybe it should be a property then if U and V should be considered too. If you only consider Y, then the effect of the filter will be different depending on the input format. RGB formats would affect all components, YUV formats only the Y component.

Ok. I was thinking on the same lines too.
I will add a property which will decide whether U, V should be used or not

I think ideally the result of the filter with the same matrix should be the same (except for rounding errors) independent of the input format.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/214.