<div dir="ltr">On 28 August 2013 11:49, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On 08/28/2013 10:57 AM, Paul Berry wrote: > On 28 August 2013 10:55, 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: > > 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> > <mailto:<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> <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> > <mailto:<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">> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>>> > > > > 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> > <mailto:<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">> <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> </div>> <mailto:<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> <mailto:<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a> <div class="im">> <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>> </div>> > <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>>> <div class="im">> > 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>> <mailto:<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. > > 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. </div></div>Well... you learn something new every day. That's an argument against as least the current version of plan B. Do we have test cases for this? </blockquote><div> </div><div>None that I can find. I've made a note of it in my to-do list, but I doubt I'll get to it until geometry shaders are a lot further along. </div> </div></div></div>