Query objects should not be shared between contexts

gpu/command_buffer/client/gles2_implementation.cc

Index: gpu/command_buffer/client/gles2_implementation.cc
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index 716fb55fbc2eaac205d249c20f9bbd4a2e761b1a..c26a85806dfd703cb61da28562774a433d86b0d6 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -15,6 +15,7 @@
 #include <set>
 #include <sstream>
 #include <string>
+#include <utility>

[no sievers, 2014/07/08 22:54:04]

needed?

[hj.r.chung, 2014/07/09 05:26:46]

Done.

 #include "base/bind.h"
 #include "gpu/command_buffer/client/buffer_tracker.h"
 #include "gpu/command_buffer/client/gpu_control.h"
@@ -192,6 +193,7 @@ bool GLES2Implementation::Initialize(
   buffer_tracker_.reset(new BufferTracker(mapped_memory_.get()));
   gpu_memory_buffer_tracker_.reset(new GpuMemoryBufferTracker(gpu_control_));
 
+  query_id_allocator_.reset(new IdAllocator());
 #if defined(GLES2_SUPPORT_CLIENT_SIDE_ARRAYS)
   GetIdHandler(id_namespaces::kBuffers)->MakeIds(
       this, kClientSideArrayId, arraysize(reserved_ids_), &reserved_ids_[0]);
@@ -325,6 +327,14 @@ IdHandlerInterface* GLES2Implementation::GetIdHandler(int namespace_id) const {
   return share_group_->GetIdHandler(namespace_id);

[no sievers, 2014/07/08 22:54:04]

How about DCHECK(namespace_id != id_namespaces::kQueries)?

[hj.r.chung, 2014/07/09 05:26:46]

Done.

[no sievers, 2014/07/09 17:50:48]

I don't see it :)

 }
 
+IdAllocatorInterface* GLES2Implementation::GetIdAllocator(
+    int namespace_id) const {
+  if (namespace_id == id_namespaces::kQueries)
+    return query_id_allocator_.get();
+
+  return NULL;

[no sievers, 2014/07/08 22:54:04]

NOTREACHED?

[hj.r.chung, 2014/07/09 05:26:46]

Done.

+}
+
 void* GLES2Implementation::GetResultBuffer() {
   return transfer_buffer_->GetResultBuffer();
 }
@@ -568,7 +578,7 @@ bool GLES2Implementation::GetBucketContents(uint32 bucket_id,
       offset += size_to_copy;
       size -= size_to_copy;
       buffer.Release();
-    };
+    }
     // Free the bucket. This is not required but it does free up the memory.
     // and we don't have to wait for the result so from the client's perspective
     // it's cheap.
@@ -1321,7 +1331,10 @@ void GLES2Implementation::VertexAttribDivisorANGLE(
 }
 
 void GLES2Implementation::ShaderSource(
-    GLuint shader, GLsizei count, const GLchar* const* source, const GLint* length) {
+    GLuint shader,
+    GLsizei count,
+    const GLchar* const* source,
+    const GLint* length) {
   GPU_CLIENT_SINGLE_THREAD_CHECK();
   GPU_CLIENT_LOG("[" << GetLogPrefix() << "] glShaderSource("
       << shader << ", " << count << ", "
@@ -1581,8 +1594,7 @@ BufferTracker::Buffer*
 GLES2Implementation::GetBoundPixelUnpackTransferBufferIfValid(
     GLuint buffer_id,
     const char* function_name,
-    GLuint offset, GLsizei size)
-{
+    GLuint offset, GLsizei size) {
   DCHECK(buffer_id);
   BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
   if (!buffer) {
@@ -3285,7 +3297,7 @@ void GLES2Implementation::GetProgramInfoCHROMIUM(
   }
   // Make sure they've set size to 0 else the value will be undefined on
   // lost context.
-  DCHECK(*size == 0);
+  DCHECK_EQ(0, *size);
   std::vector<int8> result;
   GetProgramInfoCHROMIUMHelper(program, &result);
   if (result.empty()) {
@@ -3332,26 +3344,14 @@ void GLES2Implementation::PostSubBufferCHROMIUM(
 
 void GLES2Implementation::DeleteQueriesEXTHelper(
     GLsizei n, const GLuint* queries) {
-  // TODO(gman): Remove this as queries are not shared resources.
-  if (!GetIdHandler(id_namespaces::kQueries)->FreeIds(
-      this, n, queries, &GLES2Implementation::DeleteQueriesStub)) {
-    SetGLError(
-        GL_INVALID_VALUE,
-        "glDeleteTextures", "id not created by this context.");
-    return;
-  }
-
-  for (GLsizei ii = 0; ii < n; ++ii)
+  for (GLsizei ii = 0; ii < n; ++ii) {
     query_tracker_->RemoveQuery(queries[ii]);
+    query_id_allocator_->FreeID(queries[ii]);

[no sievers, 2014/07/08 22:54:04]

You are removing the check for whether these IDs were allocated with GenQueries.
Maybe leave that in and still set the GL error in that case?

[hj.r.chung, 2014/07/09 05:26:46]

For now IdHandlerInterface::FreeIds always returns true, and glDeleteQueries
give errors only when id is negative.
Should I add the check for negative ids?

[no sievers, 2014/07/09 17:50:48]

Ok, I missed that the old code always returned true, so that's fine then.

+  }
 
   helper_->DeleteQueriesEXTImmediate(n, queries);
 }
 
-// TODO(gman): Remove this. Queries are not shared resources.
-void GLES2Implementation::DeleteQueriesStub(
-    GLsizei /* n */, const GLuint* /* queries */) {
-}
-
 GLboolean GLES2Implementation::IsQueryEXT(GLuint id) {
   GPU_CLIENT_SINGLE_THREAD_CHECK();
   GPU_CLIENT_LOG("[" << GetLogPrefix() << "] IsQueryEXT(" << id << ")");
@@ -3382,7 +3382,11 @@ void GLES2Implementation::BeginQueryEXT(GLenum target, GLuint id) {
     return;
   }
 
-  // TODO(gman) if id not GENned INV_OPERATION
+  // if not GENned INV_OPERATION
+  if (!query_id_allocator_->InUse(id)) {
+    SetGLError(GL_INVALID_OPERATION, "glBeginQueryEXT", "invalid id");
+    return;
+  }
 
   // if id does not have an object
   QueryTracker::Query* query = query_tracker_->GetQuery(id);