[Piglit] [PATCH] varying: Checks if driver can handle MAX_VARYING_FLOATS in mixed float, vec*.
Paul Berry
stereotype441 at gmail.com
Fri Feb 10 10:36:19 PST 2012
Before I get into the bulk of my comments, I want to talk about the general
approach you're taking here. To make the discussion easier, here's the
shaders your code produces if GL_MAX_VARYING_FLOATS=16, the command line
args are "0.25 0.25 0.25 0.25", and data_varying=7:
VS:
varying float v0;
varying float v1;
varying float v2;
varying float v3;
varying vec2 v4;
varying vec2 v5;
varying vec3 v6;
varying vec4 v7;
attribute vec4 green;
attribute float v_zero;
void main()
{
gl_Position = (gl_ModelViewProjectionMatrix *
gl_Vertex);
v0 = v_zero;
v1 = v_zero;
v2 = v_zero;
v3 = v_zero;
v4.x = v_zero;
v4.y = v_zero;
v5.x = v_zero;
v5.y = green.x;
v6.x = green.y;
v6.y = green.z;
v6.z = green.w;
}
FS:
varying float v0;
varying float v1;
varying float v2;
varying float v3;
varying vec2 v4;
varying vec2 v5;
varying vec3 v6;
varying vec4 v7;
uniform float zero;
uniform float one;
void main()
{
vec4 val = vec4(0.0);
val.x += zero * v0;
val.y += zero * v1;
val.z += zero * v2;
val.w += zero * v3;
val.x += zero * v4.x;
val.y += zero * v4.y;
val.z += zero * v5.x;
val.x += one * v5.y;
val.y += one * v6.x;
val.z += one * v6.y;
val.w += one * v6.z;
gl_FragColor = val;
}
What the VS does is: set up 8 varyings that use up 15 varying components.
It fills the first 7 varying components with zero, the next 4 components
(which span two of the varyings) with the components of "green", and then
leaves the remaining varyings uninitialized. Then, the FS reconstitutes
the green color by multiplying the first 7 varying components by zero and
accumulating them into "val", then adding one times the next four
components (to get green back), and then ignoring the remaining varyings.
This is problematic because:
(a) some varyings are neither written by the vertex shader nor read by the
fragment shader, which means the linker can ignore them, so they won't
contribute to testing that varyings have been packed effectively. (I
suspect this is an easily fixable bug though)
(b) since the FS only checks four components at a time, it will fail to
notice if we have a bug that causes some of the varyings to accidentally be
packed in overlapping locations.
(c) since "green" is the pattern { 0.0, 1.0, 0.0, 0.0 }, we are effectively
only testing one varying component at a time, even though it looks like we
are testing four.
(d) the test is going to be time consuming because new vertex and fragment
shaders need to be compiled for each value of data_varying.
I would recommend switching to a technique where the VS writes a known
pattern into *all* varyings, and the FS checks that the proper pattern is
present. Something like this:
VS:
varying float v0;
varying float v1;
varying float v2;
varying float v3;
varying vec2 v4;
varying vec2 v5;
varying vec3 v6;
varying vec4 v7;
void main()
{
gl_Position = (gl_ModelViewProjectionMatrix *
gl_Vertex);
v0 = 1.0;
v1 = 2.0;
v2 = 3.0;
v3 = 4.0;
v4 = vec2(5.0, 6.0);
v5 = vec2(7.0, 8.0);
v6 = vec3(9.0, 10.0, 11.0);
v7 = vec4(12.0, 13.0, 14.0, 15.0);
}
FS:
varying float v0;
varying float v1;
varying float v2;
varying float v3;
varying vec2 v4;
varying vec2 v5;
varying vec3 v6;
varying vec4 v7;
void main()
{
if (v0 == 1.0 &&
v1 == 2.0 &&
v2 == 3.0 &&
v3 == 4.0 &&
v4 == vec2(5.0, 6.0) &&
v5 == vec2(7.0, 8.0) &&
v6 == vec3(9.0, 10.0, 11.0) &&
v7 == vec4(12.0, 13.0, 14.0, 15.0)) {
gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0); /* green */
} else {
gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0); /* red */
}
}
I also have some comments about coding style and some of the implementation
choices you made (see below). Some of them may be irrelevant if you decide
to follow my suggestions above.
On 8 February 2012 12:20, Vincent Lejeune <vljn at ovi.com> wrote:
> ---
> tests/shaders/CMakeLists.gl.txt | 1 +
> tests/shaders/glsl-max-varyings-2.c | 452
> +++++++++++++++++++++++++++++++++++
>
We're trying to move away from naming tests using numbers, since it's hard
to remember what variant 2 of a given test means. Can we rename this to
something more descriptive, perhaps "glsl-max-varyings-mixed"?
> 2 files changed, 453 insertions(+), 0 deletions(-)
> create mode 100644 tests/shaders/glsl-max-varyings-2.c
>
> diff --git a/tests/shaders/CMakeLists.gl.txt
> b/tests/shaders/CMakeLists.gl.txt
> index 9d72260..b00219e 100644
> --- a/tests/shaders/CMakeLists.gl.txt
> +++ b/tests/shaders/CMakeLists.gl.txt
> @@ -143,6 +143,7 @@ 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-mat-attribute glsl-mat-attribute.c)
> add_executable (glsl-max-varyings glsl-max-varyings.c)
> +add_executable (glsl-max-varyings-2 glsl-max-varyings-2.c)
> add_executable (glsl-useprogram-displaylist glsl-useprogram-displaylist.c)
> add_executable (glsl-routing glsl-routing.c)
> add_executable (shader_runner shader_runner.c)
> diff --git a/tests/shaders/glsl-max-varyings-2.c
> b/tests/shaders/glsl-max-varyings-2.c
> new file mode 100644
> index 0000000..45e7396
> --- /dev/null
> +++ b/tests/shaders/glsl-max-varyings-2.c
> @@ -0,0 +1,452 @@
> +/*
> + * Copyright © 2010 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:
> + * Eric Anholt <eric at anholt.net>
> + * Vincent Lejeune <vljn at ovi.com>
> + *
> + */
> +
> +/** @file glsl-max-varyings-2.c
> + *
> + * Tests whether GL_MAX_VARYING_FLOATS varying components can be used
> when packed
> + * as vec4, vec3, vec2, or float. Drivers have to pack varyings to pass
> this test.
>
Can we add some documentation here about the expected command-line
arguments?
> + */
> +
> +#include "piglit-util.h"
> +
> +#define MAX_VARYING 128
> +
> +/* 10x10 rectangles with 2 pixels of pad. Deal with up to 32 varyings. */
> +int piglit_width = (2 + MAX_VARYING * 12), piglit_height = (2 +
> MAX_VARYING * 12);
>
This leads to a window size that is too large for most screens
(1538x1538). This seems particularly wasteful since the test only draws
one row of rectangles.
> +int piglit_window_mode = GLUT_RGB | GLUT_DOUBLE;
> +
> +
> +enum varyings_type {
> + FLT = 0,
> + V2 = 1,
> + V3 = 2,
> + V4 = 3
> +};
> +
> +const char* names[] = { "float", "vec2", "vec3", "vec4" };
>
A more descriptive name would be helpful here, perhaps something like
"glsl_type_names"
> +
> +char temp[2048];
>
temp is only used within functions--it doesn't communicate data between
functions. For maintainability, it should be moved into the particular
functions that need it.
> +
> +float ratio[4];
> +unsigned amounts[4];
>
+int max_components;
>
amounts and max_components don't need to be global--the code would be
easier to follow if we passed them as parameters to the functions that need
it.
+
> +static void generate_varyings_decl(char* code)
> +{
> +
> + unsigned index = 0, i;
> +
> + static const char * varying_decl[] = {
> + "varying float v%d;\n",
> + "varying vec2 v%d;\n",
> + "varying vec3 v%d;\n",
> + "varying vec4 v%d;\n",
> + };
>
This array shouldn't be necessary, since we already have the names[]
array. We should be able to just use the string "varying %s v%d;\n".
> +
> + for (i = 0; i < amounts[FLT]; i++) {
> + sprintf(temp, varying_decl[FLT], index);
> + strcat(code, temp);
> + index ++;
> + }
> +
> + for (i = 0; i < amounts[V2]; i++) {
> + sprintf(temp, varying_decl[V2], index);
> + strcat(code, temp);
> + index ++;
> + }
> +
> + for (i = 0; i < amounts[V3]; i++) {
> + sprintf(temp, varying_decl[V3], index);
> + strcat(code, temp);
> + index ++;
> + }
> +
> + for (i = 0; i < amounts[V4]; i++) {
> + sprintf(temp, varying_decl[V4], index);
> + strcat(code, temp);
> + index ++;
> + }
>
This would be simpler as a nested loop:
for (varying_type = 0; varying_type < 4; ++varying_type) {
for (i = 0; i < amounts[varying_type]; ++i) {
sprintf(temp, "varying %s v%d;\n", names[varying_type], index++);
strcat(code, temp);
}
}
> +
> + return;
>
This return statement isn't necessary.
> +}
> +
> +static void
> +get_varying_and_component_from_index(unsigned index, unsigned *varying,
> unsigned *component)
> +{
> + // Accessing a varying float
> + if (index < amounts[FLT]) {
> + *varying = index;
> + *component = 4;
>
The use of 4 as a magic number to mean "no component, just use the whole
float" should be documented somewhere. Perhaps create an enum:
enum {
COMPONENT_X = 0,
COMPONENT_Y = 1,
COMPONENT_Z = 2,
COMPONENT_W = 3,
COMPONENT_ALL = 4,
};
> + return;
> + }
> +
> + // Accessing a component of a vec2
> + if (index < amounts[FLT] + 2 *amounts[V2]) {
> + unsigned index_from_v2_start = index - amounts[FLT];
> + *varying = index_from_v2_start / 2;
> + *varying += amounts[FLT];
> + *component = index_from_v2_start % 2;
> + return;
> + }
> +
> + // Accessing a component of a vec3
> + if (index < amounts[FLT] + 2 *amounts[V2] + 3 * amounts [V3]) {
> + unsigned index_from_v3_start = index - amounts[FLT] - 2 *
> amounts[V2];
> + *varying = index_from_v3_start / 3;
> + *varying += amounts[FLT] + amounts[V2];
> + *component = index_from_v3_start % 3;
> + return;
> + }
> +
> + // Accessing a component of a vec4
> + if (index < amounts[FLT] + 2 * amounts[V2] + 3 *amounts[V3] + 4 *
> amounts[V4]) {
> + unsigned index_from_v4_start = index - amounts[FLT] - 2 *
> amounts[V2] - 3 * amounts[V3];
> + *varying = index_from_v4_start / 4;
> + *varying += amounts[FLT] + amounts[V2] + amounts[V4];
> + *component = index_from_v4_start % 4;
> + return;
> + }
> +}
>
This function is unnecessarily complex. I'd recommend modifying
generate_varyings_decl() so that as a side effect it populates two arrays:
index_to_varying[];
index_to_component[];
Then just look up the values you need in those arrays rather than call
generate_varyings_decl().
> +
> +static void
> +write_zero(char *code, unsigned index, int component) {
> +
> + switch (component) {
> + case 0:
> + sprintf(temp,
> + " v%d.x = v_zero;\n", index);
> + break;
> + case 1:
> + sprintf(temp,
> + " v%d.y = v_zero;\n", index);
> + break;
> + case 2:
> + sprintf(temp,
> + " v%d.z = v_zero;\n", index);
> + break;
> + case 3:
> + sprintf(temp,
> + " v%d.w = v_zero;\n", index);
> + break;
> + default:
> + sprintf(temp,
> + " v%d = v_zero;\n", index);
> + break;
> + }
>
This doesn't need to be a switch statement. Do what you do in
write_green_component (use a static string array and index into it using
component).
> +
> + strcat(code, temp);
> +}
> +
> +static void
> +write_green_component(char *code, unsigned index, int component_dst,
> unsigned component_src) {
> +
> + static const char * component_suffix[] = { ".x", ".y", ".z", ".w", ""};
> + sprintf(temp,
> + " v%d%s = green%s;\n", index,
> component_suffix[component_dst], component_suffix[component_src]);
> + strcat(code, temp);
> +}
> +
> +/* Generate a VS that writes to num_varyings vec4s, and put
> + * interesting data in data_varying with 0.0 everywhere else.
>
This comment can't be right.
> + */
> +static GLint get_vs(int data_varying)
> +{
> + GLuint shader;
> + unsigned i, j;
> + char *code;
> +
> + code = malloc(2048 * 4);
> + code[0] = 0;
> +
> + generate_varyings_decl(code);
> +
> + sprintf(temp,
> + "attribute vec4 green;\n"
> + "attribute float v_zero;\n"
> + "void main()\n"
> + "{\n"
> + " gl_Position = (gl_ModelViewProjectionMatrix * \n"
> + " gl_Vertex);\n"
> + );
> + strcat(code, temp);
> +
> + for (i = 0; i < data_varying; i++) {
> + unsigned index, component;
> + get_varying_and_component_from_index(i, &index, &component);
> + write_zero(code, index, component);
> + }
> +
> + j = 0;
> + for (i = data_varying; i < data_varying + 4; i++) {
> + unsigned index, component;
> + get_varying_and_component_from_index(i, &index, &component);
> + write_green_component(code, index, component, j);
> + j++;
> + }
> +
> + for (i = data_varying + 4; i < max_components - 4; i++) {
>
I think this was meant to be "for (i = data_varying + 4; i <
max_components; i++) {".
> + unsigned index, component;
> + get_varying_and_component_from_index(i, &index, &component);
> + write_zero(code, index, component);
> + }
> +
> + sprintf(temp,
> + "}\n"
> + );
> + strcat(code, temp);
> +
> + shader = piglit_compile_shader_text(GL_VERTEX_SHADER, code);
> + /* printf("%s\n", code); */
> + free(code);
> +
> + return shader;
> +}
>
+
> +static void
> +read_to_val_component(char *code, unsigned index, int component_src,
> unsigned component_dst, const char *coeff) {
> +
> + static const char * component_suffix[] = { ".x", ".y", ".z", ".w", ""};
> + sprintf(temp,
> + " val%s += %s * v%d%s;\n", component_suffix[component_dst],
> coeff, index, component_suffix[component_src]);
> + strcat(code, temp);
> +}
> +
> +/* Generate a FS that does operations on num_varyings, yet makes only
> + * data_varying contribute to the output.
> + *
> + * We could make a single FS per num_varyings that did this by using a
> + * uniform for data_varying and some multiplication by comparisons
> + * (see glsl-routing for an example), but since we're linking a new
> + * shader each time anyway, this produces a simpler FS to read and
> + * verify.
> + */
> +static GLint get_fs(int data_varying)
> +{
> + GLuint shader;
> + unsigned i, j;
> + char temp[2048];
> + char *code;
> +
> + code = malloc(2048 * 4);
> + code[0] = 0;
> +
> + generate_varyings_decl(code);
> +
> +
> + sprintf(temp,
> + "uniform float zero;\n"
> + "uniform float one;\n"
> + "void main()\n"
> + "{\n"
> + " vec4 val = vec4(0.0);\n"
> + );
> + strcat(code, temp);
> +
> + for (i = 0; i < data_varying; i++) {
> + unsigned index, component;
> + get_varying_and_component_from_index(i, &index, &component);
> + read_to_val_component(code, index, component, i % 4, "zero");
> + }
> +
> + j = 0;
> + for (i = data_varying; i < data_varying + 4; i++) {
> + unsigned index, component;
> + get_varying_and_component_from_index(i, &index, &component);
> + read_to_val_component(code, index, component, j, "one");
> + j++;
> + }
> +
> + for (i = data_varying + 4; i < max_components - 4; i++) {
> + unsigned index, component;
> + get_varying_and_component_from_index(i, &index, &component);
> + read_to_val_component(code, index, component, i % 4, "zero");
> + }
> +
> + sprintf(temp,
> + " gl_FragColor = val;\n"
> + "}\n"
> + );
> + strcat(code, temp);
> + /* printf("%s\n", code); */
> + shader = piglit_compile_shader_text(GL_FRAGMENT_SHADER, code);
> + free(code);
> +
> + return shader;
> +}
> +
> +static int
> +coord_from_index(int index)
> +{
> + return 2 + 12 * index;
> +}
> +
> +static void
> +draw()
> +{
> + int data_varying;
> + float green[4][4] = { {0.0, 1.0, 0.0, 0.0},
> + {0.0, 1.0, 0.0, 0.0},
> + {0.0, 1.0, 0.0, 0.0},
> + {0.0, 1.0, 0.0, 0.0} };
> +
> + glVertexAttribPointer(1, 4, GL_FLOAT, GL_FALSE, 4 * sizeof(float),
> + green);
> + glVertexAttribPointer(2, 1, GL_FLOAT, GL_FALSE, 4 * sizeof(float),
> + green);
> + glEnableVertexAttribArray(1);
> + glEnableVertexAttribArray(2);
> +
> + for (data_varying = 0; data_varying < max_components - 4;
> data_varying ++) {
> + GLuint prog, vs, fs;
> + GLint loc;
> +
> + vs = get_vs(data_varying);
> + fs = get_fs(data_varying);
> +
> + prog = glCreateProgram();
> + glAttachShader(prog, vs);
> + glAttachShader(prog, fs);
> +
> + glBindAttribLocation(prog, 1, "green");
> + glBindAttribLocation(prog, 2, "v_zero");
> +
> + glLinkProgram(prog);
> + if (!piglit_link_check_status(prog))
> + piglit_report_result(PIGLIT_FAIL);
> +
> + glUseProgram(prog);
> +
> + loc = glGetUniformLocation(prog, "zero");
> + if (loc != -1) /* not used for num_varyings == 1 */
> + glUniform1f(loc, 0.0);
> +
> + loc = glGetUniformLocation(prog, "one");
> + assert(loc != -1); /* should always be used */
> + glUniform1f(loc, 1.0);
> +
> +
> + piglit_draw_rect(coord_from_index(data_varying),
> + coord_from_index(0),
> + 10,
> + 10);
> +
> + glDeleteShader(vs);
> + glDeleteShader(fs);
> + glDeleteProgram(prog);
> + }
> +}
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + int col;
> + GLboolean pass = GL_TRUE, warned = GL_FALSE;
> +
> + piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);
> +
> + glGetIntegerv(GL_MAX_VARYING_FLOATS, &max_components);
> +
> + printf("GL_MAX_VARYING_FLOATS = %i\n", max_components);
> +
> + if (max_components > MAX_VARYING) {
> + printf("test not designed to handle >%d varying vec4s.\n"
> + "(implementation reports %d components)\n",
> + max_components, MAX_VARYING);
> + max_components = MAX_VARYING;
> + warned = GL_TRUE;
> + }
> +
> + amounts[FLT] = max_components * ratio[FLT];
> + amounts[V2] = (max_components * ratio[V2]) / 2;
> + amounts[V3] = (max_components * ratio[V3]) / 4;
>
Was this meant to be "/ 3"? Or are you trying to make the test lenient, to
account for the fact that the GLES 2.0 varying packing algorithm sometimes
wastes space when packing vec3s? I would recommend using "/ 3". We can
always be lenient my just supplying smaller values for the third ratio on
the command line.
> + amounts[V4] = (max_components * ratio[V4]) /4;
>
Rounding errors will sometimes prevent us from using up all varying
components. I would recommend adding some code here that detects if that
happened, and increments some of the elements of the "amounts" array to
ensure that in a test that is trying to use up all available varying
components, we actually do use up all available varying components.
> +
> + max_components = amounts[FLT] + amounts[V2] + amounts[V3] +
> amounts[V4];
> +
> + glClearColor(0.5, 0.5, 0.5, 0.5);
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + draw();
> +
> + for (col = 0; col < max_components - 4; col ++) {
> + GLboolean ok;
> + float green[3] = {0.0, 1.0, 0.0};
> +
> + ok = piglit_probe_rect_rgb(coord_from_index(col),
> + coord_from_index(0),
> + 10, 10,
> + green);
> + if (!ok) {
> + unsigned index, component;
> + get_varying_and_component_from_index(col, &index,
> &component);
> + printf(" Failure with v%d varyings\n", index);
> + pass = GL_FALSE;
> + break;
> + }
> + }
> +
> + glutSwapBuffers();
> +
> + if (!pass)
> + return PIGLIT_FAIL;
> + if (warned)
> + return PIGLIT_WARN;
> + else
> + return PIGLIT_PASS;
> +}
> +
> +
> +void piglit_init(int argc, char **argv)
> +{
> + unsigned i;
> +
> + if (!GLEW_VERSION_2_0) {
> + printf("Requires OpenGL 2.0\n");
> + piglit_report_result(PIGLIT_SKIP);
> + }
>
The standard way to do this check is:
piglit_require_gl_version(20);
> +
> + if (argc < 5) {
> + fprintf(stderr, "Not enough args.\n");
> + exit(1);
> + }
>
I'd suggest changing this to "if (argc != 5)", because if more than 4 args
are supplied, that's a problem too.
> +
> + for (i = 1; i < argc; i++) {
> + sscanf(argv[i], "%f",&ratio[i - 1]);
> + if (ratio[i - 1] < 0) {
> + fprintf(stderr, "Ratio must be positive.\n");
> + exit(1);
> + }
> + }
> +
> + if (ratio[FLT] + ratio[V2] + ratio[V3] + ratio[V4] > 1.0) {
> + fprintf(stderr, "Ratio are not normalised.\n");
> + exit(1);
> + }
> +}
> +
> +
>
One final comment: in order for the test to be run by piglit-run.py, it
needs to be added to the file tests/all.tests. In that file, you can
specify the test multiple times with different command-line arguments, for
example:
shaders['glsl-max-varyings-mixed floats'] =
concurrent_test('glsl-max-varyings-mixed 1.0 0.0 0.0 0.0')
shaders['glsl-max-varyings-mixed floats vec2s'] =
concurrent_test('glsl-max-varyings-mixed 0.5 0.5 0.0 0.0')
...and so on.
> --
> 1.7.7
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120210/c7ab30ba/attachment-0001.html>
More information about the Piglit
mailing list