[Mesa-dev] [PATCH] glsl: Make blend_colordodge compare against 1.0 - FLT_EPSILON.
Francisco Jerez
currojerez at riseup.net
Sat Sep 10 21:56:43 UTC 2016
Alejandro Piñeiro <apinheiro at igalia.com> writes:
> 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?
>
Uhm... That spec text seems vague enough that I wouldn't bet on any of
the following being the intended interpretation:
- Alpha division is supposed to be calculated with the same precision
as the computations from table X.1, and give a result within 0.5 ULP
of the exact value, which is stricter than GLSL division but would
imply that the CTS test case is correct. If this is the case
implementing alpha division as GLSL division wouldn't be quite right,
but you could potentially conceal the problem by shifting the
discontinuity as done in this patch, though the shift would likely
have to be larger than FLT_EPSILON to account for the (worse than
IEEE) accuracy of GLSL division. According to the GLSL spec division
has a maximum error of 2.5 ULP, which sounds like you'd need to
compare against '1 - 2 epsilon' instead?
If this is the case, wouldn't you need to apply a similar change to
the COLORDODGE equation too? It looks like it has a discontinuity
similar to the one of COLORBURN, but at a different point Cd=0, Cs=1.
- Alpha division is expected to have the same accuracy as GLSL
division, or some other unspecified accuracy lower than the accuracy
of the computations from table X.1. In that case the result of the
COLORBURN blending function is effectively unspecified at Cd=1, Cs=0,
and the test is wrong.
>>>>>
>>>>> 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160910/07062452/attachment.sig>
More information about the mesa-dev
mailing list