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 731722 - OpenGL: Add an element for transforming video geometry
OpenGL: Add an element for transforming video geometry
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.3.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-06-16 13:32 UTC by Lubosz Sarnecki
Modified: 2014-06-18 10:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adds the gltransformation element (21.44 KB, patch)
2014-06-16 13:32 UTC, Lubosz Sarnecki
reviewed Details | Review
Adds the gltransformation element (integrated Mathieu's comments) (21.38 KB, patch)
2014-06-17 12:15 UTC, Lubosz Sarnecki
none Details | Review
Adds the gltransformation element (integrated Matthew's comments) (22.00 KB, patch)
2014-06-17 13:20 UTC, Lubosz Sarnecki
reviewed Details | Review
version 4 (22.58 KB, patch)
2014-06-17 14:30 UTC, Lubosz Sarnecki
committed Details | Review

Description Lubosz Sarnecki 2014-06-16 13:32:27 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.
Comment 1 Mathieu Duponchelle 2014-06-16 16:52:30 UTC
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 :)
Comment 2 Lubosz Sarnecki 2014-06-17 12:15:13 UTC
Created attachment 278594 [details] [review]
Adds the gltransformation element (integrated Mathieu's comments)

Updated
Comment 3 Matthew Waters (ystreet00) 2014-06-17 12:42:59 UTC
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.
Comment 4 Lubosz Sarnecki 2014-06-17 13:20:53 UTC
Created attachment 278599 [details] [review]
Adds the gltransformation element (integrated Matthew's comments)

Updated again. Also integrated GLES shader support.
Comment 5 Matthew Waters (ystreet00) 2014-06-17 13:35:48 UTC
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_
Comment 6 Lubosz Sarnecki 2014-06-17 14:30:57 UTC
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
Comment 7 Matthew Waters (ystreet00) 2014-06-18 10:42:34 UTC
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