[Mesa-dev] [RFC PATCH] mesa: Add MESA_SHADER_CAPTURE_PATH for writing .shader_test files.

[Ben Widawsky]

On Mon, Jan 18, 2016 at 07:05:21AM -0800, Kenneth Graunke wrote:
> This writes linked shader programs to .shader_test files to
> $MESA_SHADER_CAPTURE_PATH in the format used by shader-db
> (http://cgit.freedesktop.org/mesa/shader-db).
> 
> It supports both GLSL shaders and ARB programs.  All stages that
> are linked together are written in a single .shader_test file.
> 
> This eliminates the need for shader-db's split-to-files.py, as Mesa
> produces the desired format directly.  It's much more reliable than
> parsing stdout/stderr, as those may contain extraneous messages, or
> simply be closed by the application and unavailable.

As someone with no authority on the matter, using the source to generate the
.shader_test files is always better than some post processing thing. One might
argue that generating .shader_test is a little too specific, but I guess we can
cross that bridge when we get there.

So yeah, thumbs up from me. Small things below, while I'm here.

> 
> We have many similar features already, but this is a bit different:
> - MESA_GLSL=dump writes to stdout, not files.
> - MESA_GLSL=log writes each stage to separate files (rather than
>   all linked shaders in one file), at draw time (not link time),
>   with uniform data and state flag info.
> - Tapani's shader replacement mechanism (MESA_SHADER_DUMP_PATH and
>   MESA_SHADER_READ_PATH) also uses separate files per shader stage,
>   but allows reading in files to replace an app's shader code.
> 
> v2: Dump ARB programs too, not just GLSL.
> v3: Don't dump bogus 0.shader_test file.
> v4: Add "GL_ARB_separate_shader_objects" to the [require] block.
> v5: Print "GLSL 4.00" instead of "GLSL 4.0" in the [require] block.
> v6: Don't hardcode /tmp/mesa.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/main/arbprogram.c | 20 ++++++++++++++++++++
>  src/mesa/main/mtypes.h     |  1 -
>  src/mesa/main/shaderapi.c  | 40 ++++++++++++++++++++++++++++++++++++++++
>  src/mesa/main/shaderapi.h  |  3 +++
>  4 files changed, 63 insertions(+), 1 deletion(-)
> 
> Hi Felix,
> 
> Nothing nearly as cool as apitrace :)  It's just a simple patch to write
> .shader_test files for any shader programs an application asks us to compile.
> My workflow is basically:
> 
>    $ mkdir /tmp/mesa
>    $ MESA_SHADER_CAPTURE_PATH=/tmp/mesa <some application>
>      (this writes a /tmp/mesa
>    $ cd ~/Projects/shader-db/shaders/...
>    $ find-duplicates /tmp/mesa/* | grep ^rm | sh
>    $ mv /tmp/mesa/* .
>    $ git add .
>    $ git commit -m 'Import shaders for ...'
> 
> where find-duplicates is http://whitecape.org/stuff/find-duplicates
> To do this with Steam, you can right click on the game in the library,
> click "Properties", then "Set Launch Options", and enter:
> 
>    MESA_SHADER_CAPTURE_PATH=/tmp/mesa %command%
> 
>  --Ken
> 
> diff --git a/src/mesa/main/arbprogram.c b/src/mesa/main/arbprogram.c
> index f474951..f78598c 100644
> --- a/src/mesa/main/arbprogram.c
> +++ b/src/mesa/main/arbprogram.c
> @@ -36,6 +36,7 @@
>  #include "main/macros.h"
>  #include "main/mtypes.h"
>  #include "main/arbprogram.h"
> +#include "main/shaderapi.h"
>  #include "program/arbprogparse.h"
>  #include "program/program.h"
>  #include "program/prog_print.h"
> @@ -373,6 +374,25 @@ _mesa_ProgramStringARB(GLenum target, GLenum format, GLsizei len,
>        }
>        fflush(stderr);
>     }
> +
> +   /* Capture vp-*.shader_test/fp-*.shader_test files. */
> +   const char *capture_path = _mesa_get_shader_capture_path();
> +   if (capture_path != NULL) {
> +      FILE *file;
> +      char filename[PATH_MAX];
> +      const char *shader_type =
> +         target == GL_FRAGMENT_PROGRAM_ARB ? "fragment" : "vertex";
> +
> +      _mesa_snprintf(filename, sizeof(filename), "%s/%cp-%u.shader_test",
> +                     capture_path, shader_type[0], base->Id);
> +      file = fopen(filename, "w");
> +      if (file) {
> +         fprintf(file,
> +                 "[require]\nGL_ARB_%s_program\n\n[%s program]\n%s\n",
> +                 shader_type, shader_type, (const char *) string);
> +         fclose(file);
> +      }
> +   }
>  }
>  
>  
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 0992d4d..6d35e41 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2856,7 +2856,6 @@ struct gl_shader_program
>  #define GLSL_REPORT_ERRORS 0x100  /**< Print compilation errors */
>  #define GLSL_DUMP_ON_ERROR 0x200 /**< Dump shaders to stderr on compile error */
>  
> -
>  /**
>   * Context state for GLSL vertex/fragment shaders.
>   * Extended to support pipeline object
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 126786c..030d9a1 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -96,6 +96,20 @@ _mesa_get_shader_flags(void)
>     return flags;
>  }
>  
> +/**
> + * Memoized version of getenv("MESA_SHADER_CAPTURE_PATH").
> + */

The comment is probably unnecessary as this is such a common thing to do. I only
pointed it out because I now learned the work memoization :-)

> +const char *
> +_mesa_get_shader_capture_path(void)
> +{
> +   static bool read_env_var = false;
> +   static const char *path = NULL;
> +
> +   if (!read_env_var)
> +      path = getenv("MESA_SHADER_CAPTURE_PATH");

I just noticed you spotted the problem here in a followup mail - but why not get
rid of the read_env_var completely? Either way you're subject to failures while
people are running a program and changing that variable (though admittedly, the
bool version is kind of less failure-y). Doesn't it just become:

static const char *path = getenv("MESA_SHADER_CAPTURE_PATH");
return path;

Perhaps I'm failing to see something obvious that you've thought of though, it's
early and I haven't had coffee yet.

> +
> +   return path;
> +}
>  
>  /**
>   * Initialize context's shader state.
> @@ -1047,6 +1061,32 @@ link_program(struct gl_context *ctx, GLuint program)
>  
>     _mesa_glsl_link_shader(ctx, shProg);
>  
> +   /* Capture .shader_test files. */
> +   const char *capture_path = _mesa_get_shader_capture_path();
> +   if (shProg->Name != 0 && capture_path != NULL) {
> +      FILE *file;
> +      char filename[PATH_MAX];
> +
> +      _mesa_snprintf(filename, sizeof(filename), "%s/%u.shader_test",
> +                     capture_path, shProg->Name);
> +

Was error handling left out intentionally? If you really do go past PATH_MAX,
fopen will fail for non-obvious reasons - although I notice that you have no
error messages at all (like if the file isn't writable), so perhaps that's the
intention.

> +      file = fopen(filename, "w");
> +      if (file) {
> +         fprintf(file, "[require]\nGLSL >= %u.%02u\n",
> +                 shProg->Version / 100, shProg->Version % 100);
> +         if (shProg->SeparateShader)
> +            fprintf(file, "GL_ARB_separate_shader_objects\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);
> +         }
> +         fclose(file);
> +      }
> +   }
> +
>     if (shProg->LinkStatus == GL_FALSE &&
>         (ctx->_Shader->Flags & GLSL_REPORT_ERRORS)) {
>        _mesa_debug(ctx, "Error linking program %u:\n%s\n",
> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
> index fba767b..2ac25d0 100644
> --- a/src/mesa/main/shaderapi.h
> +++ b/src/mesa/main/shaderapi.h
> @@ -43,6 +43,9 @@ struct gl_shader_program;
>  extern GLbitfield
>  _mesa_get_shader_flags(void);
>  
> +extern const char *
> +_mesa_get_shader_capture_path(void);
> +
>  extern void
>  _mesa_copy_string(GLchar *dst, GLsizei maxLength,
>                    GLsizei *length, const GLchar *src);

Bottom line is I think this is a good thing to do, for whatever that's worth.