[Piglit] [PATCH 04/13] util: Allow non-ES2 code to draw_rect with non-fixed-function arrays
Ian Romanick
idr at freedesktop.org
Wed Aug 28 11:49:59 PDT 2013
On 08/28/2013 10:57 AM, Paul Berry wrote:
> On 28 August 2013 10:55, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> On 08/28/2013 09:36 AM, Paul Berry wrote:
> > On 27 August 2013 18:45, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>
> > <mailto:idr at freedesktop.org <mailto:idr at freedesktop.org>>> wrote:
> >
> > From: Ian Romanick <ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>
> > <mailto:ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>>>
> >
> > 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 <ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>
> > <mailto:ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>>>
> > Cc: Matt Turner <mattst88 at gmail.com
> <mailto:mattst88 at gmail.com> <mailto:mattst88 at gmail.com
> <mailto:mattst88 at gmail.com>>>
> > Cc: Paul Berry <stereotype441 at gmail.com
> <mailto:stereotype441 at gmail.com>
> > <mailto:stereotype441 at gmail.com <mailto:stereotype441 at gmail.com>>>
> > Cc: Eric Anholt <eric at anholt.net <mailto:eric at anholt.net>
> <mailto:eric at anholt.net <mailto:eric at anholt.net>>>
> > ---
> > 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.
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?
> > (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 <stereotype441 at gmail.com
> <mailto:stereotype441 at gmail.com>
> > <mailto:stereotype441 at gmail.com <mailto:stereotype441 at gmail.com>>>
> >
> >
> >
> > }
> > +#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
More information about the Piglit
mailing list