GNOME Bugzilla – Bug 764873
gldeinterlace: enable this plugin on OpenGL ES using a simple deinterlace fragment shader
Last modified: 2016-09-27 06:11:24 UTC
Can we enable gldeinterlace on OpenGL ES. For better performance, we can use a simple fragment shader. I have tested this shader, it can meet requirement.
Created attachment 325696 [details] [review] gldeinterlace: enable this plugin on OpenGL ES using a simple deinterlace fragment shader
Review of attachment 325696 [details] [review]: A couple of things. It's possible to build against both the OpenGL desktop and GLES API's. As such the GST_GL_HAVE OPENGL define is not the correct way to conditionally check for the GL API. Use gst_gl_context_get_gl_api() instead. Also, the current deinterlace shader is trivial to port to GLES2. Any reason you did not do that? ::: ext/gl/gstgldeinterlace.c @@ +69,2 @@ /* *INDENT-OFF* */ +const gchar *vertex_default = You should use the default vertex string, gst_gl_shader_string_vertex_default instead of duplicating it here. @@ +157,3 @@ + +const gchar *deinterlace_fragment_source_gles2 = + "precision mediump float;\n" Should add #ifdef GL_ES, #endif around the precision qualifier. @@ +269,2 @@ //blocking call, wait the opengl thread has compiled the shader +#if GST_GL_HAVE_OPENGL This won't always do what you want. Use gst_gl_context_get_gl_api() instead. @@ +326,3 @@ + gst_gl_shader_use (deinterlace_filter->shader); + +#if GST_GL_HAVE_OPENGL ditto
(In reply to Matthew Waters (ystreet00) from comment #2) > Review of attachment 325696 [details] [review] [review]: > > A couple of things. > > It's possible to build against both the OpenGL desktop and GLES API's. As > such the GST_GL_HAVE OPENGL define is not the correct way to conditionally > check for the GL API. Use gst_gl_context_get_gl_api() instead. > > Also, the current deinterlace shader is trivial to port to GLES2. Any > reason you did not do that? We have tried the greedyh_fragment_source and it can run on GLES2, but the performance is not good on i.Mx6QP. We need this plugin to do deinterlace by GPU and create the simple one. It got good results. I wonder if we can enable this plugin on GLES2 just like gleffects, let user to choose the deinterlace shader. In this way, we can add more deinterlace shader. > > ::: ext/gl/gstgldeinterlace.c > @@ +69,2 @@ > /* *INDENT-OFF* */ > +const gchar *vertex_default = > > You should use the default vertex string, > gst_gl_shader_string_vertex_default instead of duplicating it here. > > @@ +157,3 @@ > + > +const gchar *deinterlace_fragment_source_gles2 = > + "precision mediump float;\n" > > Should add #ifdef GL_ES, #endif around the precision qualifier. > > @@ +269,2 @@ > //blocking call, wait the opengl thread has compiled the shader > +#if GST_GL_HAVE_OPENGL > > This won't always do what you want. Use gst_gl_context_get_gl_api() instead. > > @@ +326,3 @@ > + gst_gl_shader_use (deinterlace_filter->shader); > + > +#if GST_GL_HAVE_OPENGL > > ditto
Created attachment 325707 [details] [review] refine the patch
Review of attachment 325707 [details] [review]: I think this should be behind a property, say method, like the deinterlace element in -good. There's nothing technically prohibiting enabling this (vertical blur) shader (and the existing greedyh one everywhere). In fact, all that's required is to add a property and merge the two _callback() implementations. ::: ext/gl/gstgldeinterlace.c @@ +200,3 @@ + GST_GL_BASE_FILTER_CLASS (klass)->supported_gl_api = + GST_GL_API_OPENGL | GST_GL_API_GLES2; gles2 shaders are also supported for OpenGL3.
(In reply to Matthew Waters (ystreet00) from comment #5) > Review of attachment 325707 [details] [review] [review]: > > I think this should be behind a property, say method, like the deinterlace > element in -good. There's nothing technically prohibiting enabling this > (vertical blur) shader (and the existing greedyh one everywhere). In fact, > all that's required is to add a property and merge the two _callback() > implementations. > > ::: ext/gl/gstgldeinterlace.c > @@ +200,3 @@ > > + GST_GL_BASE_FILTER_CLASS (klass)->supported_gl_api = > + GST_GL_API_OPENGL | GST_GL_API_GLES2; > > gles2 shaders are also supported for OpenGL3. Yes, you are right. That's what I mean, let me refine this patch just like what we discuss above.
Created attachment 325838 [details] [review] Add method property to gldeinterlace and add blur vertical method
(In reply to Matthew Waters (ystreet00) from comment #5) > Review of attachment 325707 [details] [review] [review]: > > I think this should be behind a property, say method, like the deinterlace > element in -good. There's nothing technically prohibiting enabling this > (vertical blur) shader (and the existing greedyh one everywhere). In fact, > all that's required is to add a property and merge the two _callback() > implementations. > > ::: ext/gl/gstgldeinterlace.c > @@ +200,3 @@ > > + GST_GL_BASE_FILTER_CLASS (klass)->supported_gl_api = > + GST_GL_API_OPENGL | GST_GL_API_GLES2; > > gles2 shaders are also supported for OpenGL3. Hi Matthew, I have refine the gldeinterlace, help me review the code. Thanks a lot.
(In reply to Matthew Waters (ystreet00) from comment #5) > Review of attachment 325707 [details] [review] [review]: > > I think this should be behind a property, say method, like the deinterlace > element in -good. There's nothing technically prohibiting enabling this > (vertical blur) shader (and the existing greedyh one everywhere). In fact, > all that's required is to add a property and merge the two _callback() > implementations. > > ::: ext/gl/gstgldeinterlace.c > @@ +200,3 @@ > > + GST_GL_BASE_FILTER_CLASS (klass)->supported_gl_api = > + GST_GL_API_OPENGL | GST_GL_API_GLES2; > > gles2 shaders are also supported for OpenGL3. Hi Matthew, Any update for this patch?
Review of attachment 325838 [details] [review]: This doesn't seem to apply cleanly to latest master. After modifying the patch to apply, it also doesn't compile with opengl enabled and -Werror enabled. gstgldeinterlace.c: In function ‘gst_gl_deinterlace_vfir_callback’: gstgldeinterlace.c:437:52: error: passing argument 1 of ‘gst_gl_deinterlace_get_fragment_shader’ from incompatible pointer type [-Werror=incompatible-pointer-types] shader = gst_gl_deinterlace_get_fragment_shader (deinterlace_filter, "vfir", ^ gstgldeinterlace.c:393:1: note: expected ‘GstGLFilter * {aka struct _GstGLFilter *}’ but argument is of type ‘GstGLDeinterlace * {aka struct _GstGLDeinterlace *}’ gst_gl_deinterlace_get_fragment_shader (GstGLFilter * filter, ^ gstgldeinterlace.c:444:7: error: implicit declaration of function ‘USING_OPENGL’ [-Werror=implicit-function-declaration] if (USING_OPENGL (context)) { ^ gstgldeinterlace.c:444:3: error: nested extern declaration of ‘USING_OPENGL’ [-Werror=nested-externs] if (USING_OPENGL (context)) { ^ gstgldeinterlace.c: In function ‘gst_gl_deinterlace_greedyh_callback’: gstgldeinterlace.c:476:47: error: passing argument 1 of ‘gst_gl_deinterlace_get_fragment_shader’ from incompatible pointer type [-Werror=incompatible-pointer-types] gst_gl_deinterlace_get_fragment_shader (deinterlace_filter, "greedhy", ^ gstgldeinterlace.c:393:1: note: expected ‘GstGLFilter * {aka struct _GstGLFilter *}’ but argument is of type ‘GstGLDeinterlace * {aka struct _GstGLDeinterlace *}’ gst_gl_deinterlace_get_fragment_shader (GstGLFilter * filter, ^ ::: ext/gl/gstgldeinterlace.h @@ +41,3 @@ + + GLCB deinterlacefunc; + GHashTable *shaderstable; Why are you using a hash table?
(In reply to Matthew Waters (ystreet00) from comment #10) > Review of attachment 325838 [details] [review] [review]: > > This doesn't seem to apply cleanly to latest master. > > After modifying the patch to apply, it also doesn't compile with opengl > enabled and -Werror enabled. > > gstgldeinterlace.c: In function ‘gst_gl_deinterlace_vfir_callback’: > gstgldeinterlace.c:437:52: error: passing argument 1 of > ‘gst_gl_deinterlace_get_fragment_shader’ from incompatible pointer type > [-Werror=incompatible-pointer-types] > shader = gst_gl_deinterlace_get_fragment_shader (deinterlace_filter, > "vfir", > ^ > gstgldeinterlace.c:393:1: note: expected ‘GstGLFilter * {aka struct > _GstGLFilter *}’ but argument is of type ‘GstGLDeinterlace * {aka struct > _GstGLDeinterlace *}’ > gst_gl_deinterlace_get_fragment_shader (GstGLFilter * filter, > ^ > gstgldeinterlace.c:444:7: error: implicit declaration of function > ‘USING_OPENGL’ [-Werror=implicit-function-declaration] > if (USING_OPENGL (context)) { > ^ > gstgldeinterlace.c:444:3: error: nested extern declaration of ‘USING_OPENGL’ > [-Werror=nested-externs] > if (USING_OPENGL (context)) { > ^ > gstgldeinterlace.c: In function ‘gst_gl_deinterlace_greedyh_callback’: > gstgldeinterlace.c:476:47: error: passing argument 1 of > ‘gst_gl_deinterlace_get_fragment_shader’ from incompatible pointer type > [-Werror=incompatible-pointer-types] > gst_gl_deinterlace_get_fragment_shader (deinterlace_filter, "greedhy", > ^ > gstgldeinterlace.c:393:1: note: expected ‘GstGLFilter * {aka struct > _GstGLFilter *}’ but argument is of type ‘GstGLDeinterlace * {aka struct > _GstGLDeinterlace *}’ > gst_gl_deinterlace_get_fragment_shader (GstGLFilter * filter, > ^ > > ::: ext/gl/gstgldeinterlace.h > @@ +41,3 @@ > + > + GLCB deinterlacefunc; > + GHashTable *shaderstable; > > Why are you using a hash table? Sorry for my mistake: I forgot to declare USING_OPENGL in gstgldeinterlace.h and the function first argument of gst_gl_deinterlace_get_fragment_shader is wrong, I will update this patch immediately. Use a hash table to store shader we create. We only need to create the shader when first time to run, it will allow us to switch deinterlace shader at run time without create it again
Created attachment 326848 [details] [review] correct patch coding error
Thanks! commit 0cfb0890ce9a20f096a0a9c9f23b656f436794c6 Author: Haihua Hu <jared.hu@nxp.com> Date: Fri Apr 8 16:47:15 2016 +0800 gl: enable gldeinterlace on OpenGL ES 1.Porting the exist deinterlace shader and OpenGL callback to be compatible with OpenGL ES. 2.Add a our blur vertical shader to gldeinterlace. 3.Add a property named “method” to let user choose which deinterlace function to use. Default to choose blur vertical method for better performance. [Matthew Waters]: fix name of greedyh in method property (was greedhy) and port to git master. https://bugzilla.gnome.org/show_bug.cgi?id=764873
*** Bug 747232 has been marked as a duplicate of this bug. ***