[Piglit] [PATCH 06/16] util: Use a VAO and VBO for generic attributes in piglit_draw_rect_from_arrays

[Paul Berry]

On 26 August 2013 11:37, Ian Romanick <idr at freedesktop.org> wrote:

> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This will allow users of piglit_draw_rect_from_arrays to function in
> desktop OpenGL core profile.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: Paul Berry <stereoytpe441 at gmail.com>
> ---
>  tests/util/piglit-util-gl-common.c | 53
> ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/tests/util/piglit-util-gl-common.c
> b/tests/util/piglit-util-gl-common.c
> index d95eeac..79edd76 100644
> --- a/tests/util/piglit-util-gl-common.c
> +++ b/tests/util/piglit-util-gl-common.c
> @@ -24,6 +24,7 @@
>  #include "piglit-util-gl-common.h"
>  #include <ctype.h>
>
> +#define BUFFER_OFFSET(i) ((char *)NULL + (i))
>
>  /**
>   * An array of pointers to extension strings.
> @@ -642,15 +643,55 @@ piglit_draw_rect_from_arrays(const void *verts,
> const void *tex,
>  #if defined(PIGLIT_USE_OPENGL_ES2) || defined(PIGLIT_USE_OPENGL_ES3) \
>         || defined(PIGLIT_USE_OPENGL)
>         if (!USE_FF(fixed_function_attributes)) {
> +               GLuint buf = 0;
> +               GLuint old_buf = 0;
> +               GLuint vao = 0;
> +               GLuint old_vao = 0;
> +
> +               /* Vertex array objects were added in both OpenGL 3.0 and
> +                * OpenGL ES 3.0.  The use of VAOs is required in desktop
> +                * OpenGL 3.1 (without GL_ARB_compatibility) and all
> desktop
> +                * OpenGL core profiles.  If the functionality is
> supported,
> +                * just use it.
> +                */
> +               if (piglit_get_gl_version() >= 30
> +                   ||
> piglit_is_extension_supported("GL_OES_vertex_array_object")
> +                   ||
> piglit_is_extension_supported("GL_ARB_vertex_array_object")) {
> +                       glGetIntegerv(GL_VERTEX_ARRAY_BINDING,
> +                                     (GLint *) &old_vao);
> +                       glGenVertexArrays(1, &vao);
> +                       glBindVertexArray(vao);
> +               }
> +
> +               glGetIntegerv(GL_ARRAY_BUFFER_BINDING,
> +                             (GLint *) &old_buf);
> +               glGenBuffers(1, &buf);
> +               glBindBuffer(GL_ARRAY_BUFFER, buf);
> +               glBufferData(GL_ARRAY_BUFFER,
> +                            (sizeof(GLfloat) * 4 * 4)
> +                            + (sizeof(GLfloat) * 4 * 2),
> +                       NULL,
> +                       GL_STATIC_DRAW);
>

Shouldn't we also condition this code on the presence of
ARB_vertex_buffer_object?  Or has that extension been around long enough
that we don't care to test systems that don't support it?


> +
>                 if (verts) {
> +                       glBufferSubData(GL_ARRAY_BUFFER,
> +                                       0,
> +                                       sizeof(GLfloat) * 4 * 4,
> +                                       verts);
>                         glVertexAttribPointer(PIGLIT_ATTRIB_POS, 4,
> GL_FLOAT,
> -                                             GL_FALSE, 0, verts);
> +                                             GL_FALSE, 0,
> +                                             BUFFER_OFFSET(0));
>                         glEnableVertexAttribArray(PIGLIT_ATTRIB_POS);
>                 }
>
>                 if (tex) {
> +                       glBufferSubData(GL_ARRAY_BUFFER,
> +                                       sizeof(GLfloat) * 4 * 4,
> +                                       sizeof(GLfloat) * 4 * 2,
> +                                       tex);
>                         glVertexAttribPointer(PIGLIT_ATTRIB_TEX, 2,
> GL_FLOAT,
> -                                             GL_FALSE, 0, tex);
> +                                             GL_FALSE, 0,
> +
> BUFFER_OFFSET(sizeof(GLfloat) * 4 * 4));
>                         glEnableVertexAttribArray(PIGLIT_ATTRIB_TEX);
>                 }
>
> @@ -660,6 +701,14 @@ piglit_draw_rect_from_arrays(const void *verts, const
> void *tex,
>                         glDisableVertexAttribArray(PIGLIT_ATTRIB_POS);
>                 if (tex)
>                         glDisableVertexAttribArray(PIGLIT_ATTRIB_TEX);
> +
> +               glBindBuffer(GL_ARRAY_BUFFER, old_buf);
> +               glDeleteBuffers(1, &buf);
> +
> +               if (vao != 0) {
> +                       glBindVertexArray(old_vao);
> +                       glDeleteVertexArrays(1, &vao);
> +               }
>

I don't think this does what you want in the case where the implementation
supports vao's, but there was no vao bound before the call to
piglit_draw_rect_from_arrays().  (What it does is: generate a vao, bind it,
use it, and then leave it bound.  What I think you want is: generate a vao,
bind it, use it, unbind it, and delete it).

I think you need to do something like this instead:

bool use_vaos = piglit_get_gl_version() >= 30 || ...;
if (use_vaos) {
   glGetIntegerv(GL_VERTEX_ARRAY_BINDING...);
   ...
}

...

if (use_vaos) {
   glBindVertexArray(old_vao);
   glDeleteVertexArrays(1, &vao);
}

With that fixed, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>


>         }
>  #endif
>  }
> --
> 1.8.1.4
>
> _______________________________________________
> 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/20130827/6bda84a7/attachment-0001.html>