Alejandro Piñeiro <apinhe...@igalia.com> writes: > On 02/09/16 23:13, Kenneth Graunke wrote: >> On Friday, August 26, 2016 10:49:18 PM PDT Kenneth Graunke wrote: >>> This fixes a numerical precision issue that was causing two CTS >>> failures: >>> >>> ES31-CTS.blend_equation_advanced.blend_specific.GL_COLORBURN_KHR >>> ES31-CTS.blend_equation_advanced.blend_all.GL_COLORBURN_KHR_all_qualifier > > FWIW: it fixes the equivalent GL44 tests. > >>> >>> When blending with GL_COLORDODGE_KHR and these colors: >> >> This should be GL_COLORBURN_KHR and the subject should be >> blend_colorburn. Fixed locally. > > Gentle reminder to use the fixed version. > >>> >>> 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 imprecisions in the >>> alpha scaling, presumably), which failed 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. >>> >>> To work around this, compare with 1.0 - FLT_EPSILON. > > Makes sense: > Reviewed-by: Alejandro Piñeiro <apinhe...@igalia.com> > > Having said so, Kenneth mentioned on IRC that Francisco has some doubts > on this patch. CCing him just in case he wants to mention something. > Heh, right, my concern was that this smells strongly like a test relying on not terribly well-defined behavior... AFAICT the problem addressed here is ultimately caused by the discontinuity that the COLORBURN blending equation has at the point Cd = 1, Cs = 0, and the test authors had the awesome idea [not necessarily being sarcastic here ;)] of testing the blending function at precisely that point, even though the function is guaranteed to be numerically unstable and vary wildly given the slightest rounding error.
Does the extension impose any requirements on the precision of the division by alpha operation done on pre-multiplied color components? The test case may be valid assuming that IEEE precision rules apply, but AFAIK GLSL has considerably looser requirements on the division operation, and the KHR_blend_equation_advanced lowering code is implemented in terms of GLSL division so the result could potentially be farther off than 1 - epsilon (though AFAICT this change would be correct assuming the result of GLSL division is guaranteed to be within ~1.5 ULP of the exact value, which I don't think is the case). >>> >>> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> >>> --- >>> src/compiler/glsl/lower_blend_equation_advanced.cpp | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/compiler/glsl/lower_blend_equation_advanced.cpp >>> b/src/compiler/glsl/lower_blend_equation_advanced.cpp >>> index a998df1..f8b0261 100644 >>> --- a/src/compiler/glsl/lower_blend_equation_advanced.cpp >>> +++ b/src/compiler/glsl/lower_blend_equation_advanced.cpp >>> @@ -28,6 +28,7 @@ >>> #include "program/prog_instruction.h" >>> #include "program/prog_statevars.h" >>> #include "util/bitscan.h" >>> +#include <float.h> >>> >>> using namespace ir_builder; >>> >>> @@ -101,7 +102,7 @@ blend_colorburn(ir_variable *src, ir_variable *dst) >>> * 1 - min(1,(1-Cd)/Cs), if Cd < 1 and Cs > 0 >>> * 0, if Cd < 1 and Cs <= 0 >>> */ >>> - return csel(gequal(dst, imm3(1)), imm3(1), >>> + return csel(gequal(dst, imm3(1 - FLT_EPSILON)), imm3(1), >>> csel(lequal(src, imm3(0)), imm3(0), >>> sub(imm3(1), min2(imm3(1), div(sub(imm3(1), dst), >>> src))))); >>> } >>> >> >> >> >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > -- > Alejandro Piñeiro <apinhe...@igalia.com>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
