-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Ben Holmes wrote:
One general comment first... don't fear whitespace.
This:
if(!strncmp(&(testTexts[currentTest][i]),"expected",8))
is much harder on the eyes than:
if (!strncmp(&(testTexts[currentTest][i]), "expected", 8))
Also, our vague coding standard wants the opening curly-brace on the
same line as the if-statement, while-statement, etc.
I also think you should wrap the text handling in a couple accessor
functions. Something like:
char *get_string(char *input, char **value, int *length);
char *get_float(char *input, float *value);
Each of these would eat whitespace before (and after?) the token, and
each would return the position in the text after the token.
> ---
> tests/all.tests | 6 +
> tests/shaders/CMakeLists.txt | 1 +
> tests/shaders/glsl-shader-loader.c | 456
> ++++++++++++++++++++++++++++++++++++
> 3 files changed, 463 insertions(+), 0 deletions(-)
> create mode 100644 tests/shaders/glsl-shader-loader.c
>
> diff --git a/tests/all.tests b/tests/all.tests
> index ade7048..b05dec9 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -184,6 +184,12 @@ add_vpfpgeneric('vp-sge-alias')
> add_vpfpgeneric('vp-two-constants')
> shaders['vpfp-generic'] = vpfpgeneric
>
> +glslshaderloader = Group()
> +def add_glslshaderloader(name):
> + glslshaderloader[name] = PlainExecTest(['glsl-shader-loader', '-auto',
> testBinDir + '../tests/shaders/glsl-tests/' + name + '.ini'])
> +
> +shaders['glsl-shader-loader'] = glslshaderloader
> +
> bugs = Group()
> add_plain_test(bugs, 'crash-cubemap-order')
> add_plain_test(bugs, 'crash-texparameter-before-teximage')
> diff --git a/tests/shaders/CMakeLists.txt b/tests/shaders/CMakeLists.txt
> index 999ba4f..a60bec5 100644
> --- a/tests/shaders/CMakeLists.txt
> +++ b/tests/shaders/CMakeLists.txt
> @@ -73,3 +73,4 @@ add_executable (glsl-preprocessor-comments
> glsl-preprocessor-comments.c)
> add_executable (vp-ignore-input vp-ignore-input.c)
> add_executable (glsl-empty-vs-no-fs glsl-empty-vs-no-fs.c)
> add_executable (glsl-useprogram-displaylist glsl-useprogram-displaylist.c)
> +add_executable (glsl-shader-loader glsl-shader-loader.c)
> diff --git a/tests/shaders/glsl-shader-loader.c
> b/tests/shaders/glsl-shader-loader.c
> new file mode 100644
> index 0000000..fab09cf
> --- /dev/null
> +++ b/tests/shaders/glsl-shader-loader.c
> @@ -0,0 +1,456 @@
> +/*
> + * Copyright © 2009 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Ben Holmes <[email protected]>
> + */
> +
> +/*
> + * Draws quads using a program built from shaders/uniforms
> + * loaded from .ini specified like so: ./glsl-shader-driver ./tests/test.ini
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <GL/glew.h>
> +#if defined(__APPLE__)
> +#include <GLUT/glut.h>
> +#else
> +#include <GL/glut.h>
> +#endif
> +#include "piglit-util.h"
> +
> +int piglit_width = 600, piglit_height = 400;
> +int piglit_window_mode = GLUT_RGB | GLUT_DOUBLE;
> +static GLuint tex;
> +static GLint prog;
> +static GLint vs;
> +static GLint fs;
> +static char filename[256];
> +static char *buffer;
> +static char *vsText;
> +static GLint vsLength = 0;
> +static char *fsText;
> +static GLint fsLength = 0;
> +static char * testTexts[1024];
> +static int *testLengths;
> +static char *reqsText;
> +static int reqsLength = 0;
> +static int numTests = 0;
> +static int width = 2; //width of each quad, must be at least 2
> +static height = 2; //height of each quad, must be at least 2
> +
> +struct expected
> +{
> + GLfloat color[3];
> + float pos[2];
> +};
> +
> +static struct expected probe;
> +
> +void
> +test_parse(int currentTest)
> +{
> + int i,j;
> + float values[4];
> + char name[256];
> + char *cP;
> + GLint uLoc;
> +
Since currentTest is constant through this whole function, I strongly
recommend:
char *text = testTexts[currentTest];
That will make a lot of the code in the function much more readable.
> + for(i=0;i < testLengths[currentTest];++i)
> + {
> + if(!strncmp(&(testTexts[currentTest][i]),"expected",8))
> + {
> + i+=8;
> + cP = strchr(&(testTexts[currentTest][i]),' ');
> + for(j=0;j<3;++j)
> + {
> + sscanf(cP+1,"%f",&(probe.color[j]));
> + cP = strchr(cP+1,' ');
> + }
> + sscanf(cP+1,"%f",&(probe.pos[0]));
> + cP = strchr(cP+1,' ');
> + sscanf(cP+1,"%f",&(probe.pos[1]));
> + }
> + if(!strncmp(&(testTexts[currentTest][i]),"uniform",7))
> + {
> + i+=8;
> + if(!strncmp(&(testTexts[currentTest][i]),"vec4",4))
> + {
> + i+=5;
> + cP = strchr(&(testTexts[currentTest][i]),' ');
> +
This is not a very good way to parse this. This will accept, for
example, 'vec4xfoo '. I think you only want to advance the position by
4 (length of "vec4") and do something like:
while (isspace(testTexts[currentTest][i]))
i++;
name_ptr = & testTexts[currentTest][i];
while (isspace(testTexts[currentTest][i]))
i++;
Alternately, the get_string funtion that I suggested above would take
care of this.
text_position = get_string(text_position, & token,
& token_length);
if ((token_length == 4)
&& (strncmp(token, "vec4", 4) == 0)) {
text_position = get_string(text_position,
& name, & name_len);
for (j = 0; j < 4; j++) {
text_position = get_float(text_position,
& values[j]);
}
...
}
> + strncpy(name,&(testTexts[currentTest][i]),
> + (int)(cP - &(testTexts[currentTest][i])));
> +
> + name[(int)(cP - &(testTexts[currentTest][i]))]
> + = '\0';
> +
> + i+=(int)(cP - &(testTexts[currentTest][i]));
> +
> + for(j=0;j<4;++j)
> + {
> + sscanf(cP+1,"%f",
> + &(values[j]));
> + cP = strchr(cP+1,' ');
> + }
> + uLoc = glGetUniformLocation(prog, name);
> + glUniform4f(uLoc, values[0],values[1],
> + values[2],values[3]);
This should report an error (and fail the test) if the uniform does not
exist. glGetUniformLocation will return -1. Also, use
"glUniform4fv(uLoc, values)" here.
> + }
> + if(!strncmp(&(testTexts[currentTest][i]),"float",5))
> + {
> + i+=6;
> + cP = strchr(&(testTexts[currentTest][i]),' ');
> +
> + strncpy(name,&(testTexts[currentTest][i]),
> + (int)(cP - &(testTexts[currentTest][i])));
> +
> + name[(int)(cP - &(testTexts[currentTest][i]))]
> + = '\0';
> +
> + i+=(int)(cP - &(testTexts[currentTest][i]));
> +
> + sscanf(cP+1,"%f",&(values[0]));
> + uLoc = glGetUniformLocation(prog, name);
> + glUniform1i(uLoc, values[0]);
^ f
Same comments about error reporting as above.
> + }
> + }
> + }
> +}
> +
> +void
> +req_parse()
> +{
> + int i;
> + float reqVersion=2.0, version=0.0;
> + char requirement[256];
> + char *cP;
> +
> + for(i=0;i<reqsLength;++i)
> + {
> + if((int) (&(reqsText[strlen(reqsText)]) - &(reqsText[i]))
> + < 9)
> + break;
> + if(!strncmp(&(reqsText[i]),"extension",9))
> + {
> + i+=10;
> + cP = strchr(&(reqsText[i]),'\n');
> +
> + strncpy(requirement,&(reqsText[i]),
> + (int)(cP - &(reqsText[i])));
> + requirement[(int)(cP - &(reqsText[i]))] = '\0';
> + i+=(int)(cP - &(reqsText[i]));
> +
> + piglit_require_extension(requirement);
> + }
> + if(!strncmp(&(reqsText[i]),"version",7))
> + {
> + i+=8;
> +
> + sscanf(&(reqsText[i]),"%f",&reqVersion);
> +
> + strncpy(requirement,glGetString(GL_VERSION),3);
> + sscanf(requirement,"%f",&version);
The usual way to do this is:
version = strtod(glGetString(GL_VERSION), NULL);
> + if(version<reqVersion)
> + {
> + printf("Requires OpenGL version %f\n",
> + reqVersion);
> + piglit_report_result(PIGLIT_SKIP);
> + }
> + }
Add a case for glsl_version also. Query the supported version with
glGetString(GL_SHADING_LANGUAGE_VERSION).
> + }
> +}
> +
> +void
> +testFile_parse()
> +{
> + FILE* filePointer;
> + int i=0, secLength=0, fileLength=0, state=0, currentTest=0;
> + char c;
> + char word[32];
> + char *cP;
> +
> + filePointer = fopen(filename, "rt");
> + if(!filePointer)
> + piglit_report_result(PIGLIT_FAILURE);
> +
> +
> + while(fgetc(filePointer)!=EOF)
> + fileLength++;
> +
> + if(fileLength <1)
> + piglit_report_result(PIGLIT_FAILURE);
> +
> + fclose(filePointer);
> +
> + filePointer = fopen(filename, "rt");
> + buffer = (char*) malloc(fileLength+1);
> +
> + c = fgetc(filePointer);
> + while(c != EOF)
> + {
> + buffer[i] = c;
> + ++i;
> + c = fgetc(filePointer);
> + }
> +
> + buffer[i] = '\0';
> + fclose(filePointer);
The code above made my eyes bleed.
fp = fopen(filename, "r");
if (fp == NULL)
/* error */ ;
fseek(fp, 0, SEEK_END);
fileLength = ftell(fp);
fseek(fp, 0, SEEK_SET);
buffer = malloc(fileLength + 1);
fread(buffer, fileLength, 1, fp);
fclose(fp);
buffer[fileLength] = '\0';
Or just use piglit_load_text_file.
buffer = pitlit_load_text_file(filename, & fileLength);
> +
> + i=0;
> +
> + while(buffer[i] != '\0')
> + {
> + if(!strncmp(&(buffer[i]),"[test]",6))
> + ++numTests;
> + ++i;
> + }
> +
> + testLengths = (int *)malloc(sizeof(int)*numTests);
> + for(i=0;i<numTests;++i)
> + testLengths[i] = 0;
Use calloc instead.
> +
> + i=0;
> +
Instead of using an index, it seems like just walking a pointer would be
easier.
> + while(buffer[i] != '\0')
> + {
> + if(buffer[i] == '[')
> + {
> + switch(state)
> + {
> + case 1: //state 1 records vertex shader length
> + vsLength = secLength;
> + break;
> + case 2: //state 2 records fragment shader length
> + fsLength = secLength;
> + break;
> + case 3: //state 3 records test info length
> + testLengths[currentTest-1] = secLength;
> + break;
> + case 4: //state 4 records requirements length
> + reqsLength = secLength;
> + break;
> + }
Use an enum for state.
enum file_section {
start = 0,
vertex_section,
fragment_section,
test_section,
requirements_section
};
> + secLength = 0;
> +
> + if(state == 0)
> + cP = strchr(buffer,']');
> + else
> + cP = strchr(cP+1,']');
> +
> + strncpy(word,&(buffer[i+1]),(int)(cP-&(buffer[i+1])));
Why make a copy? It seems like this should be:
if (strncmp("[vertex]\n", buffer[i], 9) == 0) {
vsText = &buffer[i + 9];
state = vertex_section;
i += 9;
} else if (strncmp("[fragment]\n", buffer[i], 11) == 0) {
...
} ...
> + if(!strncmp(word,"vertex",6))
> + {
> + vsText = cP+1;
> + i = cP+1-buffer;
> + state = 1;
> + }
> + if(!strncmp(word,"fragment",8))
> + {
> + fsText = cP+1;
> + i = cP+1-buffer;
> + state = 2;
> + }
> + if(!strncmp(word,"test",4))
> + {
> + testTexts[currentTest] = cP+1;
> + i = cP+1-buffer;
> + state = 3;
> + ++currentTest;
> + }
> + if(!strncmp(word,"requirements",12))
> + {
> + reqsText = cP+1;
> + i = cP+1-buffer;
> + state = 4;
> + }
> +
> + }
> + ++secLength;
> + ++i;
> + }
> + switch(state)
> + {
> + case 1: //state 1 records vertex shader length
> + vsLength = secLength;
> + break;
> + case 2: //state 2 records fragment shader length
> + fsLength = secLength;
> + break;
> + case 3: //state 3 records test info length
> + testLengths[currentTest-1] = secLength;
> + break;
> + case 4: //state 4 records requirements length
> + reqsLength = secLength;
> + break;
> + }
> +
> + if(vsLength < 1 || fsLength < 1)
> + piglit_report_result(PIGLIT_FAILURE);
> + req_parse();
> +}
> +
> +static void
> +compileLinkProg()
> +{
> + GLint stat;
> +
> + vs = glCreateShader(GL_VERTEX_SHADER);
> + fs = glCreateShader(GL_FRAGMENT_SHADER);
> +
> + glShaderSource(vs, 1, (const GLchar **) &vsText, &vsLength);
> + glShaderSource(fs, 1, (const GLchar **) &fsText, &fsLength);
> +
> + glCompileShader(vs);
> + glGetShaderiv(vs, GL_COMPILE_STATUS, &stat);
> + if (!stat) {
> + printf("error compiling vertex shader1!\n");
> + exit(1);
> + }
> +
> + glCompileShader(fs);
> + glGetShaderiv(fs, GL_COMPILE_STATUS, &stat);
> + if (!stat) {
> + printf("error compiling fragment shader1!\n");
> + exit(1);
> + }
> +
> + prog = glCreateProgram();
> + glAttachShader(prog, vs);
> + glAttachShader(prog, fs);
> + glLinkProgram(prog);
> + glUseProgram(prog);
> +}
> +
> +static void
> +loadTex()
> +{
I just added a piglit_checkerboard_texture function that does this.
That function should be used instead of this one.
> + int height = 2;
> + int width = 2;
> + int i, j;
> +
> + GLfloat texData[width][height][4];
> + for (i=0; i < width; ++i) {
> + for (j=0; j < height; ++j) {
> + if ((i+j) & 1) {
> + texData[i][j][0] = 1;
> + texData[i][j][1] = 0;
> + texData[i][j][2] = 1;
> + texData[i][j][3] = 0;
> + }
> + else {
> + texData[i][j][0] = 0;
> + texData[i][j][1] = 1;
> + texData[i][j][2] = 0;
> + texData[i][j][3] = 1;
> + }
> + }
> + }
> +
> + glGenTextures(1, &tex);
> + glActiveTexture(GL_TEXTURE0);
> + glBindTexture(GL_TEXTURE_2D, tex);
> + glTexParameteri(GL_TEXTURE_2D, GL_GENERATE_MIPMAP, GL_FALSE);
> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
> + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0,
> + GL_RGBA, GL_FLOAT, texData);
> +
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + int i;
> + filename[0] = '\0';
> + if (!GLEW_VERSION_2_0) {
> + printf("Requires OpenGL 2.0\n");
> + piglit_report_result(PIGLIT_SKIP);
> + }
Just handle this in req_parse? That would require some (trivial) rework
to that function, but it seems sensible. I suspect we'll eventually
extend this to support assembly shaders, so there may not be a hard GL
2.0 requirement in the future.
> +
> + for(i=1;i<argc;++i)
> + {
> + strncpy(filename, argv[i], strlen(argv[i]));
> + }
Uh... what? strncpy(a, b, strlen(b)) is just strcpy. Seriously.
There's no reason for any of this madness anyway. This code should just be:
if (argc < 2) {
printf("Usage: %s test_file_name\n", argv[0]);
piglit_report_result(PIGLIT_FAILURE);
}
And pass argv[1] as a parameter to testFile_parse.
> +
> + if(strlen(filename) < 5 || strstr(filename,".ini") == NULL)
There's no reason to force the shader files to end in '.ini'. In fact,
I think naming them *.ini is a horrible idea. :) They use the .ini
/format/, but they're not "initialization" files.
> + piglit_report_result(PIGLIT_FAILURE);
> +
> + printf("using test file: %s\n",filename);
argv[1]
> +
> + piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);
> +
> + glEnable(GL_TEXTURE_2D);
> +
> + glClearColor(0.6, 0.6, 0.6, 1.0);
> +
> + probe.color[0]=0.0;
> + probe.color[1]=0.0;
> + probe.color[2]=0.0;
> + probe.pos[0]=0.5;
> + probe.pos[1]=0.5;
> +
> + testFile_parse();
> + compileLinkProg();
> + loadTex();
> +}
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +
> + int i;
> + int x=0, y=0;
> + GLboolean pass = GL_TRUE;
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + glUseProgram(prog);
> +
> + for(i=0;i<numTests;++i)
> + {
> + glPushMatrix();
> +
> + if(i%(int)(piglit_width/(width+1)) == 0 && i!=0)
> + {
> + y+=height+1;
> + x=0;
> + }
> +
> + test_parse(i);
> + glTranslatef(x,y,0);
> + piglit_draw_rect_tex(0,0,width,height,0,0,1,1);
> + pass = pass && piglit_probe_pixel_rgb((probe.pos[0]*width)+x,
> + (probe.pos[1]*height)+y,
> + probe.color);
> + x+=width+1;
> + glPopMatrix();
> + }
> +
> + glFinish();
Assuming there are no driver bugs, there is no reason to glFinish.
Assuming there are driver bugs, calling glFinish may hide them.
> + glutSwapBuffers();
> +
> + return pass ? PIGLIT_SUCCESS : PIGLIT_FAILURE;
> +}
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAksVhVsACgkQX1gOwKyEAw9VhACeN+fOYc7uXxngjbf0EQO92kvJ
m6AAnAniAeS7xM9tKf1UWpeePEaQIHYj
=KbUI
-----END PGP SIGNATURE-----
------------------------------------------------------------------------------
Join us December 9, 2009 for the Red Hat Virtual Experience,
a free event focused on virtualization and cloud computing.
Attend in-depth sessions from your desk. Your couch. Anywhere.
http://p.sf.net/sfu/redhat-sfdev2dev
_______________________________________________
Mesa3d-dev mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
