After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 746209 - Merge glfiltersobel, glfilterblur and glfilterlaplacian into gleffects
Merge glfiltersobel, glfilterblur and glfilterlaplacian into gleffects
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 745955
Blocks:
 
 
Reported: 2015-03-14 14:27 UTC by Michał Dębski
Modified: 2015-04-21 12:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
merge_filters_into_effects.patch (76.60 KB, patch)
2015-03-14 14:27 UTC, Michał Dębski
none Details | Review
0001-Fix-fisheye-shader-pass-float-to-sqrt (989 bytes, patch)
2015-03-22 11:40 UTC, Michał Dębski
none Details | Review
0002-gleffects-Correct-attributes-for-glow-shader (1.50 KB, patch)
2015-03-22 11:41 UTC, Michał Dębski
none Details | Review
0003-gleffects-Merge-blur-filter-into-effects (43.26 KB, patch)
2015-03-22 11:42 UTC, Michał Dębski
none Details | Review
0004-gleffects-Merge-sobel-filter-into-effects (22.97 KB, patch)
2015-03-22 11:42 UTC, Michał Dębski
none Details | Review
0005-gleffects-Merge-laplacian-filter-into-effects (17.63 KB, patch)
2015-03-22 11:43 UTC, Michał Dębski
none Details | Review
0001-Fix-fisheye-shader-pass-float-to-sqrt (996 bytes, patch)
2015-04-06 12:30 UTC, Michał Dębski
committed Details | Review
0002-gleffects-Correct-attributes-for-hconv-and-vconv-shader (2.58 KB, patch)
2015-04-06 12:32 UTC, Michał Dębski
committed Details | Review
0003-gleffects-Merge-blur-filter-into-effects (19.00 KB, patch)
2015-04-06 12:34 UTC, Michał Dębski
committed Details | Review
0004-gleffects-Merge-sobel-filter-into-effects (23.65 KB, patch)
2015-04-06 12:34 UTC, Michał Dębski
none Details | Review
0005-gleffects-Merge-laplacian-filter-into-effects (19.74 KB, patch)
2015-04-06 12:39 UTC, Michał Dębski
none Details | Review
0006-gleffects-Create-element-for-each-effect (12.24 KB, patch)
2015-04-06 12:41 UTC, Michał Dębski
none Details | Review
0004-gleffects-Merge-sobel-filter-into-effects (23.65 KB, patch)
2015-04-19 16:43 UTC, Michał Dębski
none Details | Review
0005-gleffects-Merge-laplacian-filter-into-effects (19.74 KB, patch)
2015-04-19 16:43 UTC, Michał Dębski
none Details | Review
0006-gleffects-Create-element-for-each-effect (12.24 KB, patch)
2015-04-19 16:44 UTC, Michał Dębski
none Details | Review
0004-gleffects-Merge-sobel-filter-into-effects (23.41 KB, patch)
2015-04-21 05:51 UTC, Michał Dębski
committed Details | Review
0005-gleffects-Merge-laplacian-filter-into-effects (21.10 KB, patch)
2015-04-21 05:52 UTC, Michał Dębski
committed Details | Review
0006-gleffects-Create-element-for-each-effect (12.36 KB, patch)
2015-04-21 05:52 UTC, Michał Dębski
committed Details | Review

Description Michał Dębski 2015-03-14 14:27:04 UTC
Created attachment 299403 [details] [review]
merge_filters_into_effects.patch

Merge glfiltersobel, glfilterblur and glfilterlaplacian into gleffects
Comment 1 Matthew Waters (ystreet00) 2015-03-14 15:34:23 UTC
Review of attachment 299403 [details] [review]:

Please split these up into separate commits.  One for each effect.

Also, It may make sense to split all the effects up into separate elements in order for applications (like pitivi) to select based solely on element the function/filter that they want without special casing specific elements.
Comment 2 Julien Isorce 2015-03-14 16:38:36 UTC
I think for now we can remove glfiltersobel and glfilterblur to have these effects in gleffects. If needed we can still generate automatically elements from gleffects.

For example having "gleffects effect=sobel", you could autogenerate glfiltersobel as a kind of alias.
Comment 3 Julien Isorce 2015-03-14 17:38:49 UTC
So Nicolas pointed me that actually some filters like blur and sobel do not have properties in common to other effects that's why it still make sense to have separate elements.

Maybe we could have blur and sobel in gleffects, and for more granularity having them in separates elements. Re-using the code.
Comment 4 Michał Dębski 2015-03-14 17:44:40 UTC
You are right, there is invert property on sobel, which does the color inversion. This could however be implemented for other effects as well.
Blur on the other hand has customizable kernel, but it was not exposed as a element property. Moreoever glow effect is using kernel for blurring as well, so it could be usefull to expose it for both effects to benefit.
Comment 5 Michał Dębski 2015-03-22 11:40:03 UTC
Created attachment 300066 [details] [review]
0001-Fix-fisheye-shader-pass-float-to-sqrt
Comment 6 Michał Dębski 2015-03-22 11:41:59 UTC
Created attachment 300067 [details] [review]
0002-gleffects-Correct-attributes-for-glow-shader
Comment 7 Michał Dębski 2015-03-22 11:42:21 UTC
Created attachment 300068 [details] [review]
0003-gleffects-Merge-blur-filter-into-effects
Comment 8 Michał Dębski 2015-03-22 11:42:38 UTC
Created attachment 300069 [details] [review]
0004-gleffects-Merge-sobel-filter-into-effects
Comment 9 Michał Dębski 2015-03-22 11:43:02 UTC
Created attachment 300070 [details] [review]
0005-gleffects-Merge-laplacian-filter-into-effects
Comment 10 Michał Dębski 2015-03-22 11:46:53 UTC
@Matthew Waters patch is split.
Comment 11 Julien Isorce 2015-03-27 16:18:10 UTC
About properties, just to sum-up, gleffects has only specific property (except effect) "hswap" which "Switch video texture left to right" and glfiltersobel has one "invert" which invert colors. So there are not really a lot of specific properties at the moment. 

Michal, as your patches intersects a bit with https://bugzilla.gnome.org/show_bug.cgi?id=745955 I will land Anton's patch first (once it's ready) and then I'll ask you to rebase your patches. (I think it is easier this way than the opposite)

In the mean time could you try to also register one filter per effect. In addition to gleffects, it is useful to have glglow, glblur, glsqueeze, glsobel, glbulge etc... 

You do not need to add addtional .c files, you can do it dynamically like it is done for gstlibvisual, or gstv4l2 (http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2.c#n111) or frei0r (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/frei0r/gstfrei0r.c#n659).
Installing properties depending on the effect.

Thx!
Comment 12 Michał Dębski 2015-04-06 12:30:47 UTC
Created attachment 301010 [details] [review]
0001-Fix-fisheye-shader-pass-float-to-sqrt
Comment 13 Michał Dębski 2015-04-06 12:32:23 UTC
Created attachment 301011 [details] [review]
0002-gleffects-Correct-attributes-for-hconv-and-vconv-shader

width and height were mixed up. This made sampling incorrect for blur and glow effects.
Comment 14 Michał Dębski 2015-04-06 12:34:06 UTC
Created attachment 301012 [details] [review]
0003-gleffects-Merge-blur-filter-into-effects
Comment 15 Michał Dębski 2015-04-06 12:34:42 UTC
Created attachment 301013 [details] [review]
0004-gleffects-Merge-sobel-filter-into-effects
Comment 16 Michał Dębski 2015-04-06 12:39:30 UTC
Created attachment 301014 [details] [review]
0005-gleffects-Merge-laplacian-filter-into-effects

I have also fixed the shader, because it was substracting alpha channel as well as color channels resulting in completely transparent color and I have added invert property similar to sobel effect.
I also did version for GLES, but I'm not able to test it (it is the same as other convolution effects so it should work in theory). However I did not correct the issues described in the comment above shader source so I didn't move it into gleffectssources.c.
Comment 17 Michał Dębski 2015-04-06 12:41:12 UTC
Created attachment 301015 [details] [review]
0006-gleffects-Create-element-for-each-effect
Comment 18 Julien Isorce 2015-04-15 10:47:03 UTC
Review of attachment 301011 [details] [review]:

Correct.
Comment 19 Julien Isorce 2015-04-15 10:47:26 UTC
Review of attachment 301010 [details] [review]:

Correct.
Comment 20 Julien Isorce 2015-04-15 10:51:50 UTC
Review of attachment 301012 [details] [review]:

Looks good but has to be committed just near patch "0006-gleffects-Create-element-for-each-effect "
Comment 21 Julien Isorce 2015-04-15 11:00:18 UTC
Review of attachment 301013 [details] [review]:

Thx for the patch, just a few cleanup required.

::: ext/gl/effects/gstgleffectsobel.c
@@ +55,3 @@
+    gst_gl_shader_set_uniform_1i (shader, "tex", 0);
+    gst_gl_shader_set_uniform_1f (shader, "width", width);
+    gst_gl_shader_set_uniform_1f (shader, "height", height);

These 2 lines are should be removed.

@@ +90,3 @@
+    gst_gl_shader_set_uniform_1i (shader, "tex", 0);
+    gst_gl_shader_set_uniform_1f (shader, "width", width);
+    gst_gl_shader_set_uniform_1f (shader, "height", height);

This 1 line should be removed.

@@ +124,3 @@
+
+    gst_gl_shader_set_uniform_1i (shader, "tex", 0);
+    gst_gl_shader_set_uniform_1f (shader, "width", width);

This line should be removed.

@@ +160,3 @@
+    gst_gl_shader_set_uniform_1i (shader, "tex", 0);
+    gst_gl_shader_set_uniform_1f (shader, "width", width);
+    gst_gl_shader_set_uniform_1f (shader, "height", height);

These 2 lines should be removed.
Comment 22 Julien Isorce 2015-04-15 13:01:10 UTC
Review of attachment 301014 [details] [review]:

Looks good otherwise.

::: ext/gl/effects/gstgleffectlaplacian.c
@@ +90,3 @@
+  "}";
+/* *INDENT-ON* */
+

The 2 shaders should go to gstgleffectssources.c
Comment 23 Julien Isorce 2015-04-15 14:22:07 UTC
Review of attachment 301015 [details] [review]:

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
Comment 24 Michał Dębski 2015-04-19 16:43:19 UTC
Created attachment 301936 [details] [review]
0004-gleffects-Merge-sobel-filter-into-effects
Comment 25 Michał Dębski 2015-04-19 16:43:47 UTC
Created attachment 301937 [details] [review]
0005-gleffects-Merge-laplacian-filter-into-effects
Comment 26 Michał Dębski 2015-04-19 16:44:18 UTC
Created attachment 301938 [details] [review]
0006-gleffects-Create-element-for-each-effect
Comment 27 Michał Dębski 2015-04-19 16:45:49 UTC
Fixed all remarks.
Comment 28 Julien Isorce 2015-04-20 09:47:19 UTC
Thx for the update but the patches do not seem to have changed since previous review. Maybe you uploaded wrong branch ?
Comment 29 Michał Dębski 2015-04-21 05:51:26 UTC
Created attachment 302043 [details] [review]
0004-gleffects-Merge-sobel-filter-into-effects
Comment 30 Michał Dębski 2015-04-21 05:52:05 UTC
Created attachment 302044 [details] [review]
0005-gleffects-Merge-laplacian-filter-into-effects
Comment 31 Michał Dębski 2015-04-21 05:52:28 UTC
Created attachment 302045 [details] [review]
0006-gleffects-Create-element-for-each-effect
Comment 32 Julien Isorce 2015-04-21 10:45:50 UTC
Review of attachment 302043 [details] [review]:

You forgot a remark but that's fine I'll amend the commit before pushing.

::: ext/gl/effects/gstgleffectsobel.c
@@ +156,3 @@
+    gst_gl_shader_set_uniform_1i (shader, "tex", 0);
+    gst_gl_shader_set_uniform_1f (shader, "width", width);
+    gst_gl_shader_set_uniform_1f (shader, "height", height);

These 2 lines should be removed.
Comment 33 Julien Isorce 2015-04-21 10:47:46 UTC
Review of attachment 302044 [details] [review]:

Looks good. Need to be committed near patch "0006-gleffects-Create-element-for-each-effect "
Comment 34 Julien Isorce 2015-04-21 11:44:44 UTC
Review of attachment 302045 [details] [review]:

Good. I'll push all the patches shortly.
Comment 35 Julien Isorce 2015-04-21 12:06:14 UTC
Comment on attachment 301010 [details] [review]
0001-Fix-fisheye-shader-pass-float-to-sqrt

commit 40422d03c610f578db97680020218bd1dc1b4ff0
Author: Michał Dębski <debski.mi.zd@gmail.com>
Date:   Sat Mar 21 23:21:13 2015 +0100

    gleffects: Fix fisheye shader - pass float to sqrt
    
    On OSX passing literal int to sqrt() in GLSL results in error.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746209
Comment 36 Julien Isorce 2015-04-21 12:06:42 UTC
Comment on attachment 301011 [details] [review]
0002-gleffects-Correct-attributes-for-hconv-and-vconv-shader

commit ff4bc2364ccda262bc0690fe2934fcc68698a4a8
Author: Michał Dębski <debski.mi.zd@gmail.com>
Date:   Sat Mar 21 23:50:33 2015 +0100

    gleffects: Correct attributes for hconv and vconv shaders
    
    Width and height were switched for glow shaders. For blur
    filter attributes names were obsolete.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746209
Comment 37 Julien Isorce 2015-04-21 12:07:00 UTC
Comment on attachment 301012 [details] [review]
0003-gleffects-Merge-blur-filter-into-effects

commit 7770fb2f578bc0ccdb728f05637bd9bce377614a
Author: Michał Dębski <debski.mi.zd@gmail.com>
Date:   Sun Mar 22 11:13:30 2015 +0100

    gleffects: Merge blur filter into effects
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746209
Comment 38 Julien Isorce 2015-04-21 12:07:17 UTC
Comment on attachment 302043 [details] [review]
0004-gleffects-Merge-sobel-filter-into-effects

commit 0165b163db6696e1b6f2c0942c5d007042538033
Author: Michał Dębski <debski.mi.zd@gmail.com>
Date:   Sun Mar 22 11:20:49 2015 +0100

    gleffects: Merge sobel filter into effects
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746209
Comment 39 Julien Isorce 2015-04-21 12:07:29 UTC
Comment on attachment 302044 [details] [review]
0005-gleffects-Merge-laplacian-filter-into-effects

commit 532f332c2ff13f3ec204e971a73ea2a6c4320e58
Author: Michał Dębski <debski.mi.zd@gmail.com>
Date:   Sun Mar 22 11:22:52 2015 +0100

    gleffects: Merge laplacian filter into effects
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746209
Comment 40 Julien Isorce 2015-04-21 12:07:43 UTC
Comment on attachment 302045 [details] [review]
0006-gleffects-Create-element-for-each-effect

commit b12f975e0af1ae7f204c62efb152539e730c7fbd
Author: Michał Dębski <debski.mi.zd@gmail.com>
Date:   Sun Apr 5 20:18:56 2015 +0200

    gleffects: Create element for each effect
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746209