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 745955 - gleffects: port all effects to GLES2
gleffects: port all effects to GLES2
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 746209
 
 
Reported: 2015-03-10 11:30 UTC by Julien Isorce
Modified: 2015-03-30 18:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Draft patch to port all effects to GLES2.0. (89.12 KB, patch)
2015-03-14 19:00 UTC, Anton Obzhirov
none Details | Review
Updated after review (80.20 KB, patch)
2015-03-29 22:54 UTC, Anton Obzhirov
none Details | Review
Hopefully last update. (79.07 KB, patch)
2015-03-30 14:16 UTC, Anton Obzhirov
committed Details | Review

Description Julien Isorce 2015-03-10 11:30:42 UTC
Currently only 2 of the 15 effects are compatible with GLES2.

See http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/gstgleffects.c#n101

We need to make them work with GLES2.
Comment 1 Anton Obzhirov 2015-03-14 10:37:25 UTC
I am working on it. Today I am planning to upload a draft patch which will port all effects including blur, sobel and others,
Comment 2 Anton Obzhirov 2015-03-14 19:00:43 UTC
Created attachment 299414 [details] [review]
Draft patch to port all effects to GLES2.0.
Comment 3 Julien Isorce 2015-03-27 13:05:13 UTC
Review of attachment 299414 [details] [review]:

In addition to the remarks, I compared all the 16 effects between gles2 and opengl:

GST_GL_API=opengl GST_GL_PLATFORM=glx
gst-launch-1.0 videotestsrc ! glupload ! gleffects effect=$i ! glimagesink

GST_GL_API=gles2  GST_GL_PLATFORM=egl
gst-launch-1.0 videotestsrc ! glupload ! gleffects effect=$i ! glimagesink

Everything it's fines except xpro which is wrong on gles2.

::: ext/gl/effects/gstgleffectbulge.c
@@ +29,3 @@
+#define USING_GLES2(context) (gst_gl_context_check_gl_version (context, GST_GL_API_GLES2, 2, 0))
+#define USING_GLES3(context) (gst_gl_context_check_gl_version (context, GST_GL_API_GLES2, 3, 0))
+

I think it is safe to put this in existing file gstgleffect.h

@@ +64,3 @@
+      return;
+    }
+#endif

for this "lookup/if(!share)/GLES2/OPENGL3/OPENGL/endif" block you could provide a helper function in gstgleffect.h

::: ext/gl/gstgleffects.c
@@ +120,3 @@
     {0, NULL, NULL}
   };
 

Just remove GST_GL_HAVE_OPENGL guard.
Comment 4 Anton Obzhirov 2015-03-29 22:54:32 UTC
Created attachment 300555 [details] [review]
Updated after review

All comments are addressed and XPro effect is fixed.
Comment 5 Julien Isorce 2015-03-30 12:04:04 UTC
Review of attachment 300555 [details] [review]:

Thx for the updated patch. I put some new remarks since it breaks build when only enabling gles2: ./configure --disable-glx --enable-egl --disable-opengl --enable-gles2

::: ext/gl/effects/gstgleffectssources.c
@@ +125,2 @@
 /* Stretch Effect */
+#if GST_GL_HAVE_OPENGL

Do not had this guard, it breaks compilation when only enabling gles2.

@@ +665,3 @@
+  "  vec4 basecolor = texture2D (base, v_texcoord.xy);"
+  "  vec4 blendcolor = texture2D (blend, v_texcoord.xy);"
+	//"  gl_FragColor = basecolor * 0.5 + blendcolor * 0.5;"

Remove testing line.

::: ext/gl/effects/gstgleffectssources.h
@@ +24,1 @@
 #if GST_GL_HAVE_OPENGL

Remve this guard, it fine to compile both all the time, it was the case before already.

::: ext/gl/gstgleffects.c
@@ +237,3 @@
+            "cannot change effect type"), ("%s",
+            "the current OpenGL context does not support the GL API required"));
+    return;

Just remove this block since all effects are supported by all api now.

@@ +258,3 @@
+    case GST_GL_EFFECT_GLOW:
+      break;
+

Just remove this block.

@@ +261,3 @@
     default:
       g_assert_not_reached ();
   }

Move assert to previous switch since it handles all effects now.

@@ +491,3 @@
+    const gchar * shader_id, const gchar * shader_name,
+    const gchar * shader_source_gles2, const gchar * shader_source_opengl)
+{

Squash shader_id and shader_name. Having the 2 sources (opengl/gles2) is fine if more customization.
Comment 6 Anton Obzhirov 2015-03-30 14:16:33 UTC
Created attachment 300595 [details] [review]
Hopefully last update.

Fixed compilation with opengl disabled. Also addressed other comments.
Comment 7 Julien Isorce 2015-03-30 18:18:43 UTC
Comment on attachment 300595 [details] [review]
Hopefully last update.

commit 4504356ddab34a680d0d19f5d4c130114f621ea3
Author: Anton Obzhirov <obzhirov@yahoo.co.uk>
Date:   Mon Mar 30 13:49:01 2015 +0100

    gleffects: port all effects to GLES2.0
    
    https://bugzilla.gnome.org/show_bug.cgi?id=745955