[Mesa-dev] [PATCH] glsl: Trigger precision mismatch error only if both symbols are used

Tue Sep 26 11:30:19 UTC 2017

Hi,

On 26.09.2017 11:33, Tomasz Figa wrote:
> An update on my testing inline.
> 
> On Tue, Sep 26, 2017 at 7:13 AM, Tomasz Figa <tfiga at chromium.org> wrote:
>> On Tue, Sep 26, 2017 at 6:21 AM, Tomasz Figa <tfiga at chromium.org> wrote:
>>> Hi Ian,
>>>
>>> On Tue, Sep 26, 2017 at 5:59 AM, Ian Romanick <idr at freedesktop.org> wrote:
>>>> On 09/25/2017 02:53 AM, Tomasz Figa wrote:
>>>>> Commit 259fc505454ea6a67aeacf6cdebf1398d9947759 added linker error for
>>>>> mismatching uniform precision, as required by GLES specification and
>>>>> conformance test-suite.
>>>>>
>>>>> Several Android applications, including Forge of Empires, have shaders
>>>>> which violate this rule, on a dead varying that will be eliminated.
>>>>> The problem affects a big number of applications using Cocos2D engine
>>>>> and other GLES implementations accept this, so work around the problem
>>>>> by erroring out only if both symbols are actually referenced in the
>>>>> code, which is the only case when the mismatch can cause incorrect
>>>>> behavior.
>>>>
>>>> I think this change will cause failures in the CTS... but, there's a
>>>> Khronos bug about this issue, and I think some of the rules are going to
>>>> get relaxed.  I'd like to wait until that gets sorted before we start
>>>> changing how our linker applies the rules.
>>>
>>> Are we 100% sure about the failures? There are multiple GLES
>>> implementations that claim to be compliant, yet they allow such
>>> behavior.
>>
>> I'm looking through Khronos CTS github repository and it seems like a
>> related test (dEQP-GLES3.functional.shaders.linkage.uniform.block.differing_precision)
>> was removed from GLES 3 suite and instead few others added to GLES 3.1
>> suite:
>>
>> https://github.com/KhronosGroup/VK-GL-CTS/commit/b8d0d0a842280c2594703ecce985563aebe1cde8#diff-72bd3f02df978e071f2a53ec2654ddd5
>>
>> Moreover I can see a test being included in GLES 3.2 suite
>> (KHR-GLES32.shaders.negative.unused_uniform_precision_matching) that
>> prohibits precision mismatch even for unused uniforms, but it is not
>> included in any suites for earlier GLES versions.
> 
> Just ran the whole suite for GLES 3.0 conformance run with i965 driver
> on Android and there is only 1 failure that seems to be unrelated to
> the workaround (fails both with and without the change):
> 
> KHR-GLES3.shaders.uniform_block.common.precision_matching
> 
> However, I just realized that the applications affected are all using
> GLSL ES 1.00. So, if we turn the condition introduced by my patch into
> "(existing->data.used && var->data.used) || prog->data->Version >=
> 300", we should be safe both on compatibility and conformance side.

Btw. If you're also interested about performance, there's a difference 
between GLES 3.x and earlier GLES version semantics that affects 
performance.

According to the GLES 3.0 spec, cubemap sampling is required to always 
be seamless (sampling from multiple surfaces near the edges), but in 
earlier GLES versions, this isn't required (on GL side, it's application 
decision, so there it doesn't matter).

For anything using 1x1 / 2x2 cubemaps, this can have clear performance. 
Do many Android applications use them?

This is relevant as Mesa uses GLES 3 semantics also for GLES 2:
------------- src/mesa/main/texstate.c -------------
    /* Appendix F.2 of the OpenGL ES 3.0 spec says:
     *
     *     "OpenGL ES 3.0 requires that all cube map filtering be
     *     seamless. OpenGL ES 2.0 specified that a single cube map face be
     *     selected and used for filtering."
     *
     * Unfortunatley, a call to _mesa_is_gles3 below will only work if
     * the driver has already computed and set ctx->Version, however drivers
     * seem to call _mesa_initialize_context (which calls this) early
     * in the CreateContext hook and _mesa_compute_version much later (since
     * it needs information about available extensions). So, we will
     * enable seamless cubemaps by default since GLES2. This should work
     * for most implementations and drivers that don't support seamless
     * cubemaps for GLES2 can still disable it.
     */
    ctx->Texture.CubeMapSeamless = ctx->API == API_OPENGLES2;
----------------------------------------------------

	- Eero

>>> Note that this is affecting a huge number of Android applications,
>>> including a commonly used 2D game engine and we're having a really big
>>> problem right now, because even if the engine gets fixed, it's very
>>> unlikely that all the developers update their engine.
>>>
>>>>
>>>>> Based on Kenneth Graunke's patch from Bugzilla, reworked from a drirc
>>>>> option that completely bypasses the check into an incoditional check
>>>>
>>>>                                                      unconditional
>>>>
>>>>> that triggers either an error or warning, respectively if both
>>>>> declarations are further referenced by the code or not.
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97532
>>>>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>>>>> ---
>>>>>   src/compiler/glsl/linker.cpp | 14 ++++++++++----
>>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>>>>> index f352c5385ca..1c534ea1a3b 100644
>>>>> --- a/src/compiler/glsl/linker.cpp
>>>>> +++ b/src/compiler/glsl/linker.cpp
>>>>> @@ -1121,10 +1121,16 @@ cross_validate_globals(struct gl_shader_program *prog,
>>>>>            if (prog->IsES && (prog->data->Version != 310 ||
>>>>>
>>>>
>>>> Does just removing the "prog->data->Version != 310" clause also fix the
>>>> problem?
>>>
>>> Will check later today, but I suspect it would, since the applications
>>> are not GLES 3.1.
> 
> I just tested it now and it doesn't fix the problem. However, I
> misunderstood the code before. The shaders affected are not using
> uniform blocks, so "!var->get_interface_type()" hits and leads to the
> error.
> 
> Best regards,
> Tomasz
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 