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 722670 - add GRAY8/GRAY16_LE/GRAY16_BE support to glupload
add GRAY8/GRAY16_LE/GRAY16_BE support to glupload
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-gl
1.2.2
Other Linux
: Normal enhancement
: 1.2.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-21 03:27 UTC by comicfans44
Modified: 2014-01-29 19:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
the patch file (6.82 KB, patch)
2014-01-21 03:27 UTC, comicfans44
needs-work Details | Review
improved patch (7.31 KB, patch)
2014-01-22 02:18 UTC, comicfans44
committed Details | Review
upload: use GL_UNSIGNED_SHORT for 16-bit formats (4.63 KB, patch)
2014-01-24 09:16 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
upload: use GL_UNSIGNED_SHORT for 16-bit formats (5.60 KB, patch)
2014-01-24 21:37 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
upload: fix compilation for GLES2 (2.35 KB, patch)
2014-01-27 20:52 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description comicfans44 2014-01-21 03:27:12 UTC
Created attachment 266835 [details] [review]
the patch file

this patch implements GRAY format upload to glupload . 

From 6837cb52a4edcd9319fe619f1c01b18c9149f56a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?"Wang=20Xin-yu=20(=E7=8E=8B=E6=98=95=E5=AE=87)"?=
 <comicfans44@gmail.com>
Date: Fri, 17 Jan 2014 08:46:02 +0800
Subject: [PATCH] add GRAY8/GRAY16LE/BE upload support to glimagesink

the 16bit data is uploaded as LUMINANCE_ALPHA, then expanded, composed
in shader. value weight is a little complicate, high byte weight is
255*256/65535 (denormalize to [0~255] ,shift to high byte,then normalize
to [0~1]), low byte weight is 255/65535(similar)
---
 gst-libs/gst/gl/gstglupload.c |   80 +++++++++++++++++++++++++++++++++++++++++
 gst-libs/gst/gl/gstglupload.h |    3 +-
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/gst-libs/gst/gl/gstglupload.c b/gst-libs/gst/gl/gstglupload.c
index 3dac6fe..d9c69b4 100644
--- a/gst-libs/gst/gl/gstglupload.c
+++ b/gst-libs/gst/gl/gstglupload.c
@@ -143,6 +143,25 @@ static const char *frag_REORDER_opengl = {
       "}"
 };
 
+#define COMPOSE_WEIGHT \
+    "const vec2 compose_weight = vec2(0.996109, 0.003891);\n"
+
+/* Compose LUMINANCE/ALPHA as 8bit-8bit value */
+static const char *frag_COMPOSE_opengl = {
+      "uniform sampler2D tex;\n"
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"
+      COMPOSE_WEIGHT
+      "void main(void)\n"
+      "{\n"
+      " vec4 t = texture2D(tex, gl_TexCoord[0].xy);\n"
+      " float value = dot(t.%c%c, compose_weight);"
+      " gl_FragColor = vec4(value, value, value, 1.0);\n"
+      "}"
+
+};
+
 /* Direct fragments copy with stride-scaling */
 static const char *frag_COPY_opengl = {
       "uniform sampler2D tex;\n"
@@ -196,6 +215,23 @@ static const char *frag_REORDER_gles2 = {
       "}"
 };
 
+/* Compose LUMINANCE/ALPHA as 8bit-8bit value */
+static const char *frag_COMPOSE_gles2 = {
+      "precision mediump float;\n"
+      "varying vec2 v_texcoord;\n"
+      "uniform sampler2D tex;\n"
+      "uniform vec2 tex_scale0;\n"
+      "uniform vec2 tex_scale1;\n"
+      "uniform vec2 tex_scale2;\n"
+      COMPOSE_WEIGHT
+      "void main(void)\n"
+      "{\n"
+      " vec4 t = texture2D(tex, v_texcoord);\n"
+      " float value = dot(t.%c%c, compose_weight);"
+      " gl_FragColor = vec4(value, value, value, 1.0);\n"
+      "}"
+
+};
 static const char *frag_AYUV_gles2 = {
       "precision mediump float;\n"
       "varying vec2 v_texcoord;\n"
@@ -322,6 +358,7 @@ struct _GstGLUploadPrivate
   const gchar *NV12_NV21;
   const gchar *REORDER;
   const gchar *COPY;
+  const gchar *COMPOSE;
   const gchar *vert_shader;
 
     gboolean (*draw) (GstGLContext * context, GstGLUpload * download);
@@ -398,6 +435,7 @@ gst_gl_upload_new (GstGLContext * context)
     priv->PLANAR_YUV = frag_PLANAR_YUV_opengl;
     priv->AYUV = frag_AYUV_opengl;
     priv->REORDER = frag_REORDER_opengl;
+    priv->COMPOSE = frag_COMPOSE_opengl;
     priv->COPY = frag_COPY_opengl;
     priv->NV12_NV21 = frag_NV12_NV21_opengl;
     priv->vert_shader = text_vertex_shader_opengl;
@@ -410,6 +448,7 @@ gst_gl_upload_new (GstGLContext * context)
     priv->PLANAR_YUV = frag_PLANAR_YUV_gles2;
     priv->AYUV = frag_AYUV_gles2;
     priv->REORDER = frag_REORDER_gles2;
+    priv->COMPOSE = frag_COMPOSE_gles2;
     priv->COPY = frag_COPY_gles2;
     priv->NV12_NV21 = frag_NV12_NV21_gles2;
     priv->vert_shader = text_vertex_shader_gles2;
@@ -1017,6 +1056,17 @@ _init_upload (GstGLContext * context, GstGLUpload * upload)
       free_frag_prog = TRUE;
       upload->priv->n_textures = 1;
       break;
+    case GST_VIDEO_FORMAT_GRAY16_BE:
+      frag_prog = g_strdup_printf (upload->priv->COMPOSE, 'r', 'a');
+      free_frag_prog = TRUE;
+      upload->priv->n_textures = 1;
+      break;
+    case GST_VIDEO_FORMAT_GRAY16_LE:
+      frag_prog = g_strdup_printf (upload->priv->COMPOSE, 'a', 'r');
+      free_frag_prog = TRUE;
+      upload->priv->n_textures = 1;
+      break;
+    case GST_VIDEO_FORMAT_GRAY8:
     case GST_VIDEO_FORMAT_RGB:
     case GST_VIDEO_FORMAT_RGBx:
     case GST_VIDEO_FORMAT_RGBA:
@@ -1230,6 +1280,21 @@ _do_upload_make (GstGLContext * context, GstGLUpload * upload)
       tex[0].width = in_width;
       tex[0].height = in_height;
       break;
+    case GST_VIDEO_FORMAT_GRAY8:
+      tex[0].internal_format = GL_LUMINANCE;
+      tex[0].format = GL_LUMINANCE;
+      tex[0].type = GL_UNSIGNED_BYTE;
+      tex[0].width = in_width;
+      tex[0].height = in_height;
+      break;
+    case GST_VIDEO_FORMAT_GRAY16_BE:
+    case GST_VIDEO_FORMAT_GRAY16_LE:
+      tex[0].internal_format = GL_LUMINANCE_ALPHA;
+      tex[0].format = GL_LUMINANCE_ALPHA;
+      tex[0].type = GL_UNSIGNED_BYTE;
+      tex[0].width = in_width;
+      tex[0].height = in_height;
+      break;
     case GST_VIDEO_FORMAT_YUY2:
       tex[0].internal_format = GL_LUMINANCE_ALPHA;
       tex[0].format = GL_LUMINANCE_ALPHA;
@@ -1372,6 +1437,15 @@ _do_upload_fill (GstGLContext * context, GstGLUpload * upload)
   gl->BindTexture (GL_TEXTURE_2D, upload->in_texture[0]);
 
   switch (v_format) {
+    case GST_VIDEO_FORMAT_GRAY8:
+      gl->TexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, in_width, in_height,
+          GL_LUMINANCE, GL_UNSIGNED_BYTE, upload->data[0]);
+      break;
+    case GST_VIDEO_FORMAT_GRAY16_BE:
+    case GST_VIDEO_FORMAT_GRAY16_LE:
+      gl->TexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, in_width, in_height,
+          GL_LUMINANCE_ALPHA, GL_UNSIGNED_BYTE, upload->data[0]);
+      break;
     case GST_VIDEO_FORMAT_RGB:
     case GST_VIDEO_FORMAT_BGR:
       gl->TexSubImage2D (GL_TEXTURE_2D, 0, 0, 0, in_width, in_height,
@@ -1570,6 +1644,9 @@ _do_upload_draw_opengl (GstGLContext * context, GstGLUpload * upload)
   gl->Clear (GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
 
   switch (v_format) {
+    case GST_VIDEO_FORMAT_GRAY8:
+    case GST_VIDEO_FORMAT_GRAY16_BE:
+    case GST_VIDEO_FORMAT_GRAY16_LE:
     case GST_VIDEO_FORMAT_RGBx:
     case GST_VIDEO_FORMAT_BGRx:
     case GST_VIDEO_FORMAT_xRGB:
@@ -1717,6 +1794,9 @@ _do_upload_draw_gles2 (GstGLContext * context, GstGLUpload * upload)
   gl->Clear (GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
 
   switch (v_format) {
+    case GST_VIDEO_FORMAT_GRAY8:
+    case GST_VIDEO_FORMAT_GRAY16_BE:
+    case GST_VIDEO_FORMAT_GRAY16_LE:
     case GST_VIDEO_FORMAT_RGBx:
     case GST_VIDEO_FORMAT_BGRx:
     case GST_VIDEO_FORMAT_xRGB:
diff --git a/gst-libs/gst/gl/gstglupload.h b/gst-libs/gst/gl/gstglupload.h
index a56759f..c8dcb57 100644
--- a/gst-libs/gst/gl/gstglupload.h
+++ b/gst-libs/gst/gl/gstglupload.h
@@ -90,7 +90,8 @@ struct _GstGLUploadClass
  */
 #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 }"
+                               "Y41B, NV12, NV21, YUY2, UYVY, AYUV, " \
+                               "GRAY8, GRAY16_LE, GRAY16_BE }"
 
 /**
  * GST_GL_UPLOAD_VIDEO_CAPS:
-- 
1.7.9.5
Comment 1 Sebastian Dröge (slomo) 2014-01-21 08:54:31 UTC
Please don't paste complete patches into Bugzilla comments :)
Comment 2 Sebastian Dröge (slomo) 2014-01-21 08:57:05 UTC
Review of attachment 266835 [details] [review]:

Generally looks good but why not also add support to gldownload? :)

::: gst-libs/gst/gl/gstglupload.c
@@ +145,3 @@
 
+#define COMPOSE_WEIGHT \
+    "const vec2 compose_weight = vec2(0.996109, 0.003891);\n"

Please write a comment where these magic numbers come from, it might not be obvious for everybody :)

@@ +1063,3 @@
+      break;
+    case GST_VIDEO_FORMAT_GRAY16_LE:
+      frag_prog = g_strdup_printf (upload->priv->COMPOSE, 'a', 'r');

I'm not 100% sure if this is correct for big endian systems too. Maybe you need to do the inverse there, as GL formats are usually native-endianness.
Comment 3 comicfans44 2014-01-21 23:51:45 UTC
(In reply to comment #1)
> Please don't paste complete patches into Bugzilla comments :)

Thanks for advice. I'll pay attention to this.

(In reply to comment #2)
> Review of attachment 266835 [details] [review]:
> ...
> I'm not 100% sure if this is correct for big endian systems too. Maybe you need
> to do the inverse there, as GL formats are usually native-endianness.

OK I'll make some research and try to make it better...
Comment 4 comicfans44 2014-01-22 02:18:00 UTC
Created attachment 266940 [details] [review]
improved patch

(In reply to comment #2)
> Review of attachment 266835 [details] [review]:
> 
> Generally looks good but why not also add support to gldownload? :)
> 
> ::: gst-libs/gst/gl/gstglupload.c
> @@ +145,3 @@
> 
> +#define COMPOSE_WEIGHT \
> +    "const vec2 compose_weight = vec2(0.996109, 0.003891);\n"
> 
> Please write a comment where these magic numbers come from, it might not be
> obvious for everybody :)
> 


I created a improved patch with magic numbers comments ,please review it :)
I'll try to implement gldownload support later in separate  patch,
since I've not reached that code ...

> @@ +1063,3 @@
> +      break;
> +    case GST_VIDEO_FORMAT_GRAY16_LE:
> +      frag_prog = g_strdup_printf (upload->priv->COMPOSE, 'a', 'r');
> 
> I'm not 100% sure if this is correct for big endian systems too. Maybe you need
> to do the inverse there, as GL formats are usually native-endianness.

firstly, LUMINANCE_ALPHA seems the same byte order for LE/BE machine (not found this explicitly in glspec,neither google ,and not have such machine to test)

secondly, 'LE'  in GRAY16_LE means 'data stored byte order is LE', not means machine is LE(so is GRAY16_BE), so data is the same byte array on any machine.  ( video-format.c defined the read 16bit uint value from GRAY16_LE  as  GST_READ_UINT16_LE , this read native 16bit on LE machine, swap read on BE machine).

so I think there's no need to do byte swap when uploading texture. please correct me if there's misunderstood
Comment 5 Sebastian Dröge (slomo) 2014-01-22 08:42:00 UTC
commit 2afcb176f7d3c2cee80562aee01eb2579ea6ca24
Author: Wang Xin-yu (王昕宇) <comicfans44@gmail.com>
Date:   Fri Jan 17 08:46:02 2014 +0800

    glupload: Add GRAY8/GRAY16LE/BE upload support to glimagesink
    
    the 16bit data is uploaded as LUMINANCE_ALPHA, then expanded, composed
    in shader. value weight is a little complicate, high byte weight is
    255*256/65535 (denormalize to [0~255] ,shift to high byte,then normalize
    to [0~1]), low byte weight is 255/65535(similar)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722670
Comment 6 Matthew Waters (ystreet00) 2014-01-24 09:16:52 UTC
Created attachment 267113 [details] [review]
upload: use GL_UNSIGNED_SHORT for 16-bit formats

Just wondering, but there are 16-bit OpenGL formats available (GL_LUMINANCE16 + GL_UNSIGNED_SHORT) which would avoid the shader entirely for GRAY16.

Patch attached.
Comment 7 Sebastian Dröge (slomo) 2014-01-24 09:20:16 UTC
You need to swap the bytes for one of the variants though... but I think I missed that in the original patch too.
Comment 8 Sebastian Dröge (slomo) 2014-01-24 09:20:43 UTC
Ah no, the original patch had that.
Comment 9 Matthew Waters (ystreet00) 2014-01-24 21:37:26 UTC
Created attachment 267161 [details] [review]
upload: use GL_UNSIGNED_SHORT for 16-bit formats

(In reply to comment #7)
> You need to swap the bytes for one of the variants though... but I think I
> missed that in the original patch too.

Right, videotestsrc seems to put the same data in both bytes.

Anyway, endian-aware patch.
Comment 10 Sebastian Dröge (slomo) 2014-01-24 21:44:33 UTC
Review of attachment 267161 [details] [review]:

Looks good, please push :)
Comment 11 Matthew Waters (ystreet00) 2014-01-26 21:09:00 UTC
I can't push yet :)
Comment 12 Tim-Philipp Müller 2014-01-26 22:26:59 UTC
I get

  CC       libgstgl_1.0_la-gstglupload.lo
gstglupload.c: In function '_do_upload_make':
gstglupload.c:1249:32: error: 'GL_LUMINANCE16' undeclared (first use in this function)
       tex[0].internal_format = GL_LUMINANCE16;
                                ^
gstglupload.c:1249:32: note: each undeclared identifier is reported only once for each function it appears in
gstglupload.c: In function '_do_upload_fill':
gstglupload.c:1403:24: error: 'GL_UNPACK_SWAP_BYTES' undeclared (first use in this function)
       gl->PixelStorei (GL_UNPACK_SWAP_BYTES, GL_TRUE);
                        ^
with that patch (with HAVE: GLES2, WINDOW_X11/WAYLAND, PLATFORM_EGL, GLEGLIMAGEOES, GLCHAR, GLSIZEIPTR, GLINTPTR as per gstglconfig.h).
Comment 13 Matthew Waters (ystreet00) 2014-01-27 20:25:01 UTC
(In reply to comment #12)
> I get
> 
>   CC       libgstgl_1.0_la-gstglupload.lo
> gstglupload.c: In function '_do_upload_make':
> gstglupload.c:1249:32: error: 'GL_LUMINANCE16' undeclared (first use in this
> function)
>        tex[0].internal_format = GL_LUMINANCE16;
>                                 ^
> gstglupload.c:1249:32: note: each undeclared identifier is reported only once
> for each function it appears in
> gstglupload.c: In function '_do_upload_fill':
> gstglupload.c:1403:24: error: 'GL_UNPACK_SWAP_BYTES' undeclared (first use in
> this function)
>        gl->PixelStorei (GL_UNPACK_SWAP_BYTES, GL_TRUE);
>                         ^
> with that patch (with HAVE: GLES2, WINDOW_X11/WAYLAND, PLATFORM_EGL,
> GLEGLIMAGEOES, GLCHAR, GLSIZEIPTR, GLINTPTR as per gstglconfig.h).

Right, so GLES2 doesn't have SWAP_BYTES, which means it's probably not viable to use the patch anymore.  Carry on :)
Comment 14 Matthew Waters (ystreet00) 2014-01-27 20:28:57 UTC
Review of attachment 267161 [details] [review]:

Needs work for GLES2
Comment 15 Matthew Waters (ystreet00) 2014-01-27 20:52:14 UTC
Created attachment 267341 [details] [review]
upload: fix compilation for GLES2

This is needed for GLES2 to compile though.
Comment 16 Sebastian Dröge (slomo) 2014-01-29 19:14:18 UTC
Comment on attachment 267341 [details] [review]
upload: fix compilation for GLES2

Please put the other patch in a separate bug if there's still interest in fixing that