[Mesa-stable] [PATCH] glsl: Improve accuracy of alpha scaling in advanced blend lowering.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> When blending with GL_COLORBURN_KHR and these colors:
>
>    dst = <0.372549027, 0.372549027, 0.372549027, 0.372549027>
>    src = <0.09375, 0.046875, 0.0, 0.375>
>
> the normalized dst value became 0.99999994 (due to precision problems
> in the floating point divide of rgb by alpha).  This caused the color
> burn equation to fail the dst >= 1.0 comparison.  The blue channel would
> then fall through to the dst < 1.0 && src >= 0 comparison, which was
> true, since src.b == 0.  This produced a factor of 0.0 instead of 1.0.
>
> This is an inherent numerical instability in the color burn and dodge
> equations - depending on the precision of alpha scaling, the value can
> be either 0.0 or 1.0.  Technically, GLSL floating point division doesn't
> even guarantee that 0.372549027 / 0.372549027 = 1.0.  So arguably, the
> CTS should allow either value.  I've filed a bug at Khronos for further
> discussion (linked below).
>
> In the meantime, this patch improves the precision of alpha scaling by
> replacing the division with (rgb == alpha ? 1.0 : rgb / alpha).  We may
> not need this long term, but for now, it fixes the following CTS tests:
>
> ES31-CTS.blend_equation_advanced.blend_specific.GL_COLORBURN_KHR
> ES31-CTS.blend_equation_advanced.blend_all.GL_COLORBURN_KHR_all_qualifier
>
> Cc: currojerez at riseup.net
> Cc: mesa-stable at lists.freedesktop.org
> Bugzilla: https://cvs.khronos.org/bugzilla/show_bug.cgi?id=16042
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/compiler/glsl/lower_blend_equation_advanced.cpp | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/glsl/lower_blend_equation_advanced.cpp b/src/compiler/glsl/lower_blend_equation_advanced.cpp
> index 1d03392..f8210e3 100644
> --- a/src/compiler/glsl/lower_blend_equation_advanced.cpp
> +++ b/src/compiler/glsl/lower_blend_equation_advanced.cpp
> @@ -308,12 +308,18 @@ calc_blend_result(ir_factory f,
>     f.emit(assign(dst_alpha, swizzle_w(fb)));
>     f.emit(if_tree(equal(dst_alpha, imm1(0)),
>                       assign(dst_rgb, imm3(0)),
> -                     assign(dst_rgb, div(swizzle_xyz(fb), dst_alpha))));
> +                     assign(dst_rgb, csel(equal(swizzle_xyz(fb),
> +                                                swizzle(fb, SWIZZLE_WWWW, 3)),
> +                                          imm3(1),
> +                                          div(swizzle_xyz(fb), dst_alpha)))));
>  
>     f.emit(assign(src_alpha, swizzle_w(src)));
>     f.emit(if_tree(equal(src_alpha, imm1(0)),
>                       assign(src_rgb, imm3(0)),
> -                     assign(src_rgb, div(swizzle_xyz(src), src_alpha))));
> +                     assign(src_rgb, csel(equal(swizzle_xyz(src),
> +                                                swizzle(src, SWIZZLE_WWWW, 3)),
> +                                          imm3(1),
> +                                          div(swizzle_xyz(src), src_alpha)))));
>  
>     ir_variable *factor = f.make_temp(glsl_type::vec3_type, "__blend_factor");
>  

Looks reasonable to me, patch is:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

> -- 
> 2.10.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20161028/599e636f/attachment.sig>