[Mesa-dev] [PATCH] fix minor error in YUV2RGB matrix used in shader

[Kristian H. Kristensen]

Johnson Lin <johnson.lin at intel.com> writes:

> The matrix used for YCbCr to RGB is listed in Wiki https://en.wikipedia.org/wiki/YCbCr;
> There is minor error in the matrix constant: 0.0625=16/256 should be 16.0/255,
>  and 0.5=128.0/256 should be 128.0/255.
> Note that conversion from a 0-255 byte number to 0-1.0 float is to divide by 255
>  instead of 256. That's we get 255=1.0f.
> By the constant change we can see the CSC result is bit aligned with
> Wiki conversion result and FFMPeg result.
> Otherwise in some situation, there will be one bit difference

Thanks for fixing this.  Try to wrap the the commit message so it fits
in 80 columns, eg:

The matrix used for YCbCr to RGB is listed in:

  https://en.wikipedia.org/wiki/YCbCr;

There is minor error in the matrix constant: 0.0625=16/256 should be
16.0/255, and 0.5=128.0/256 should be 128.0/255.  Note that conversion
from a 0-255 byte number to 0-1.0 float is to divide by 255 instead of
256. That's we get 255=1.0f.

By the constant change we can see the CSC result is bit aligned with
Wiki conversion result and FFMPeg result.  Otherwise in some situation,
there will be one bit difference

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100854
> ---
>  src/compiler/nir/nir_lower_tex.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/nir/nir_lower_tex.c b/src/compiler/nir/nir_lower_tex.c
> index 352d1499bc8d..f20425e84aab 100644
> --- a/src/compiler/nir/nir_lower_tex.c
> +++ b/src/compiler/nir/nir_lower_tex.c
> @@ -244,9 +244,9 @@ convert_yuv_to_rgb(nir_builder *b, nir_tex_instr *tex,
>     nir_ssa_def *yuv =
>        nir_vec4(b,
>                 nir_fmul(b, nir_imm_float(b, 1.16438356f),
> -                        nir_fadd(b, y, nir_imm_float(b, -0.0625f))),
> -               nir_channel(b, nir_fadd(b, u, nir_imm_float(b, -0.5f)), 0),
> -               nir_channel(b, nir_fadd(b, v, nir_imm_float(b, -0.5f)), 0),
> +                        nir_fadd(b, y, nir_imm_float(b, -16.0f/255))),
> +               nir_channel(b, nir_fadd(b, u, nir_imm_float(b, -128.0f/255)), 0),
> +               nir_channel(b, nir_fadd(b, v, nir_imm_float(b, -128.0f/255)), 0),

I'd like to make two changes here: use a float constant (255.0f instead
of 255) and add spaces around the '/'. That is:

    nir_channel(b, nir_fadd(b, v, nir_imm_float(b, -128.0f / 255.0f)), 0),

with that and the 'nir:' prefix in the commit subject:

Reviewed-by: Kristian H. Kristensen <hoegsberg at google.com>

>                 nir_imm_float(b, 0.0));
>  
>     nir_ssa_def *red = nir_fdot4(b, yuv, nir_build_imm(b, 4, 32, m[0]));
> -- 
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev