[vlc-devel] [PATCH 2/7] opengl: apply chroma conversion using a matrix

Alexandre Janniaux ajanni at videolabs.io
Thu Jan 23 11:45:52 CET 2020


Hi,

Comments inline,

On Wed, Jan 22, 2020 at 01:33:44PM +0100, Romain Vimont wrote:
> Use native matrix multiplication to apply the chroma conversion.
>
> Concretely, the generated fragment shader looked like:
>
>     uniform vec4 Coefficients[4];
>     uniform vec4 FillColor;
>     void main(void) {
>      float val;vec4 colors;
>      colors = texture2D(Texture0, TexCoord0);
>      val = colors.r;
>      vec4 color0 = vec4(val, val, val, 1);
>      colors = texture2D(Texture1, TexCoord1);
>      val = colors.r;
>      vec4 color1 = vec4(val, val, val, 1);
>      val = colors.g;
>      vec4 color2 = vec4(val, val, val, 1);
>      vec4 result = (color0 * Coefficients[0]) + Coefficients[3];
>      result = (color1 * Coefficients[1]) + result;
>      result = (color2 * Coefficients[2]) + result;
>      result = _main_0_0(result);
>      gl_FragColor = result * FillColor;
>     }
>
> Now, it looks like:
>
>     uniform mat4 ConvMatrix;
>     uniform vec4 FillColor;
>     void main(void) {
>      vec4 texel;
>      vec4 pixel = vec4(0.0, 0.0, 0.0, 1.0);
>      texel = texture2D(Texture0, TexCoord0);
>      pixel[0] = texel.r;
>      texel = texture2D(Texture1, TexCoord1);
>      pixel[1] = texel.r;
>      pixel[2] = texel.g;
>      vec4 result = ConvMatrix * pixel;
>      result = _main_0_0(result);
>      gl_FragColor = result * FillColor;
>     }
> ---
>  modules/video_output/opengl/converter.h       |  2 +-
>  .../video_output/opengl/fragment_shaders.c    | 55 ++++++++-----------
>  2 files changed, 24 insertions(+), 33 deletions(-)
>
> diff --git a/modules/video_output/opengl/converter.h b/modules/video_output/opengl/converter.h
> index ceb515d268..3e79f1eda3 100644
> --- a/modules/video_output/opengl/converter.h
> +++ b/modules/video_output/opengl/converter.h
> @@ -68,7 +68,7 @@ struct opengl_tex_converter_t
>      struct {
>          GLint Texture[PICTURE_PLANE_MAX];
>          GLint TexSize[PICTURE_PLANE_MAX]; /* for GL_TEXTURE_RECTANGLE */
> -        GLint Coefficients;
> +        GLint ConvMatrix;
>          GLint FillColor;
>          GLint *pl_vars; /* for pl_sh_res */
>      } uloc;
> diff --git a/modules/video_output/opengl/fragment_shaders.c b/modules/video_output/opengl/fragment_shaders.c
> index ff5481a50b..46b65aa69b 100644
> --- a/modules/video_output/opengl/fragment_shaders.c
> +++ b/modules/video_output/opengl/fragment_shaders.c
> @@ -90,8 +90,12 @@ tc_yuv_base_init(opengl_tex_converter_t *tc, vlc_fourcc_t chroma,
>          /* We place coefficient values for coefficient[4] in one array from
>           * matrix values. Notice that we fill values from top down instead
>           * of left to right.*/
> -        for (int j = 0; j < 4; j++)
> -            tc->yuv_coefficients[i*4+j] = j < 3 ? correction * matrix[j*4+i] : 0.f;
> +        for (int j = 0; j < 3; j++)
> +            tc->yuv_coefficients[i*4+j] = correction * matrix[j*4+i];
> +        tc->yuv_coefficients[3] = 0.f;
> +        tc->yuv_coefficients[7] = 0.f;
> +        tc->yuv_coefficients[11] = 0.f;
> +        tc->yuv_coefficients[15] = 1.f;
>      }
>
>      tc->yuv_color = true;
> @@ -108,9 +112,9 @@ tc_base_fetch_locations(opengl_tex_converter_t *tc, GLuint program)
>
>      if (tc->yuv_color)
>      {
> -        tc->uloc.Coefficients = tc->vt->GetUniformLocation(program,
> -                                                            "Coefficients");
> -        if (tc->uloc.Coefficients == -1)
> +        tc->uloc.ConvMatrix = tc->vt->GetUniformLocation(program,
> +                                                         "ConvMatrix");
> +        if (tc->uloc.ConvMatrix == -1)
>              return VLC_EGENERIC;
>      }
>
> @@ -154,7 +158,8 @@ tc_base_prepare_shader(const opengl_tex_converter_t *tc,
>      const struct vlc_gl_interop *interop = tc->interop;
>
>      if (tc->yuv_color)
> -        tc->vt->Uniform4fv(tc->uloc.Coefficients, 4, tc->yuv_coefficients);
> +        tc->vt->UniformMatrix4fv(tc->uloc.ConvMatrix, 1, GL_FALSE,
> +                                 tc->yuv_coefficients);
>
>      for (unsigned i = 0; i < interop->tex_count; ++i)
>          tc->vt->Uniform1i(tc->uloc.Texture[i], i);
> @@ -481,7 +486,7 @@ opengl_fragment_shader_init(opengl_tex_converter_t *tc, GLenum tex_target,
>      }
>
>      if (is_yuv)
> -        ADD("uniform vec4 Coefficients[4];\n");
> +        ADD("uniform mat4 ConvMatrix;\n");
>
>      ADD("uniform vec4 FillColor;\n"
>          "void main(void) {\n");
> @@ -495,52 +500,38 @@ opengl_fragment_shader_init(opengl_tex_converter_t *tc, GLenum tex_target,
>
>      unsigned color_count;
>      if (is_yuv) {
> -        ADD(" float val;\n"
> -            " vec4 colors;\n");
> +        ADD(" vec4 texel;\n"
> +            " vec4 pixel = vec4(0.0, 0.0, 0.0, 1.0);\n");
>          unsigned color_idx = 0;
>          for (unsigned i = 0; i < interop->tex_count; ++i)
>          {
>              const char *swizzle = swizzle_per_tex[i];
>              assert(swizzle);
>              size_t swizzle_count = strlen(swizzle);
> -            ADDF(" colors = %s(Texture%u, %s%u);\n", lookup, i, coord_name, i);
> +            ADDF(" texel = %s(Texture%u, %s%u);\n", lookup, i, coord_name, i);
>              for (unsigned j = 0; j < swizzle_count; ++j)
>              {
> -                ADDF(" val = colors.%c;\n"
> -                     " vec4 color%u = vec4(val, val, val, 1);\n",
> -                     swizzle[j], color_idx);
> +                ADDF(" pixel[%u] = texel.%c;\n", color_idx, swizzle[j]);

Maybe you can indicate that the indexing here (pixel[%u]) is
to change pixel value in the vector pixel. Not really important
though but it was confusing for reviewing. Otherwise, best
would be to use the swizzle syntax on the left operand too and
would remove the need for the second loop. :)

>                  color_idx++;
>                  assert(color_idx <= PICTURE_PLANE_MAX);
>              }
>          }
> -        ADD(" vec4 result = (color0 * Coefficients[0]) + Coefficients[3];\n");
> +        if (yuv_swap_uv) {
> +            ADD(" pixel = pixel.xzyw;\n");
> +        }

The change above would also remoe the need to handle
yuv_swap_uv here and below.

> +        ADD(" vec4 result = ConvMatrix * pixel;\n");
>          color_count = color_idx;
>      }
>      else
>      {
>          ADDF(" vec4 result = %s(Texture0, %s0);\n", lookup, coord_name);
> +        if (yuv_swap_uv) {

Can you have yuv_swap_uv even if `is_yuv` is false? In that
case maybe the variable should be renamed.


> +            ADD(" result = result.xzyw;\n");
> +        }
>          color_count = 1;
>      }
>      assert(yuv_space == COLOR_SPACE_UNDEF || color_count == 3);
>
> -    for (unsigned i = 1; i < color_count; ++i)
> -    {
> -        unsigned color_idx;
> -        if (yuv_swap_uv)
> -        {
> -            assert(color_count == 3);
> -            color_idx = (i % 2) + 1;
> -        }
> -        else
> -            color_idx = i;
> -
> -        if (is_yuv)
> -            ADDF(" result = (color%u * Coefficients[%u]) + result;\n",
> -                 color_idx, i);
> -        else
> -            ADDF(" result = color%u + result;\n", color_idx);
> -    }
> -
>  #ifdef HAVE_LIBPLACEBO
>      if (tc->pl_sh_res) {
>          const struct pl_shader_res *res = tc->pl_sh_res;
> --
> 2.25.0
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list