Flag INV_OP during BeginQuery if id is not obtained from GenQueriesEXT.

Flag INV_OP during BeginQuery if id is not obtained from GenQueriesEXT.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265360

[rptr, 2014-04-13 16:56:53]

PTAL.

[rptr, 2014-04-14 14:33:29]

PTAL.

[piman, 2014-04-14 19:39:25]

Why? The spec says if the query id doesn't exist it is created.

[rptr, 2014-04-15 01:13:40]

On 2014/04/14 19:39:25, piman wrote:
> Why? The spec says if the query id doesn't exist it is created.
hmm, before starting on TODO by gman, i checked from spec
http://www.khronos.org/registry/gles/extensions/EXT/EXT_occlusion_query_boole...

Initially it says for BeginQueryEXT : 
    If the query object name <id> has not been created, the name is marked as
used and associated with a new 
    query object of the type specified by <target>.

then in next line it says:
    BeginQueryEXT fails and an INVALID_OPERATION error is generated if <id> 
    is not a name returned from a previous call to GenQueriesEXT, or if such 
    a name has since been deleted with DeleteQueriesEXT.

isn't this contradictory?

Again in error section spec says:
Errors:
    The error INVALID_OPERATION is generated if BeginQueryEXT is called
    where <id> is not a name returned from a previous call to GenQueriesEXT,
    or if such a name has since been deleted with DeleteQueriesEXT.

[rptr, 2014-04-15 01:29:10]

On 2014/04/15 01:13:40, rptr wrote:
> On 2014/04/14 19:39:25, piman wrote:
> > Why? The spec says if the query id doesn't exist it is created.
> hmm, before starting on TODO by gman, i checked from spec
>
http://www.khronos.org/registry/gles/extensions/EXT/EXT_occlusion_query_boole...
> 
> Initially it says for BeginQueryEXT : 
>     If the query object name <id> has not been created, the name is marked as
> used and associated with a new 
>     query object of the type specified by <target>.
> 
> then in next line it says:
>     BeginQueryEXT fails and an INVALID_OPERATION error is generated if <id> 
>     is not a name returned from a previous call to GenQueriesEXT, or if such 
>     a name has since been deleted with DeleteQueriesEXT.
> 
> isn't this contradictory?
> 
> Again in error section spec says:
> Errors:
>     The error INVALID_OPERATION is generated if BeginQueryEXT is called
>     where <id> is not a name returned from a previous call to GenQueriesEXT,
>     or if such a name has since been deleted with DeleteQueriesEXT.

just to be sure i checked latest gles spec3.1 what it says:
query must be a name previously returned from a call to glGenQueries. If a
"query object" with name id does not yet exist it is created with the type
determined by target. 

so spec is not contradicting : its saying BeginQueryEXT should create "query
object" if its not there yet. 
but id name should be generated by a previous call to glGenQueries. 
This is exactly what is done with new change. 

Please correct me I'm missing something here.

[piman, 2014-04-15 01:42:19]

On Mon, Apr 14, 2014 at 6:29 PM, <puttaraju.r@samsung.com> wrote:

> On 2014/04/15 01:13:40, rptr wrote:
>
>> On 2014/04/14 19:39:25, piman wrote:
>> > Why? The spec says if the query id doesn't exist it is created.
>> hmm, before starting on TODO by gman, i checked from spec
>>
>
> http://www.khronos.org/registry/gles/extensions/EXT/
> EXT_occlusion_query_boolean.txt
>
>  Initially it says for BeginQueryEXT :
>>      If the query object name <id> has not been created, the name is
>> marked as
>> used and associated with a new
>>      query object of the type specified by <target>.
>>
>
>  then in next line it says:
>>      BeginQueryEXT fails and an INVALID_OPERATION error is generated if
>> <id>
>>      is not a name returned from a previous call to GenQueriesEXT, or if
>> such
>>      a name has since been deleted with DeleteQueriesEXT.
>>
>
>  isn't this contradictory?
>>
>
>  Again in error section spec says:
>> Errors:
>>      The error INVALID_OPERATION is generated if BeginQueryEXT is called
>>      where <id> is not a name returned from a previous call to
>> GenQueriesEXT,
>>      or if such a name has since been deleted with DeleteQueriesEXT.
>>
>
> just to be sure i checked latest gles spec3.1 what it says:
> query must be a name previously returned from a call to glGenQueries. If a
> "query object" with name id does not yet exist it is created with the type
> determined by target.
>
> so spec is not contradicting : its saying BeginQueryEXT should create
> "query
> object" if its not there yet.
> but id name should be generated by a previous call to glGenQueries.
> This is exactly what is done with new change.
>
> Please correct me I'm missing something here.
>

I see. That's a discrepancy between ARB_occlusion_query/Desktop GL and
EXT_occlusion_query_boolean/GLES.

Then let's go for the change. However, we need more changes, to also fix
the service side (e.g. GLES2DecoderImpl::HandleBeginQueryEXT).


>
>
> https://codereview.chromium.org/226483003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-15 01:48:09]

Actually, I think it is simpler to do it only on the service side, it can
generate the error, that will trickle back to the client (similar to say
UseProgram).

[rptr, 2014-04-19 11:48:45]

Please take a look.

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (left):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/client/gles2_implementation_unittest.cc:2910: // TODO(gman):
Check for invalid query id is now in service side.

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9591: // if
(!id_allocator->InUse(client_id)) {
id_allocator->InUse(client_id) won't work as we have separate instances of
id_allocator for queries in client(ShareGroup) and service(ContextGroup).

[piman, 2014-04-22 00:37:30]

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9470: // NOTE: We don't generate
Query objects here. Only in BeginQueryEXT
Please remove comment that is now invalid

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9485:
query_manager_->RemoveQuery(client_ids[ii]);
This should be called regardless of whether query exists / is deleted (see
comment in RemoveQuery).

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7074: ASSERT_TRUE(query
== NULL);
nit: EXPECT_TRUE

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/query_manager.cc (right):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/query_manager.cc:524: return it !=
generated_query_ids_.end() ? true : false;
'? true : false' doesn't add anything. Just return it !=
generated_query_ids_.end()

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/query_manager.cc:537:
generated_query_ids_.erase(client_id);
According to spec, this must be removed in DeleteQueriesEXT even if no query was
created with BeginQueryEXT.

So, this should be out of the if test.

[rptr, 2014-04-22 05:43:18]

PTAL.

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9470: // NOTE: We don't generate
Query objects here. Only in BeginQueryEXT
On 2014/04/22 00:37:31, piman wrote:
> Please remove comment that is now invalid

Done.

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9485:
query_manager_->RemoveQuery(client_ids[ii]);
On 2014/04/22 00:37:31, piman wrote:
> This should be called regardless of whether query exists / is deleted (see
> comment in RemoveQuery).

Done.

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7074: ASSERT_TRUE(query
== NULL);
On 2014/04/22 00:37:31, piman wrote:
> nit: EXPECT_TRUE

Done.

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
File gpu/command_buffer/service/query_manager.cc (right):

https://chromiumcodereview.appspot.com/226483003/diff/40001/gpu/command_buffe...
gpu/command_buffer/service/query_manager.cc:537:
generated_query_ids_.erase(client_id);
On 2014/04/22 00:37:31, piman wrote:
> According to spec, this must be removed in DeleteQueriesEXT even if no query
was
> created with BeginQueryEXT.
> 
> So, this should be out of the if test.

Done.

[piman, 2014-04-22 17:16:16]

lgtm

[rptr, 2014-04-22 18:12:59]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-22 18:13:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/226483003/60001

[commit-bot: I haz the power, 2014-04-22 21:14:44]

Change committed as 265360