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

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

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);

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