[PATCH] wined3d: Properly handle immediate NaN and Inf in GLSL backend.

[Guillaume Charifi]

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 at sfr.fr>  wrote:
>> Signed-off-by: Guillaume Charifi<guillaume.charifi at 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 *)&reg->immconst_data[0], imm_str[0]);
>> -                            wined3d_ftoa(*(const float *)&reg->immconst_data[1], imm_str[1]);
>> -                            wined3d_ftoa(*(const float *)&reg->immconst_data[2], imm_str[2]);
>> -                            wined3d_ftoa(*(const float *)&reg->immconst_data[3], imm_str[3]);
>> +                            wined3d_glsl_ftoa(*(const float *)&reg->immconst_data[0], imm_str[0]);
>> +                            wined3d_glsl_ftoa(*(const float *)&reg->immconst_data[1], imm_str[1]);
>> +                            wined3d_glsl_ftoa(*(const float *)&reg->immconst_data[2], imm_str[2]);
>> +                            wined3d_glsl_ftoa(*(const float *)&reg->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.