[vlc-devel] [PATCH 2/7] opengl: apply chroma conversion using a matrix
Romain Vimont
rom1v at videolabs.io
Thu Jan 23 11:56:59 CET 2020
On Thu, Jan 23, 2020 at 11:45:52AM +0100, Alexandre Janniaux wrote:
> Hi,
>
> Comments inline,
Thanks for the review.
>
> 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. :)
You mean replacing (for example):
pixel[1] = texel.x;
pixel[2] = texel.y;
by:
pixel.yz = texel.xy;
This would simplify the fragment shader, but complexify its code
generation. I'm not sure it is worth it.
>
> > 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.
This piece of code is temporary (not to break the behavior between
patches), it is removed by patch 7.
>
> > + 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.
No, yuv_swap_uv may only be set if is_yuv.
>
>
> > + 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
> _______________________________________________
> 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