Re: [PATCH] wined3d: Properly handle immediate NaN and Inf in GLSL backend.
In my opinion, we do not want to solve the issue with immediate constant buffers this way. However, this patch might still be useful. Some games (Magicka 2) use shaders which contain NaN values in immediate constants. This patch represents a one way to solve the issue with NaN values in immediate constants. If we decide that we want to apply this change we should be more careful though. We probably want to use this for SM4+ shaders only, or use it only when GL_ARB_shader_bit_encoding is available. On a side note, "uintBitsToFloat" cannot be used as a constant expression (at least in GLSL 1.30), so to make this work with the immediate constant buffers patch you would have to drop "const". On Mon, Jun 13, 2016 at 9:30 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote:
Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> --- dlls/wined3d/glsl_shader.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index def224c..fde4c5e 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -34,6 +34,8 @@
#include <limits.h> #include <stdio.h> +#include <stdint.h> +#include <inttypes.h>
We avoid C99 in Wine. unsigned int and %#xu should be good enough.
#ifdef HAVE_FLOAT_H # include <float.h> #endif @@ -321,14 +323,26 @@ static const char *shader_glsl_get_version(const struct wined3d_gl_info *gl_info return "#version 120"; }
+/* Allow raw NaN and Inf in GLSL code. */ +void wined3d_glsl_ftoa(float value, char *s)
The function should be static.
+{ + if (!isfinite(value)) + { + /* We need a buffer of at least 28 characters. */ + snprintf(s, 28, "uintBitsToFloat(%"PRIu32"u)", *(uint32_t *)&value);
It seems that 29 bytes are needed at least (including the null character).
+ } + else + wined3d_ftoa(value, s); +}
In wined3d, generally, if one block in if-else statement needs curly braces we use curly braces for all blocks in this statement. else { wined3d_ftoa(value, s); }
+ static void shader_glsl_append_imm_vec4(struct wined3d_string_buffer *buffer, const float *values) { - char str[4][17]; + char str[4][28];
- wined3d_ftoa(values[0], str[0]); - wined3d_ftoa(values[1], str[1]); - wined3d_ftoa(values[2], str[2]); - wined3d_ftoa(values[3], str[3]); + wined3d_glsl_ftoa(values[0], str[0]); + wined3d_glsl_ftoa(values[1], str[1]); + wined3d_glsl_ftoa(values[2], str[2]); + wined3d_glsl_ftoa(values[3], str[3]); shader_addline(buffer, "vec4(%s, %s, %s, %s)", str[0], str[1], str[2], str[3]); }
@@ -2299,7 +2313,7 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * const struct wined3d_gl_info *gl_info = ins->ctx->gl_info; const char *prefix = shader_glsl_get_prefix(version->type); struct glsl_src_param rel_param0, rel_param1; - char imm_str[4][17]; + char imm_str[4][28];
if (reg->idx[0].offset != ~0U && reg->idx[0].rel_addr) shader_glsl_add_src_param(ins, reg->idx[0].rel_addr, WINED3DSP_WRITEMASK_0, &rel_param0); @@ -2513,7 +2527,7 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * switch (reg->data_type) { case WINED3D_DATA_FLOAT: - wined3d_ftoa(*(const float *)reg->immconst_data, register_name); + wined3d_glsl_ftoa(*(const float *)reg->immconst_data, register_name); break; case WINED3D_DATA_INT: sprintf(register_name, "%#x", reg->immconst_data[0]); @@ -2533,10 +2547,10 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * switch (reg->data_type) { case WINED3D_DATA_FLOAT: - wined3d_ftoa(*(const float *)®->immconst_data[0], imm_str[0]); - wined3d_ftoa(*(const float *)®->immconst_data[1], imm_str[1]); - wined3d_ftoa(*(const float *)®->immconst_data[2], imm_str[2]); - wined3d_ftoa(*(const float *)®->immconst_data[3], imm_str[3]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[0], imm_str[0]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[1], imm_str[1]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[2], imm_str[2]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[3], imm_str[3]); sprintf(register_name, "vec4(%s, %s, %s, %s)", imm_str[0], imm_str[1], imm_str[2], imm_str[3]);
It looks like shader_glsl_append_imm_vec4() could be called there instead.
Le 14/06/2016 10:09, Józef Kucia a écrit :
In my opinion, we do not want to solve the issue with immediate constant buffers this way. However, this patch might still be useful. Some games (Magicka 2) use shaders which contain NaN values in immediate constants. This patch represents a one way to solve the issue with NaN values in immediate constants. If we decide that we want to apply this change we should be more careful though. We probably want to use this for SM4+ shaders only, or use it only when GL_ARB_shader_bit_encoding is available. To be honest, I wrote this patch before the ICB one, because a game (The Witcher 3) makes use of immediate NaNs in it's shaders. I'm adding the GL_ARB_shader_bit_encoding checks. :) On a side note, "uintBitsToFloat" cannot be used as a constant expression (at least in GLSL 1.30), so to make this work with the immediate constant buffers patch you would have to drop "const". Mh, I see, it will be *way* easier to implement ICB as a uniform, no need to deal with NaN nor driver problems... On Mon, Jun 13, 2016 at 9:30 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote:
Signed-off-by: Guillaume Charifi<guillaume.charifi(a)sfr.fr> --- dlls/wined3d/glsl_shader.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index def224c..fde4c5e 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -34,6 +34,8 @@
#include <limits.h> #include <stdio.h> +#include <stdint.h> +#include <inttypes.h> We avoid C99 in Wine. unsigned int and %#xu should be good enough. Fixed. #ifdef HAVE_FLOAT_H # include <float.h> #endif @@ -321,14 +323,26 @@ static const char *shader_glsl_get_version(const struct wined3d_gl_info *gl_info return "#version 120"; }
+/* Allow raw NaN and Inf in GLSL code. */ +void wined3d_glsl_ftoa(float value, char *s) The function should be static. Woops, fixed. +{ + if (!isfinite(value)) + { + /* We need a buffer of at least 28 characters. */ + snprintf(s, 28, "uintBitsToFloat(%"PRIu32"u)", *(uint32_t *)&value); It seems that 29 bytes are needed at least (including the null character). Er... I think that sizeof("uintBitsToFloat(4294967295)") == 28... Anyway, I use %#x now, so it should be easier to read. :) + } + else + wined3d_ftoa(value, s); +} In wined3d, generally, if one block in if-else statement needs curly braces we use curly braces for all blocks in this statement. Well, no braces at all is funnier, so bye braces :) else { wined3d_ftoa(value, s); }
+ static void shader_glsl_append_imm_vec4(struct wined3d_string_buffer *buffer, const float *values) { - char str[4][17]; + char str[4][28];
- wined3d_ftoa(values[0], str[0]); - wined3d_ftoa(values[1], str[1]); - wined3d_ftoa(values[2], str[2]); - wined3d_ftoa(values[3], str[3]); + wined3d_glsl_ftoa(values[0], str[0]); + wined3d_glsl_ftoa(values[1], str[1]); + wined3d_glsl_ftoa(values[2], str[2]); + wined3d_glsl_ftoa(values[3], str[3]); shader_addline(buffer, "vec4(%s, %s, %s, %s)", str[0], str[1], str[2], str[3]); }
@@ -2299,7 +2313,7 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * const struct wined3d_gl_info *gl_info = ins->ctx->gl_info; const char *prefix = shader_glsl_get_prefix(version->type); struct glsl_src_param rel_param0, rel_param1; - char imm_str[4][17]; + char imm_str[4][28];
if (reg->idx[0].offset != ~0U && reg->idx[0].rel_addr) shader_glsl_add_src_param(ins, reg->idx[0].rel_addr, WINED3DSP_WRITEMASK_0, &rel_param0); @@ -2513,7 +2527,7 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * switch (reg->data_type) { case WINED3D_DATA_FLOAT: - wined3d_ftoa(*(const float *)reg->immconst_data, register_name); + wined3d_glsl_ftoa(*(const float *)reg->immconst_data, register_name); break; case WINED3D_DATA_INT: sprintf(register_name, "%#x", reg->immconst_data[0]); @@ -2533,10 +2547,10 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * switch (reg->data_type) { case WINED3D_DATA_FLOAT: - wined3d_ftoa(*(const float *)®->immconst_data[0], imm_str[0]); - wined3d_ftoa(*(const float *)®->immconst_data[1], imm_str[1]); - wined3d_ftoa(*(const float *)®->immconst_data[2], imm_str[2]); - wined3d_ftoa(*(const float *)®->immconst_data[3], imm_str[3]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[0], imm_str[0]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[1], imm_str[1]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[2], imm_str[2]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[3], imm_str[3]); sprintf(register_name, "vec4(%s, %s, %s, %s)", imm_str[0], imm_str[1], imm_str[2], imm_str[3]); It looks like shader_glsl_append_imm_vec4() could be called there instead. Mh, I can't do that, shader_glsl_appends_imm_vec4() pushes data directly to a struct wined3d_string_buffer *, whereas here I have only a char * to write on.
On Tue, Jun 14, 2016 at 11:32 AM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote:
It seems that 29 bytes are needed at least (including the null character).
Er... I think that sizeof("uintBitsToFloat(4294967295)") == 28... Anyway, I use %#x now, so it should be easier to read. :)
I think you forgot about the 'u' suffix.
Le 14/06/2016 11:48, Józef Kucia a écrit :
On Tue, Jun 14, 2016 at 11:32 AM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote:
It seems that 29 bytes are needed at least (including the null character). Er... I think that sizeof("uintBitsToFloat(4294967295)") == 28... Anyway, I use %#x now, so it should be easier to read. :) I think you forgot about the 'u' suffix. Yes, exactly, thank you! :D
Do you handle negative NaN and negative Infinity? Ced On 14 June 2016 at 10:09, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
In my opinion, we do not want to solve the issue with immediate constant buffers this way. However, this patch might still be useful. Some games (Magicka 2) use shaders which contain NaN values in immediate constants. This patch represents a one way to solve the issue with NaN values in immediate constants. If we decide that we want to apply this change we should be more careful though. We probably want to use this for SM4+ shaders only, or use it only when GL_ARB_shader_bit_encoding is available.
On a side note, "uintBitsToFloat" cannot be used as a constant expression (at least in GLSL 1.30), so to make this work with the immediate constant buffers patch you would have to drop "const".
On Mon, Jun 13, 2016 at 9:30 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote:
Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> --- dlls/wined3d/glsl_shader.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index def224c..fde4c5e 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -34,6 +34,8 @@
#include <limits.h> #include <stdio.h> +#include <stdint.h> +#include <inttypes.h>
We avoid C99 in Wine. unsigned int and %#xu should be good enough.
#ifdef HAVE_FLOAT_H # include <float.h> #endif @@ -321,14 +323,26 @@ static const char *shader_glsl_get_version(const struct wined3d_gl_info *gl_info return "#version 120"; }
+/* Allow raw NaN and Inf in GLSL code. */ +void wined3d_glsl_ftoa(float value, char *s)
The function should be static.
+{ + if (!isfinite(value)) + { + /* We need a buffer of at least 28 characters. */ + snprintf(s, 28, "uintBitsToFloat(%"PRIu32"u)", *(uint32_t *)&value);
It seems that 29 bytes are needed at least (including the null character).
+ } + else + wined3d_ftoa(value, s); +}
In wined3d, generally, if one block in if-else statement needs curly braces we use curly braces for all blocks in this statement.
else { wined3d_ftoa(value, s); }
+ static void shader_glsl_append_imm_vec4(struct wined3d_string_buffer *buffer, const float *values) { - char str[4][17]; + char str[4][28];
- wined3d_ftoa(values[0], str[0]); - wined3d_ftoa(values[1], str[1]); - wined3d_ftoa(values[2], str[2]); - wined3d_ftoa(values[3], str[3]); + wined3d_glsl_ftoa(values[0], str[0]); + wined3d_glsl_ftoa(values[1], str[1]); + wined3d_glsl_ftoa(values[2], str[2]); + wined3d_glsl_ftoa(values[3], str[3]); shader_addline(buffer, "vec4(%s, %s, %s, %s)", str[0], str[1], str[2], str[3]); }
@@ -2299,7 +2313,7 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * const struct wined3d_gl_info *gl_info = ins->ctx->gl_info; const char *prefix = shader_glsl_get_prefix(version->type); struct glsl_src_param rel_param0, rel_param1; - char imm_str[4][17]; + char imm_str[4][28];
if (reg->idx[0].offset != ~0U && reg->idx[0].rel_addr) shader_glsl_add_src_param(ins, reg->idx[0].rel_addr, WINED3DSP_WRITEMASK_0, &rel_param0); @@ -2513,7 +2527,7 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * switch (reg->data_type) { case WINED3D_DATA_FLOAT: - wined3d_ftoa(*(const float *)reg->immconst_data, register_name); + wined3d_glsl_ftoa(*(const float *)reg->immconst_data, register_name); break; case WINED3D_DATA_INT: sprintf(register_name, "%#x", reg->immconst_data[0]); @@ -2533,10 +2547,10 @@ static void shader_glsl_get_register_name(const struct wined3d_shader_register * switch (reg->data_type) { case WINED3D_DATA_FLOAT: - wined3d_ftoa(*(const float *)®->immconst_data[0], imm_str[0]); - wined3d_ftoa(*(const float *)®->immconst_data[1], imm_str[1]); - wined3d_ftoa(*(const float *)®->immconst_data[2], imm_str[2]); - wined3d_ftoa(*(const float *)®->immconst_data[3], imm_str[3]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[0], imm_str[0]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[1], imm_str[1]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[2], imm_str[2]); + wined3d_glsl_ftoa(*(const float *)®->immconst_data[3], imm_str[3]); sprintf(register_name, "vec4(%s, %s, %s, %s)", imm_str[0], imm_str[1], imm_str[2], imm_str[3]);
It looks like shader_glsl_append_imm_vec4() could be called there instead.
-- Cedric Blancher <cedric.blancher(a)gmail.com> Institute Pasteur
Yes, of course. To better understand, here is a (more or less correct) equivalent between GLSL and C: In GLSL: uint a = 0xFFFFFFFFu; float b = uintBitsToFloat(a); In C: unsigned int a = 0xFFFFFFFFu; float b = *(float *)&a; So you can see that the sign of the integer doesn't influence the float (and it is easier to use an uint than an int, because no extra - sign to deal with). Le 14/06/2016 16:35, Cedric Blancher a écrit :
Do you handle negative NaN and negative Infinity?
Ced
On 14 June 2016 at 10:09, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
In my opinion, we do not want to solve the issue with immediate constant buffers this way. However, this patch might still be useful. Some games (Magicka 2) use shaders which contain NaN values in immediate constants. This patch represents a one way to solve the issue with NaN values in immediate constants. If we decide that we want to apply this change we should be more careful though. We probably want to use this for SM4+ shaders only, or use it only when GL_ARB_shader_bit_encoding is available.
Note that SM3 and earlier shaders have a similar issue. (In particular, see "lconst_inf_or_nan" and "load_local_constsF" in struct wined3d_shader.) It seems tempting to always use unsigned integers for immediate values if we have ARB_shader_bit_encoding.
On Wed, Jun 15, 2016 at 1:44 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
Note that SM3 and earlier shaders have a similar issue. (In particular, see "lconst_inf_or_nan" and "load_local_constsF" in struct wined3d_shader.) It seems tempting to always use unsigned integers for immediate values if we have ARB_shader_bit_encoding.
Yes, it's tempting and it might lead to nicer code, i.e. wined3d_glsl_ftoa() should not be needed.
participants (4)
-
Cedric Blancher -
Guillaume Charifi -
Henri Verbeet -
Józef Kucia