[Mesa-dev] [PATCH] glsl: Allow precision mismatch on dead uniform with GLSL ES 1.00 (v3)

[Tomasz Figa]

Hi Ian, Kenneth,

On Wed, Sep 27, 2017 at 2:57 AM, Tomasz Figa <tfiga at chromium.org> wrote:
> Commit 259fc505454ea6a67aeacf6cdebf1398d9947759 added linker error for
> mismatching uniform precision, as required by GLES 3.0 specification and
> conformance test-suite.
>
> Several Android applications, including Forge of Empires, have shaders
> which violate this rule, on uniforms that are declared but not used
> further in shader code. The problem affects a big number of Android
> games, including ones built on top of one of the common 2D graphics
> engines and other GLES implementations accept this, which poses a serious
> application compatibility issue.
>
> Starting from GLSL ES 3.0, declarations with conflicting precision
> qualifiers are explicitly prohibited. However GLSL ES 1.00 does not
> clearly specify the behavior, except that
>
>   "Uniforms are defined to behave as if they are using the same storage in
>   the vertex and fragment processors and may be implemented this way.
>   If uniforms are used in both the vertex and fragment shaders, developers
>   should be warned if the precisions are different. Conversion of
>   precision should never be implicit."
>
> The word "used" is not clear in this context and might refer to
>  1) declared (same as GLES 3.x)
>  2) referred after post-processing, or
>  3) linked after all optimizations are done.
>
> Looking at existing applications, 2) or 3) seems to be widely adopted.
> To avoid compatibility issues, turn the error into a warning if GLSL ES
> version is lower than 3.0 and the data is dead in at least one of the
> shaders.
>
> v3:
>  - Add a comment explaining the behavior.
>  - Fix bad copy/paste in commit message (s/varyings/uniforms).

Would you be able to take a look?

Ian, I believe your previous NAK was due to the confusing erroneous
copy/paste from the freedesktop bug I made in commit message. Looking
at last comment from Kenneth there, we were going to go with my patch,
but things remained quiet after that.

Thanks,
Tomasz

> v2:
>  - Change the behavior only for GLSL ES 1.00 shaders.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97532
> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
> ---
>  src/compiler/glsl/linker.cpp | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index f352c5385ca..693c5ae3664 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -1121,10 +1121,34 @@ cross_validate_globals(struct gl_shader_program *prog,
>           if (prog->IsES && (prog->data->Version != 310 ||
>                              !var->get_interface_type()) &&
>               existing->data.precision != var->data.precision) {
> -            linker_error(prog, "declarations for %s `%s` have "
> -                         "mismatching precision qualifiers\n",
> -                         mode_string(var), var->name);
> -            return;
> +            /*
> +             * GLSL ES 3.00 is the first version that explicitly requires
> +             * that uniform declarations have matching precision qualifiers.
> +             *
> +             * The only relevant part of GLSL ES 1.00 spec,
> +             *
> +             *  "If uniforms are used in both the vertex and fragment shaders,
> +             *   developers should be warned if the precisions are different.
> +             *   Conversion of precision should never be implicit."
> +             *
> +             * leaves too wide field for intepretation. However, judging by
> +             * applications and implementations existing in the wild, it seems
> +             * to be widely assumed that declarations alone are not enough to
> +             * fail the link.
> +             *
> +             * Thus, in case of GLSL ES < 3.00, trigger an error only if the
> +             * uniform is actually referenced in the code of both shaders.
> +             */
> +            if ((existing->data.used && var->data.used) || prog->data->Version >= 300) {
> +               linker_error(prog, "declarations for %s `%s` have "
> +                            "mismatching precision qualifiers\n",
> +                            mode_string(var), var->name);
> +               return;
> +            } else {
> +               linker_warning(prog, "declarations for %s `%s` have "
> +                              "mismatching precision qualifiers\n",
> +                              mode_string(var), var->name);
> +            }
>           }
>        } else
>           variables->add_variable(var);
> --
> 2.14.1.992.g2c7b836f3a-goog
>