Re: [Mesa-dev] [PATCH 2/3] mesa/main/shaderapi: Use sha as part of captured shaders name

2018-05-17 Thread Tapani Pälli 



On 05/17/2018 11:49 AM, Benedikt Schemmer wrote:



Am 17.05.2018 um 09:11 schrieb Tapani Pälli:

some nitpicking below .. I'll do some testing to see that things work as before 
but overall these changes look good to me. Would be nice to hear comments from 
active shader-db users.

On 05/10/2018 12:05 PM, Benedikt Schemmer wrote:

It is inconvenient to capture shaders by program name alone because this does
not allow to capture shaders that get overwritten by shaders with the same
program name (ie games when you change settings or piglit).

---
   src/mesa/main/shaderapi.c | 47 
---
   1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 1d0ca5374b..65cf0a67a2 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -1343,29 +1343,46 @@ link_program(struct gl_context *ctx, struct 
gl_shader_program *shProg,
  /* Capture .shader_test files. */
  const char *capture_path = _mesa_get_shader_capture_path();
  if (shProg->Name != 0 && shProg->Name != ~0 && capture_path != NULL) {
+


extra space


     FILE *file;
-  char *filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
-   capture_path, shProg->Name);
+  char *filename = NULL;
+  char *fsource = NULL;
+  char *ftemp = NULL;
+
+  asprintf(, "[require]\nGLSL%s >= %u.%02u\n",
+   shProg->IsES ? " ES" : "",
+   shProg->data->Version / 100, shProg->data->Version % 100);
+
+  if (shProg->SeparateShader) {
+ ftemp = fsource;
+ asprintf(, "%sGL_ARB_separate_shader_objects\nSSO ENABLED\n",
+  ftemp);
+ free(ftemp);
+  }
+
+  for (unsigned i = 0; i < shProg->NumShaders; i++) {
+  ftemp = fsource;
+  asprintf(, "%s\n[%s shader]\n%s\n", ftemp,
+   _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
+   shProg->Shaders[i]->Source);
+  free(ftemp);
+  }
+
+  char shabuf[64] = {"mylittlebunny"};


please remove the initialization, it is unnecessary


That was left from before I used generate_sha1 which sometimes failed for some 
reason.
I have one little thing though:
Most of the time this is defined as shabuf[41] (20 bytes sha plus trailing \0 )
In this file it is 64 for some reason.
Should I change that?


Yes please, 41 is enough space there.




+  generate_sha1(fsource, shabuf);
+
+  asprintf(, "%s/%s_%u.shader_test", capture_path,
+   shabuf, shProg->Name);
     file = fopen(filename, "w");
     if (file) {
- fprintf(file, "[require]\nGLSL%s >= %u.%02u\n",
- shProg->IsES ? " ES" : "",
- shProg->data->Version / 100, shProg->data->Version % 100);
- if (shProg->SeparateShader)
-    fprintf(file, "GL_ARB_separate_shader_objects\nSSO ENABLED\n");
- fprintf(file, "\n");
-
- for (unsigned i = 0; i < shProg->NumShaders; i++) {
-    fprintf(file, "[%s shader]\n%s\n",
-    _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
-    shProg->Shaders[i]->Source);
- }
+ fprintf(file, "%s", fsource);
    fclose(file);
     } else {
    _mesa_warning(ctx, "Failed to open %s", filename);
     }

-  ralloc_free(filename);
+  free(filename);
+  free(fsource);
  }

  if (shProg->data->LinkStatus == LINKING_FAILURE &&


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

Re: [Mesa-dev] [PATCH 2/3] mesa/main/shaderapi: Use sha as part of captured shaders name

2018-05-17 Thread Benedikt Schemmer 


Am 17.05.2018 um 09:11 schrieb Tapani Pälli:
> some nitpicking below .. I'll do some testing to see that things work as 
> before but overall these changes look good to me. Would be nice to hear 
> comments from active shader-db users.
> 
> On 05/10/2018 12:05 PM, Benedikt Schemmer wrote:
>> It is inconvenient to capture shaders by program name alone because this does
>> not allow to capture shaders that get overwritten by shaders with the same
>> program name (ie games when you change settings or piglit).
>>
>> ---
>>   src/mesa/main/shaderapi.c | 47 
>> ---
>>   1 file changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>> index 1d0ca5374b..65cf0a67a2 100644
>> --- a/src/mesa/main/shaderapi.c
>> +++ b/src/mesa/main/shaderapi.c
>> @@ -1343,29 +1343,46 @@ link_program(struct gl_context *ctx, struct 
>> gl_shader_program *shProg,
>>  /* Capture .shader_test files. */
>>  const char *capture_path = _mesa_get_shader_capture_path();
>>  if (shProg->Name != 0 && shProg->Name != ~0 && capture_path != NULL) {
>> +
> 
> extra space
> 
>>     FILE *file;
>> -  char *filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
>> -   capture_path, shProg->Name);
>> +  char *filename = NULL;
>> +  char *fsource = NULL;
>> +  char *ftemp = NULL;
>> +
>> +  asprintf(, "[require]\nGLSL%s >= %u.%02u\n",
>> +   shProg->IsES ? " ES" : "",
>> +   shProg->data->Version / 100, shProg->data->Version % 100);
>> +
>> +  if (shProg->SeparateShader) {
>> + ftemp = fsource;
>> + asprintf(, "%sGL_ARB_separate_shader_objects\nSSO 
>> ENABLED\n",
>> +  ftemp);
>> + free(ftemp);
>> +  }
>> +
>> +  for (unsigned i = 0; i < shProg->NumShaders; i++) {
>> +  ftemp = fsource;
>> +  asprintf(, "%s\n[%s shader]\n%s\n", ftemp,
>> +   _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
>> +   shProg->Shaders[i]->Source);
>> +  free(ftemp);
>> +  }
>> +
>> +  char shabuf[64] = {"mylittlebunny"};
> 
> please remove the initialization, it is unnecessary

That was left from before I used generate_sha1 which sometimes failed for some 
reason.
I have one little thing though:
Most of the time this is defined as shabuf[41] (20 bytes sha plus trailing \0 )
In this file it is 64 for some reason.
Should I change that?

> 
>> +  generate_sha1(fsource, shabuf);
>> +
>> +  asprintf(, "%s/%s_%u.shader_test", capture_path,
>> +   shabuf, shProg->Name);
>>     file = fopen(filename, "w");
>>     if (file) {
>> - fprintf(file, "[require]\nGLSL%s >= %u.%02u\n",
>> - shProg->IsES ? " ES" : "",
>> - shProg->data->Version / 100, shProg->data->Version % 100);
>> - if (shProg->SeparateShader)
>> -    fprintf(file, "GL_ARB_separate_shader_objects\nSSO ENABLED\n");
>> - fprintf(file, "\n");
>> -
>> - for (unsigned i = 0; i < shProg->NumShaders; i++) {
>> -    fprintf(file, "[%s shader]\n%s\n",
>> -    _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
>> -    shProg->Shaders[i]->Source);
>> - }
>> + fprintf(file, "%s", fsource);
>>    fclose(file);
>>     } else {
>>    _mesa_warning(ctx, "Failed to open %s", filename);
>>     }
>>
>> -  ralloc_free(filename);
>> +  free(filename);
>> +  free(fsource);
>>  }
>>
>>  if (shProg->data->LinkStatus == LINKING_FAILURE &&
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] mesa/main/shaderapi: Use sha as part of captured shaders name

2018-05-17 Thread Tapani Pälli 
some nitpicking below .. I'll do some testing to see that things work as 
before but overall these changes look good to me. Would be nice to hear 
comments from active shader-db users.


On 05/10/2018 12:05 PM, Benedikt Schemmer wrote:

It is inconvenient to capture shaders by program name alone because this does
not allow to capture shaders that get overwritten by shaders with the same
program name (ie games when you change settings or piglit).

---
  src/mesa/main/shaderapi.c | 47 ---
  1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 1d0ca5374b..65cf0a67a2 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -1343,29 +1343,46 @@ link_program(struct gl_context *ctx, struct 
gl_shader_program *shProg,
 /* Capture .shader_test files. */
 const char *capture_path = _mesa_get_shader_capture_path();
 if (shProg->Name != 0 && shProg->Name != ~0 && capture_path != NULL) {
+


extra space


FILE *file;
-  char *filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
-   capture_path, shProg->Name);
+  char *filename = NULL;
+  char *fsource = NULL;
+  char *ftemp = NULL;
+
+  asprintf(, "[require]\nGLSL%s >= %u.%02u\n",
+   shProg->IsES ? " ES" : "",
+   shProg->data->Version / 100, shProg->data->Version % 100);
+
+  if (shProg->SeparateShader) {
+ ftemp = fsource;
+ asprintf(, "%sGL_ARB_separate_shader_objects\nSSO ENABLED\n",
+  ftemp);
+ free(ftemp);
+  }
+
+  for (unsigned i = 0; i < shProg->NumShaders; i++) {
+  ftemp = fsource;
+  asprintf(, "%s\n[%s shader]\n%s\n", ftemp,
+   _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
+   shProg->Shaders[i]->Source);
+  free(ftemp);
+  }
+
+  char shabuf[64] = {"mylittlebunny"};


please remove the initialization, it is unnecessary


+  generate_sha1(fsource, shabuf);
+
+  asprintf(, "%s/%s_%u.shader_test", capture_path,
+   shabuf, shProg->Name);
file = fopen(filename, "w");
if (file) {
- fprintf(file, "[require]\nGLSL%s >= %u.%02u\n",
- shProg->IsES ? " ES" : "",
- shProg->data->Version / 100, shProg->data->Version % 100);
- if (shProg->SeparateShader)
-fprintf(file, "GL_ARB_separate_shader_objects\nSSO ENABLED\n");
- fprintf(file, "\n");
-
- for (unsigned i = 0; i < shProg->NumShaders; i++) {
-fprintf(file, "[%s shader]\n%s\n",
-_mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
-shProg->Shaders[i]->Source);
- }
+ fprintf(file, "%s", fsource);
   fclose(file);
} else {
   _mesa_warning(ctx, "Failed to open %s", filename);
}

-  ralloc_free(filename);
+  free(filename);
+  free(fsource);
 }

 if (shProg->data->LinkStatus == LINKING_FAILURE &&


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