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

Sat Sep 10 15:39:29 UTC 2016

On 09/09/16 07:33, Francisco Jerez wrote:
> 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.

Ok, thanks for the explanation.

> Does the extension impose any requirements on the precision of the
> division by alpha operation done on pre-multiplied color components?

There are only two mentions to precision on the extension spec:

1. At the Overview:

"Additionally, blending precision may be limited to 16-bit
 floating-point, which could result in a loss of precision and dynamic
 range for framebuffer formats with 32-bit floating-point components,
 and in a loss of precision for formats with 12- and 16-bit signed or
 unsigned normalized integer components."

2. At "Additions to Chapter 4 of the OpenGL 4.1 Specification
(Per-Fragment Operations and the Frame Buffer)"

"(modify the third paragraph, p. 361, specifying minimum precision and
 dynamic range for blend operations) ... Blending computations are
 treated as if carried out in floating-point.  For the equations in
 table 4.1, blending computations will be performed with a precision
 and dynamic range no lower than that used to represent destination
 components.  For the equations in table X.1 and X.2, blending
 computations will be performed with a precision and dynamic range no
 lower than the smaller of that used to represent destination
 components or that used to represent 16-bit floating-point values as
 described in section 2.1.1."

Such table 4.1 doesn't include alpha division (as far as I saw on a skim
reading, that alpha dividing is part of the advanced blend extension).

> 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).

Well, as far as I see the extension spec doesn't enter too much into
details about precision, or at least on this specific case.

Any suggestion about how to proceed?

>>>>
>>>> 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>

-- 
Alejandro Piñeiro <apinheiro at igalia.com>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160910/f3daf69c/attachment.sig>