GNOME Bugzilla – Bug 731722
OpenGL: Add an element for transforming video geometry
Last modified: 2014-06-18 10:42:50 UTC
Created attachment 278540 [details] [review] Adds the gltransformation element Video of the element: https://www.youtube.com/watch?v=Jn_09BeUEI0 The python script from the video: https://github.com/lubosz/gst-gl-tests/blob/master/transformation.py This element needs the graphene library as a soft dependency: https://github.com/ebassi/graphene/ The release plan for this is "around GUADEC" according to ebassi. Graphene will be beneficial for all opengl plugins, or things which need linear algebra. Looking forward to comments and review.
Review of attachment 278540 [details] [review]: Very clean patch in my opinion, I did not review the OpenGL related code as I have no knowledge of OpenGL, but only had very minor nitpicks. ::: ext/gl/gstgltransformation.c @@ +121,3 @@ + gobject_class->set_property = gst_gl_transformation_set_property; + gobject_class->get_property = gst_gl_transformation_get_property; + Unneeded new line @@ +138,3 @@ + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + + // Rotation C++ style comment, not sure what the policy is but I guess it might be better to stick to "/* */" @@ +179,3 @@ + g_object_class_install_property (gobject_class, PROP_XSCALE, + g_param_spec_float ("xscale", "X Scale", + "Scales the video at the X-Axis in times.", Might be clearer to say "Scale multiplier for the X-axis" ? @@ +299,3 @@ + GstGLTransformation *transformation = GST_GL_TRANSFORMATION (filter); + + if (transformation->aspect == 0) This means you won't let the aspect change if the geometry of the input stream changes right ? @@ +312,3 @@ + GstGLTransformation *transformation = GST_GL_TRANSFORMATION (filter); + + /* blocking call, wait the opengl thread has destroyed the shader */ wait until* :) @@ +324,3 @@ + + if (gst_gl_context_get_gl_api (filter->context)) { + /* blocking call, wait the opengl thread has compiled the shader */ wait until @@ +412,3 @@ + if (transformation->ortho) { + graphene_matrix_init_ortho (&projection_matrix, + -1 * transformation->aspect, 1 * transformation->aspect, No need to multiply transformation->aspect by 1 in the third argument I guess :)
Created attachment 278594 [details] [review] Adds the gltransformation element (integrated Mathieu's comments) Updated
Review of attachment 278540 [details] [review]: I think it would be better to cache the transformation matrix and only recompute when some value changes instead of each frame? Otherwise, just a couple of little things on top of Mathieu's comments. Good job :) ::: ext/gl/gstgltransformation.c @@ +140,3 @@ + // Rotation + g_object_class_install_property (gobject_class, PROP_XROTATION, + g_param_spec_float ("xrotation", "X Rotation", Name should be "rotation-x". Same for all the other property names @@ +430,3 @@ + gl->Disable (GL_TEXTURE_2D); + + gl->Enable (GL_DEPTH_TEST); This doesn't need the depth test.
Created attachment 278599 [details] [review] Adds the gltransformation element (integrated Matthew's comments) Updated again. Also integrated GLES shader support.
Review of attachment 278599 [details] [review]: Very close :) ::: ext/gl/gstgltransformation.c @@ +32,3 @@ + * gst-launch gltestsrc ! gltransformation xtranslate=4 ! video/x-raw, width=640, height=480 ! glimagesink + * ]| Resize scene after drawing. + * The scene size is greater than the input video size. (Actually the pipeline is negotiated such that gltestsrc produces full-frame 640x480 buffers) @@ +36,3 @@ + * gst-launch gltestsrc ! video/x-raw, width=1280, height=720 ! gltransformation xscale=1.5 ! glimagesink + * ]| Resize scene before drawing the cube. + * The scene size is greater than the input video size. copy-paste error? @@ +66,3 @@ + +#define DEBUG_INIT \ + GST_DEBUG_CATEGORY_INIT (gst_gl_transformation_debug, "GLTransformation", 0, "GLTransformation element"); Debug categories are generally lowercase for elements @@ +92,3 @@ + "#ifdef GL_ES \n" + " precision mediump float; \n" + "#endif \n" The precision qualifiers are only needed in the fragment stage, not the vertex stage. ::: ext/gl/gstgltransformation.h @@ +75,3 @@ +G_END_DECLS + +#endif /* _GST_GLTransformation_H_ */ nitpick. All CAPS - _GST_GL_TRANSFORMATION_H_
Created attachment 278606 [details] [review] version 4 * removed GL_ES from vertex shader * updated examples * set all props to float, and make them use G_MAXFLOAT * fixed cases
commit 053252ccc6347cb2a951a41f47358e32130b5f71 Author: Lubosz Sarnecki <lubosz@gmail.com> Date: Tue May 27 12:40:09 2014 +0200 opengl: add element for transforming video geometry * add graphene as soft dependency for linear algebra