[Mesa-dev] [PATCH] Remove error when calling glGenQueries/glDeleteQueries while a query is active

[Brian Paul]

On 10/17/2013 02:35 PM, Carl Worth wrote:
> There is nothing in the OpenGL specification which prevents the user from
> calling glGenQueries to generate a new query object while another object is
> active. Neither is there anything in the Mesa implementation which prevents
> this. So remove the INVALID_OPERATION errors in this case.
>
> Similarly, it is explicitly allowed by the OpenGL specification to delete an
> active query, so remove the assertion for that case and be sure to call the
> driver's EndQuery hook.
>
> CC: <mesa-stable at lists.freedesktop.org>
> ---
>
> Brian Paul <brianp at vmware.com> writes:
>> On 10/17/2013 12:14 PM, Carl Worth wrote:
>> But from http://www.opengl.org/registry/specs/ARB/occlusion_query.txt:
>>
>> """
>>       Calling either GenQueriesARB or DeleteQueriesARB while any query of
>>       any target is active causes an INVALID_OPERATION error to be
>>       generated.
>> """
>> (it's about half-way down in the file)  It's also mentioned in the
>> "Errors" section.
>
> Thanks, Brian. That certainly does justify where the original code came
> from.
>
>> Maybe that was rescinded since that spec was done.  If so, I'm fine with
>> removing the code.
>
> I can't find any similar error requirement in the OpenGL 4.4 (Core)
> specification. (And with the increased number of different query types,
> it doesn't seem that the error requirement makes sense.) I did find the
> following sentence in the specification (section 4.2):
>
> 	If an active query object is deleted its name immediately
> 	becomes unused, but the underlying object is not deleted until
> 	it is no longer active.
>
> This sentence presumes the possibility of deleting an active query, so I
> think it is reasonable to remove the error.
>
>> However, I wouldn't be surprised if our drivers crashed and burned if an
>> active query is deleted.  gl_query_object isn't referenced counted.
>
> Thanks for the catch. My revised patch below calls the driver's EndQuery
> hook, which will hopefully avoid this problem. This does inactivate the
> query immediately at delete time, which might seem inconsistent with the
> sentence I quoted from the specification above. But I think this is fine
> since once the object is deleted the user has no visibility into whether
> the object is active or not.
>
> -Carl
>
>   src/mesa/main/queryobj.c | 19 ++++---------------
>   1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/main/queryobj.c b/src/mesa/main/queryobj.c
> index a180133..ebdb71c 100644
> --- a/src/mesa/main/queryobj.c
> +++ b/src/mesa/main/queryobj.c
> @@ -202,13 +202,6 @@ _mesa_GenQueries(GLsizei n, GLuint *ids)
>         return;
>      }
>
> -   /* No query objects can be active at this time! */
> -   if (ctx->Query.CurrentOcclusionObject ||
> -       ctx->Query.CurrentTimerObject) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "glGenQueriesARB");
> -      return;
> -   }
> -
>      first = _mesa_HashFindFreeKeyBlock(ctx->Query.QueryObjects, n);
>      if (first) {
>         GLsizei i;
> @@ -241,18 +234,14 @@ _mesa_DeleteQueries(GLsizei n, const GLuint *ids)
>         return;
>      }
>
> -   /* No query objects can be active at this time! */
> -   if (ctx->Query.CurrentOcclusionObject ||
> -       ctx->Query.CurrentTimerObject) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "glDeleteQueriesARB");
> -      return;
> -   }
> -
>      for (i = 0; i < n; i++) {
>         if (ids[i] > 0) {
>            struct gl_query_object *q = _mesa_lookup_query_object(ctx, ids[i]);
>            if (q) {
> -            ASSERT(!q->Active); /* should be caught earlier */
> +            if (q->Active) {
> +               q->Active = GL_FALSE;
> +               ctx->Driver.EndQuery(ctx, q);

Valgrind found an invalid pointer (and crashed!) when I modified your 
piglit test (see other msg).  We also need to make sure that the 
ctx->Query.CurrentFoo binding point is cleared.  Something like this:

             if (q->Active) {
                struct gl_query_object **bindpt =
                   get_query_binding_point(ctx, q->Target);
                assert(bindpt);  /* _should_ be non-null if q is active */
                if (bindpt) {
                   *bindpt = NULL;
                }
                ...


> +            }
>               _mesa_HashRemove(ctx->Query.QueryObjects, ids[i]);
>               ctx->Driver.DeleteQuery(ctx, q);
>            }
>