[Bug 703346] Integrate shaders from eglglessink

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=703346
  GStreamer | gst-plugins-gl | git

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #249483|none                        |needs-work
             status|                            |

--- Comment #3 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-07-18 10:54:20 UTC ---
Review of attachment 249483:
 --> (https://bugzilla.gnome.org/review?bug=703346&attachment=249483)

::: gst-libs/gst/gl/gstglupload.c
@@ +126,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

tex_scale2 is not used

@@ +142,3 @@
+
+/* Channel reordering for XYZ <-> ZYX conversion */
+static const char *frag_REORDER_opengl = {

With desktop GL no reordering is necessary, right? It might be more optimal to
use the integrated support for the different RGBA orders

@@ +147,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

tex_scale are not used

@@ +161,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

tex_scale are not used

@@ +183,3 @@
+    "  yuv.x = texture2DRect(Ytex, gl_TexCoord[0].xy * tex_scale0).%c;\n"
+    "  yuv.y = texture2DRect(UVtex, gl_TexCoord[0].xy * tex_scale1).%c;\n"
+    "  yuv.z = texture2DRect(UVtex, gl_TexCoord[0].xy * tex_scale2).%c;\n"

YUY2,etc only have a single stride. Why three scales? Two might make sense
though

@@ +202,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

Scales not used

@@ +216,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

Scales not used

@@ +237,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

Scales not used

@@ +260,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

Scales not used

@@ +282,3 @@
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"

Scales not used

@@ +334,2 @@
   const gchar *AYUV;
+  const gchar *NV12_NV21;

NV12 shaders can also be used for NV16 and NV24 (same format, just chroma plane
of a slightly different size)

@@ +1127,3 @@
+      tex[1].type = GL_UNSIGNED_BYTE;
+      tex[1].width = GST_ROUND_UP_2 (in_width) / 2;
+      tex[1].height = GST_ROUND_UP_2 (in_height) / 2;

Ideally you would just use the information from the GstVideoFrame /
GstVideoInfo for this everywhere

@@ +1227,3 @@
+      gl->BindTexture (GL_TEXTURE_RECTANGLE_ARB, upload->in_texture[1]);
+      gl->TexSubImage2D (GL_TEXTURE_RECTANGLE_ARB, 0, 0, 0,
+          GST_ROUND_UP_2 (in_width) / 2, in_height,

Same here, GstVideoInfo/GstVideoFrame

@@ +1228,3 @@
+      gl->TexSubImage2D (GL_TEXTURE_RECTANGLE_ARB, 0, 0, 0,
+          GST_ROUND_UP_2 (in_width) / 2, in_height,
+          GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, upload->data[0]);

GL_RGBA, GL_UNSIGNED_BYTE?

@@ +1247,3 @@
+      gl->TexSubImage2D (GL_TEXTURE_RECTANGLE_ARB, 0, 0, 0,
+          in_width / 2, in_height / 2,
+          GL_LUMINANCE_ALPHA, GL_UNSIGNED_BYTE, upload->data[1]);

If available it is more optimal to use GL_R8 and GL_RG8 here (red, red/green),
instead of LUM/ALPHA. Everywhere

@@ +1441,3 @@
+        tex_scaling[2] = tex_scaling[4] = 0.5;
+      else if (v_format == GST_VIDEO_FORMAT_Y41B)
+        tex_scaling[2] = tex_scaling[4] = 0.25;

That's not what the point of the scaling was :) The scaling was used to handle
arbitrary strides, together with GL_UNPACK. The scales should be at 1.0 for all
the different planes, and GL should do bilinear interpolation for us instead of
us worrying in the shaders about the different subsampling.

Maybe drop the tex_scaling bits everywhere for now until we support arbitrary
strides. That makes the code simpler

::: gst-libs/gst/gl/gstglupload.h
@@ +95,3 @@
 #define GST_GL_UPLOAD_FORMATS "{ RGB, RGBx, RGBA, BGR, BGRx, BGRA, xRGB, " \
+                               "xBGR, ARGB, ABGR, Y444, I420, YV12, Y42B, " \
+                               "Y41B, NV12, NV21, YUY2, UYVY, AYUV }"

Btw, you can easily also support YVYU (same as YUY2 just chroma components
swapped)

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.