[Mesa-dev] [PATCH 1/4] vl: Apply luma key filter before CSC conversion

[Christian König]

Am 07.06.2016 um 13:28 schrieb Nayan Deshmukh:
> Apply the luma key filter to the YCbCr values during the CSC conversion
> in video buffer shader. The initial values of max and min luma are set
> to opposite values to disable the filter initially and will be set when
> enabling it.
>
> Signed-off-by: Nayan Deshmukh <nayan26deshmukh at gmail.com>

Really nice that you got that working.

First of all squash patches #1,#2 and #3 together into one. A single 
patch should never break the build of other components.

A few more notes on the code below:

> ---
>   src/gallium/auxiliary/vl/vl_compositor.c | 40 ++++++++++++++++++++------------
>   src/gallium/auxiliary/vl/vl_compositor.h |  2 +-
>   2 files changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/src/gallium/auxiliary/vl/vl_compositor.c b/src/gallium/auxiliary/vl/vl_compositor.c
> index acb2f4f..05c2d5a 100644
> --- a/src/gallium/auxiliary/vl/vl_compositor.c
> +++ b/src/gallium/auxiliary/vl/vl_compositor.c
> @@ -130,8 +130,9 @@ create_frag_shader_video_buffer(struct vl_compositor *c)
>   {
>      struct ureg_program *shader;
>      struct ureg_src tc;
> -   struct ureg_src csc[3];
> +   struct ureg_src csc[4];

Better give the luma key parameters its own variable, cause it's not 
really part of the CSC matrix. Something like:

+   struct ureg_src lumakey;

>      struct ureg_src sampler[3];
> +   struct ureg_dst t_tex[3];

Maybe call that temp or something like this.

>      struct ureg_dst texel;
>      struct ureg_dst fragment;
>      unsigned i;
> @@ -142,9 +143,13 @@ create_frag_shader_video_buffer(struct vl_compositor *c)
>   
>      tc = ureg_DECL_fs_input(shader, TGSI_SEMANTIC_GENERIC, VS_O_VTEX, TGSI_INTERPOLATE_LINEAR);
>      for (i = 0; i < 3; ++i) {
> -      csc[i] = ureg_DECL_constant(shader, i);
>         sampler[i] = ureg_DECL_sampler(shader, i);
> +      t_tex[i] = ureg_DECL_temporary(shader);
>      }
> +
> +   for (i = 0; i < 4; ++i)
> +       csc[i] = ureg_DECL_constant(shader, i);
> +

Then this change becomes:

lumakey = uref_DECL_constant(shader, 3);

>      texel = ureg_DECL_temporary(shader);
>      fragment = ureg_DECL_output(shader, TGSI_SEMANTIC_COLOR, 0);
>   
> @@ -156,12 +161,16 @@ create_frag_shader_video_buffer(struct vl_compositor *c)
>         ureg_TEX(shader, ureg_writemask(texel, TGSI_WRITEMASK_X << i), TGSI_TEXTURE_2D_ARRAY, tc, sampler[i]);
>   
>      ureg_MOV(shader, ureg_writemask(texel, TGSI_WRITEMASK_W), ureg_imm1f(shader, 1.0f));
> +   ureg_DP4(shader, ureg_writemask(t_tex[0], TGSI_WRITEMASK_W), ureg_src(texel), ureg_imm4f(shader, 0,0,1,0));
> +   ureg_DP4(shader, ureg_writemask(t_tex[1], TGSI_WRITEMASK_W), csc[3], ureg_imm4f(shader, 1,0,0,0));
> +   ureg_DP4(shader, ureg_writemask(t_tex[2], TGSI_WRITEMASK_W), csc[3], ureg_imm4f(shader, 0,1,0,0));

Well that made me smile :) What you are missing here is the function 
ureg_swizzle().

This function can move the different channels around without the use of 
a DP4 instruction.

> +   ureg_SLE(shader, t_tex[1], ureg_src(t_tex[0]), ureg_src(t_tex[1]));
> +   ureg_SGT(shader, t_tex[2], ureg_src(t_tex[0]), ureg_src(t_tex[2]));
> +   ureg_MAX(shader, ureg_writemask(fragment, TGSI_WRITEMASK_W), ureg_src(t_tex[1]), ureg_src(t_tex[2]));
>   
>      for (i = 0; i < 3; ++i)
>         ureg_DP4(shader, ureg_writemask(fragment, TGSI_WRITEMASK_X << i), csc[i], ureg_src(texel));
>   
> -   ureg_MOV(shader, ureg_writemask(fragment, TGSI_WRITEMASK_W), ureg_imm1f(shader, 1.0f));
> -

Better move calculating the W component here where we have set it 
previously.

And you should release your temporary here again after using it.

Cheers,
Christian.

>      ureg_release_temporary(shader, texel);
>      ureg_END(shader);
>   
> @@ -852,20 +861,21 @@ vl_compositor_cleanup(struct vl_compositor *c)
>   }
>   
>   void
> -vl_compositor_set_csc_matrix(struct vl_compositor_state *s, vl_csc_matrix const *matrix)
> +vl_compositor_set_csc_matrix(struct vl_compositor_state *s, vl_csc_matrix const *matrix, float luma_min, float luma_max)
>   {
>      struct pipe_transfer *buf_transfer;
>   
>      assert(s);
>   
> -   memcpy
> -   (
> -      pipe_buffer_map(s->pipe, s->csc_matrix,
> -                      PIPE_TRANSFER_WRITE | PIPE_TRANSFER_DISCARD_RANGE,
> -                      &buf_transfer),
> -      matrix,
> -      sizeof(vl_csc_matrix)
> -   );
> +   float *ptr = pipe_buffer_map(s->pipe, s->csc_matrix,
> +                               PIPE_TRANSFER_WRITE | PIPE_TRANSFER_DISCARD_RANGE,
> +                               &buf_transfer);
> +
> +   memcpy(ptr, matrix, sizeof(vl_csc_matrix));
> +
> +   ptr += sizeof(vl_csc_matrix)/sizeof(float);
> +   ptr[0] = luma_min;
> +   ptr[1] = luma_max;
>   
>      pipe_buffer_unmap(s->pipe, buf_transfer);
>   }
> @@ -1142,13 +1152,13 @@ vl_compositor_init_state(struct vl_compositor_state *s, struct pipe_context *pip
>         pipe->screen,
>         PIPE_BIND_CONSTANT_BUFFER,
>         PIPE_USAGE_DEFAULT,
> -      sizeof(csc_matrix)
> +      sizeof(csc_matrix) + 2*sizeof(float)
>      );
>   
>      vl_compositor_clear_layers(s);
>   
>      vl_csc_get_matrix(VL_CSC_COLOR_STANDARD_IDENTITY, NULL, true, &csc_matrix);
> -   vl_compositor_set_csc_matrix(s, (const vl_csc_matrix *)&csc_matrix);
> +   vl_compositor_set_csc_matrix(s, (const vl_csc_matrix *)&csc_matrix, 1.f, 0.f);
>   
>      return true;
>   }
> diff --git a/src/gallium/auxiliary/vl/vl_compositor.h b/src/gallium/auxiliary/vl/vl_compositor.h
> index 934b634..23cab6c 100644
> --- a/src/gallium/auxiliary/vl/vl_compositor.h
> +++ b/src/gallium/auxiliary/vl/vl_compositor.h
> @@ -138,7 +138,7 @@ vl_compositor_init_state(struct vl_compositor_state *state, struct pipe_context
>    * set yuv -> rgba conversion matrix
>    */
>   void
> -vl_compositor_set_csc_matrix(struct vl_compositor_state *settings, const vl_csc_matrix *matrix);
> +vl_compositor_set_csc_matrix(struct vl_compositor_state *settings, const vl_csc_matrix *matrix, float luma_min, float luma_max);
>   
>   /**
>    * reset dirty area, so it's cleared with the clear colour