Query objects should not be shared between contexts

gpu/command_buffer/client/gles2_implementation.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // A class to emulate GLES2 over command buffers.
 
 #include "gpu/command_buffer/client/gles2_implementation.h"
 
 #include <GLES2/gl2ext.h>
 #include <GLES2/gl2extchromium.h>
 #include <algorithm>
 #include <limits>
 #include <map>
 #include <queue>
 #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"
 #include "gpu/command_buffer/client/gpu_memory_buffer_tracker.h"
 #include "gpu/command_buffer/client/program_info_manager.h"
 #include "gpu/command_buffer/client/query_tracker.h"
 #include "gpu/command_buffer/client/transfer_buffer.h"
 #include "gpu/command_buffer/client/vertex_array_object_manager.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
 #include "gpu/command_buffer/common/trace_event.h"
@@ skipping 157 matching lines @@
       static_state_.int_state.num_shader_binary_formats);
 
   texture_units_.reset(
       new TextureUnit[
           static_state_.int_state.max_combined_texture_image_units]);
 
   query_tracker_.reset(new QueryTracker(mapped_memory_.get()));
   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]);
 #endif
 
   vertex_array_object_manager_.reset(new VertexArrayObjectManager(
       static_state_.int_state.max_vertex_attribs,
       reserved_ids_[0],
       reserved_ids_[1]));
 
@@ skipping 110 matching lines @@
 
   // Make sure the commands make it the service.
   WaitForCmd();
 }
 
 GLES2CmdHelper* GLES2Implementation::helper() const {
   return helper_;
 }
 
 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();
 }
 
 int32 GLES2Implementation::GetResultShmId() {
   return transfer_buffer_->GetShmId();
 }
 
 uint32 GLES2Implementation::GetResultShmOffset() {
   return transfer_buffer_->GetResultOffset();
@@ skipping 223 matching lines @@
         }
         helper_->GetBucketData(
             bucket_id, offset, buffer.size(), buffer.shm_id(), buffer.offset());
         WaitForCmd();
       }
       uint32 size_to_copy = std::min(size, buffer.size());
       memcpy(&(*data)[offset], buffer.address(), size_to_copy);
       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.
     helper_->SetBucketSize(bucket_id, 0);
   }
   return true;
 }
 
 void GLES2Implementation::SetBucketContents(
     uint32 bucket_id, const void* data, size_t size) {
@@ skipping 732 matching lines @@
   GPU_CLIENT_LOG("[" << GetLogPrefix() << "] glVertexAttribDivisorANGLE("
       << index << ", "
       << divisor << ") ");
   // Record the info on the client side.
   vertex_array_object_manager_->SetAttribDivisor(index, divisor);
   helper_->VertexAttribDivisorANGLE(index, divisor);
   CheckGLError();
 }
 
 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 << ", "
       << static_cast<const void*>(source) << ", "
       << static_cast<const void*>(length) << ")");
   GPU_CLIENT_LOG_CODE_BLOCK({
     for (GLsizei ii = 0; ii < count; ++ii) {
       if (source[ii]) {
         if (length && length[ii] >= 0) {
           std::string str(source[ii], length[ii]);
@@ skipping 239 matching lines @@
   if (!*buffer_id) {
     SetGLError(GL_INVALID_OPERATION, function_name, "no buffer bound");
   }
   return true;
 }
 
 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) {
     SetGLError(GL_INVALID_OPERATION, function_name, "invalid buffer");
     return NULL;
   }
   if (buffer->mapped()) {
     SetGLError(GL_INVALID_OPERATION, function_name, "buffer mapped");
     return NULL;
   }
@@ skipping 1682 matching lines @@
     SetGLError(
         GL_INVALID_VALUE, "glProgramInfoCHROMIUM", "bufsize less than 0.");
     return;
   }
   if (size == NULL) {
     SetGLError(GL_INVALID_VALUE, "glProgramInfoCHROMIUM", "size is null.");
     return;
   }
   // 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()) {
     return;
   }
   *size = result.size();
   if (!info) {
     return;
   }
   if (static_cast<size_t>(bufsize) < result.size()) {
@@ skipping 26 matching lines @@
   helper_->PostSubBufferCHROMIUM(x, y, width, height);
   helper_->CommandBufferHelper::Flush();
   if (swap_buffers_tokens_.size() > kMaxSwapBuffers + 1) {
     helper_->WaitForToken(swap_buffers_tokens_.front());
     swap_buffers_tokens_.pop();
   }
 }
 
 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) {
+    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.

   }
 
-  for (GLsizei ii = 0; ii < n; ++ii)
-    query_tracker_->RemoveQuery(queries[ii]);
-
   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 << ")");
 
   // TODO(gman): To be spec compliant IDs from other contexts sharing
   // resources need to return true here even though you can't share
   // queries across contexts?
   return query_tracker_->GetQuery(id) != NULL;
 }
 
@@ skipping 10 matching lines @@
         GL_INVALID_OPERATION, "glBeginQueryEXT", "query already in progress");
     return;
   }
 
   // id = 0 INV_OP
   if (id == 0) {
     SetGLError(GL_INVALID_OPERATION, "glBeginQueryEXT", "id is 0");
     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);
   if (!query) {
     query = query_tracker_->CreateQuery(id, target);
     if (!query) {
       SetGLError(GL_OUT_OF_MEMORY,
                  "glBeginQueryEXT",
                  "transfer buffer allocation failed");
       return;
@@ skipping 771 matching lines @@
   return true;
 }
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "gpu/command_buffer/client/gles2_implementation_impl_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu