[Mesa-dev] [PATCH] glsl: Make blend_colordodge compare against 1.0 - FLT_EPSILON.

[Francisco Jerez]

Alejandro Piñeiro <apinheiro at 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 <apinheiro at 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 <kenneth at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> 
>
> -- 
> Alejandro Piñeiro <apinheiro at igalia.com>
-------------- 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-dev/attachments/20160908/99cf57a9/attachment.sig>