On 28 August 2013 10:55, Ian Romanick <[email protected]> wrote: > On 08/28/2013 09:36 AM, Paul Berry wrote: > > On 27 August 2013 18:45, Ian Romanick <[email protected] > > <mailto:[email protected]>> wrote: > > > > From: Ian Romanick <[email protected] > > <mailto:[email protected]>> > > > > This will be used in future commits. > > > > v2: Use current program state to determine whether to use > fixed-function > > attributes. There are two proposals in this patch. Hopefully > reviewers > > will pick one. The first method checks whether the current program > > contains an attribute named "piglit_vertex". If it does, > fixed-function > > attributes are not used. The other method check whether the current > > program contains an attribute whose name starts with "gl_". If it > does, > > fixed-function attributes are used. > > > > Signed-off-by: Ian Romanick <[email protected] > > <mailto:[email protected]>> > > Cc: Matt Turner <[email protected] <mailto:[email protected]>> > > Cc: Paul Berry <[email protected] > > <mailto:[email protected]>> > > Cc: Eric Anholt <[email protected] <mailto:[email protected]>> > > --- > > tests/util/piglit-util-gl-common.c | 123 > > ++++++++++++++++++++++++++++--------- > > 1 file changed, 95 insertions(+), 28 deletions(-) > > > > diff --git a/tests/util/piglit-util-gl-common.c > > b/tests/util/piglit-util-gl-common.c > > index b5e87bf..ec66a65 100644 > > --- a/tests/util/piglit-util-gl-common.c > > +++ b/tests/util/piglit-util-gl-common.c > > @@ -603,42 +603,109 @@ > > required_gl_version_from_glsl_version(unsigned glsl_version) > > void > > piglit_draw_rect_from_arrays(const void *verts, const void *tex) > > { > > -#if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL) > > - if (verts) { > > - glVertexPointer(4, GL_FLOAT, 0, verts); > > - glEnableClientState(GL_VERTEX_ARRAY); > > - } > > +#if defined(PIGLIT_USE_OPENGL_ES1) > > + static const bool use_fixed_function_attributes = true; > > +#elif defined(PIGLIT_USE_OPENGL_ES2) || > defined(PIGLIT_USE_OPENGL_ES3) > > + static const bool use_fixed_function_attributes = false; > > > > > > I realize I'm quibbling here, but using "static" on these consts seems > > strange to me. Since the compiler is going to optimize them away > > anyhow, it seems silly to ask it to put them in the data segment. In > > fact, it makes me worry that it might defeat constant folding for some > > stupid reason. > > > > > > +#elif defined(PIGLIT_USE_OPENGL) > > + bool use_fixed_function_attributes = true; > > +#else > > +#error "don't know how to draw arrays" > > +#endif > > > > - if (tex) { > > - glTexCoordPointer(2, GL_FLOAT, 0, tex); > > - glEnableClientState(GL_TEXTURE_COORD_ARRAY); > > +#if defined(PIGLIT_USE_OPENGL) > > + if (piglit_get_gl_version() >= 20 > > + || > piglit_is_extension_supported("GL_ARB_shader_objects")) { > > + GLuint prog; > > + > > + glGetIntegerv(GL_CURRENT_PROGRAM, (GLint *) &prog); > > + > > +#define PLAN_A > > +#ifdef PLAN_A > > + /* If there is a current program and that program > has an > > + * active attribute named piglit_vertex, don't use > > the fixed > > + * function inputs. > > + */ > > + use_fixed_function_attributes = prog == 0 > > + || glGetAttribLocation(prog, > > "piglit_vertex") == -1; > > +#elif defined PLAN_B > > + if (prog != 0) { > > + GLint num_attribs; > > + int i; > > + char name[64]; > > + GLint size; > > + GLenum type; > > + > > + /* Assume that fixed-function attributes > > won't be used > > + * until an attribute with the name "gl_*" > > is found. > > + */ > > + use_fixed_function_attributes = false; > > + > > + glGetProgramiv(prog, > > + GL_ACTIVE_ATTRIBUTES, > > + &num_attribs); > > + > > + for (i = 0; i < num_attribs; i++) { > > + glGetActiveAttrib(prog, > > + i, > > + sizeof(name), > > + NULL, > > + &size, > > + &type, > > + name); > > + > > + if (strncmp("gl_", name, 3) == 0) { > > + > > use_fixed_function_attributes = true; > > + break; > > + } > > + } > > + } > > +#endif > > > > > > I have a strong preference for plan A. Here's my reasons: > > > > 1. It's simpler. > > That is its primary advantage. As a disadvantage, it means that we have > to have a variable named "piglit_vertex". I've noticed that most of the > existing test that use 'draw arrays' name the vertex input "vertex". > Plan B would correctly identify those. > > What I wanted as a query that would tell me what name was bound to > attribute 0. I believe ARB_program_interface_query gives this ability, > but alas. > > > 2. I didn't have to look at the GL spec to reassure myself that it was > > correct (with plan B, I was briefly worried that glGetActiveAttrib would > > only return information about user-defined vertex inputs). > > > > 3. It correctly classifies shaders like this: > > > > in vec4 piglit_vertex; > > out vec4 vertex_id; > > void main() > > { > > gl_Position = piglit_vertex; > > vertex_id = gl_VertexID; > > } > > Are you sure? That seems surprising to me. It's a vertex shader input, > but it's not really an attribute (you can't source it's value from > anywhere). I wouldn't have thought that any of the gl_*ID variables > would be returned by glGetActiveAttrib. >
the GL 4.4 core spec says it does. From section 11.1.1 (Vertex Attributes), on page 343: For GetActiveAttrib, all active vertex shader input variables are enumerated, including the special built-in inputs gl_VertexID and gl_InstanceID. > > > (plan B would have incorrectly classified this as using fixed function > > attributes, because of the presence of gl_VertexID). > > > > If we choose plan A, this patch is: > > > > Reviewed-by: Paul Berry <[email protected] > > <mailto:[email protected]>> > > > > > > > > } > > +#endif > > > > - glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); > > +#if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL) > > + if (use_fixed_function_attributes) { > > + if (verts) { > > + glVertexPointer(4, GL_FLOAT, 0, verts); > > + glEnableClientState(GL_VERTEX_ARRAY); > > + } > > > > - if (verts) > > - glDisableClientState(GL_VERTEX_ARRAY); > > - if (tex) > > - glDisableClientState(GL_TEXTURE_COORD_ARRAY); > > -#elif defined(PIGLIT_USE_OPENGL_ES2) > ||defined(PIGLIT_USE_OPENGL_ES3) > > - if (verts) { > > - glVertexAttribPointer(PIGLIT_ATTRIB_POS, 4, > > GL_FLOAT, GL_FALSE, 0, verts); > > - glEnableVertexAttribArray(PIGLIT_ATTRIB_POS); > > - } > > + if (tex) { > > + glTexCoordPointer(2, GL_FLOAT, 0, tex); > > + glEnableClientState(GL_TEXTURE_COORD_ARRAY); > > + } > > + > > + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); > > > > - if (tex) { > > - glVertexAttribPointer(PIGLIT_ATTRIB_TEX, 2, > > GL_FLOAT, GL_FALSE, 0, tex); > > - glEnableVertexAttribArray(PIGLIT_ATTRIB_TEX); > > + if (verts) > > + glDisableClientState(GL_VERTEX_ARRAY); > > + if (tex) > > + glDisableClientState(GL_TEXTURE_COORD_ARRAY); > > } > > +#endif > > +#if defined(PIGLIT_USE_OPENGL_ES2) || > defined(PIGLIT_USE_OPENGL_ES3) \ > > + || defined(PIGLIT_USE_OPENGL) > > + if (!use_fixed_function_attributes) { > > + if (verts) { > > + glVertexAttribPointer(PIGLIT_ATTRIB_POS, 4, > > GL_FLOAT, > > + GL_FALSE, 0, verts); > > + glEnableVertexAttribArray(PIGLIT_ATTRIB_POS); > > + } > > > > - glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); > > + if (tex) { > > + glVertexAttribPointer(PIGLIT_ATTRIB_TEX, 2, > > GL_FLOAT, > > + GL_FALSE, 0, tex); > > + glEnableVertexAttribArray(PIGLIT_ATTRIB_TEX); > > + } > > > > - if (verts) > > - glDisableVertexAttribArray(PIGLIT_ATTRIB_POS); > > - if (tex) > > - glDisableVertexAttribArray(PIGLIT_ATTRIB_TEX); > > -#else > > -# error "don't know how to draw arrays" > > + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); > > + > > + if (verts) > > + > glDisableVertexAttribArray(PIGLIT_ATTRIB_POS); > > + if (tex) > > + > glDisableVertexAttribArray(PIGLIT_ATTRIB_TEX); > > + } > > #endif > > } > > > > -- > > 1.8.1.4 > >
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
