<div dir="ltr">On 28 August 2013 10:55, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im">On 08/28/2013 09:36 AM, Paul Berry wrote: > On 27 August 2013 18:45, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> </div><div class="im">> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: > > From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> </div>> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>> <div class="im">> > 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 <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> </div>> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>> > Cc: Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a> <mailto:<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>>> > Cc: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> > <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>> > Cc: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a> <mailto:<a href="mailto:eric@anholt.net">eric@anholt.net</a>>> <div><div class="h5">> --- > 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. </div></div>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. <div class="im"> > 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; > } </div>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. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > (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 <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> </div>> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>> <div class=""><div class="h5">> > > > } > +#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 </div></div></blockquote></div> </div></div>