[Bug 746209] Merge glfiltersobel, glfilterblur and glfilterlaplacian into gleffects

[GStreamer (GNOME Bugzilla)]

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

Julien Isorce <julien.isorce at gmail.com> changed:

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

--- Comment #23 from Julien Isorce <julien.isorce at gmail.com> ---
Review of attachment 301015:
 --> (https://bugzilla.gnome.org/review?bug=746209&attachment=301015)

Looks good, nice patch! Just minor remarks.

::: ext/gl/gstgleffects.c
@@ +314,1 @@
   gobject_class = (GObjectClass *) klass;

error: variable 'gobject_class' set but not used
[-Werror=unused-but-set-variable]

@@ +562,3 @@
+
+static const GstGLEffectsFilterDescriptor *
+gst_gl_effects_filters_supported_properties ()

error: old-style function definition [-Werror=old-style-definition]
-> (void)

@@ +564,3 @@
+gst_gl_effects_filters_supported_properties ()
+{
+  // Horizontal swap property is supported by all filters

Use /* */

@@ +577,3 @@
+    * descriptor, gint property)
+{
+  // generic filter (NULL descriptor) supports all properties

Use /* */

@@ +582,3 @@
+
+static const GstGLEffectsFilterDescriptor *
+gst_gl_effects_filters_descriptors ()

error: old-style function definition [-Werror=old-style-definition]
-> (void)

@@ +645,3 @@
+            g_strdup_printf ("GstGLEffects-%s", filters->filter_name);
+        gchar *element_name =
+            g_strdup_printf ("gleffects-%s", filters->filter_name);

You can put the same name for type and element (it is the case for avdec_h264
for example).
Also use "_" instead of "-".

::: ext/gl/gstgleffects.h
@@ +95,1 @@
 };

It should go to gstgleffects.c

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