GNOME Bugzilla – Bug 746209
Merge glfiltersobel, glfilterblur and glfilterlaplacian into gleffects
Last modified: 2015-04-21 12:08:20 UTC
Created attachment 299403 [details] [review] merge_filters_into_effects.patch Merge glfiltersobel, glfilterblur and glfilterlaplacian into gleffects
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.
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.
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.
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.
Created attachment 300066 [details] [review] 0001-Fix-fisheye-shader-pass-float-to-sqrt
Created attachment 300067 [details] [review] 0002-gleffects-Correct-attributes-for-glow-shader
Created attachment 300068 [details] [review] 0003-gleffects-Merge-blur-filter-into-effects
Created attachment 300069 [details] [review] 0004-gleffects-Merge-sobel-filter-into-effects
Created attachment 300070 [details] [review] 0005-gleffects-Merge-laplacian-filter-into-effects
@Matthew Waters patch is split.
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!
Created attachment 301010 [details] [review] 0001-Fix-fisheye-shader-pass-float-to-sqrt
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.
Created attachment 301012 [details] [review] 0003-gleffects-Merge-blur-filter-into-effects
Created attachment 301013 [details] [review] 0004-gleffects-Merge-sobel-filter-into-effects
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.
Created attachment 301015 [details] [review] 0006-gleffects-Create-element-for-each-effect
Review of attachment 301011 [details] [review]: Correct.
Review of attachment 301010 [details] [review]: Correct.
Review of attachment 301012 [details] [review]: Looks good but has to be committed just near patch "0006-gleffects-Create-element-for-each-effect "
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.
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
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
Created attachment 301936 [details] [review] 0004-gleffects-Merge-sobel-filter-into-effects
Created attachment 301937 [details] [review] 0005-gleffects-Merge-laplacian-filter-into-effects
Created attachment 301938 [details] [review] 0006-gleffects-Create-element-for-each-effect
Fixed all remarks.
Thx for the update but the patches do not seem to have changed since previous review. Maybe you uploaded wrong branch ?
Created attachment 302043 [details] [review] 0004-gleffects-Merge-sobel-filter-into-effects
Created attachment 302044 [details] [review] 0005-gleffects-Merge-laplacian-filter-into-effects
Created attachment 302045 [details] [review] 0006-gleffects-Create-element-for-each-effect
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.
Review of attachment 302044 [details] [review]: Looks good. Need to be committed near patch "0006-gleffects-Create-element-for-each-effect "
Review of attachment 302045 [details] [review]: Good. I'll push all the patches shortly.
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 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 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 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 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 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