[Bug 703346] Integrate shaders from eglglessink

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #4 from Matthew Waters <ystreet00 at gmail.com> 2013-07-18 13:45:31 UTC ---
(In reply to comment #3)
> Review of attachment 249483 [details]:
> 
> ::: 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

Doesn't really matter much (actually not at all).

> @@ +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

Yea sure you can try and mix and match texture formats to get the result
however chances are you'll hit the slow path in most drivers because it has to
convert CPU side to a format that the GPU can use.  It's simpler to use a
shader and may even be faster.  If you really want to check, profile it.

> @@ +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

I just put them in based on what was in the others.

> @@ +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)

Can do that later.

> @@ +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

Correct, another patch.

> @@ +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?

That might need changing.  Anyone got a big-endian machine?

> @@ +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

Do that later.  This is the general case.

> @@ +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.

The bilinear interpolation comes from the texture parameters, not from what you
specify in GL_UNPACK/strides.  Arbitrary strides should come with the patch
that enables GstVideoInfo/GstVideoFrame from the caller.

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

And breaks the resulting image for video formats with subsampling.

> ::: 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)

Later.

-- 
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.