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 <kenn...@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!
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
