[Mesa-dev] [RFC PATCH] mesa: Add MESA_SHADER_CAPTURE_PATH for writing .shader_test files.
Kenneth Graunke
kenneth at whitecape.org
Mon Jan 18 14:55:28 PST 2016
On Monday, January 18, 2016 8:53:18 AM PST Ben Widawsky wrote:
> 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 :-)
Yeah, and the function is tiny and obvious. I just have a habit of
writing comments above my functions. We could drop it. Either way.
> > +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.
That's what I thought too - but it doesn't work:
main/shaderapi.c: In function ‘_mesa_get_shader_capture_path’:
main/shaderapi.c:105:30: error: initializer element is not constant
static const char *path = getenv("MESA_SHADER_CAPTURE_PATH");
Apparently, initializers for static variables have to be data that can
be baked in, rather than function calls or code that has to be executed.
Which kind of makes sense - they probably wanted to support the simple
case of "initialize to 0" without having to define when that code would
be run (dlopen?).
I thought about doing
static const char *path = 0x1;
if (path == 0x1) {
path = getenv("MESA_SHADER_CAPTURE_PATH");
}
to avoid the extra static variable, but I decided the boolean was a
little cleaner. (Though, I think it makes the driver a bit bigger...)
> > +
> > + 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.
Laziness. This was more a hack I developed for my personal use.
I'd originally hardcoded it to /tmp/mesa, and /tmp/mesa/%u.shader_test
won't even exceed PATH_MAX.
I suppose we could guard against that error by adding a check to
_mesa_get_shader_capture_path():
if (strlen(path) > PATH_MAX - strlen("/4294967296.shader_test")) {
_mesa_warning("MESA_SHADER_CAPTURE_PATH too long; ignoring");
path = NULL;
}
I suppose adding a _mesa_warning if fopen fails would be appropriate.
> > + 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.
Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160118/26c69b37/attachment-0001.sig>
More information about the mesa-dev
mailing list