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

[Tomasz Figa]

Hi Kenneth,

On Tue, Nov 7, 2017 at 8:18 AM, Kenneth Graunke  wrote:
> On Thursday, October 19, 2017 9:02:20 PM PST Tomasz Figa wrote:
>> Hi Ian, Kenneth,
>>
>> On Wed, Sep 27, 2017 at 2:57 AM, Tomasz Figa  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
>
> Sorry, this completely fell off my radar.  I've gone ahead and pushed
> your patch.  Dylan also sent out Piglit tests for this case today, which
> I've reviewed.

No worries, thanks a lot!

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Kenneth Graunke]

On Thursday, October 19, 2017 9:02:20 PM PST Tomasz Figa wrote:
> Hi Ian, Kenneth,
> 
> On Wed, Sep 27, 2017 at 2:57 AM, Tomasz Figa  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

Sorry, this completely fell off my radar.  I've gone ahead and pushed
your patch.  Dylan also sent out Piglit tests for this case today, which
I've reviewed.

--Ken


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  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 
> ---
>  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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Tomasz Figa]

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev