cc: Avoid issuing COMMANDS_COMPLETED queries unless necessary.

Minor change to the Fence API that allows GLRenderer to not
issue sync queries unless some resource that depend on them
was used to produce a frame.

Also includes some minor renaming cleanup related to usage of
Fences in ResourceProvider code.

Change-Id: I45165a8f6a91b3ad56c5db02489ea83a782d6b05
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284318

5 files changed, 65 insertions, 22 deletions

diff --git a/cc/output/gl_renderer.cc b/cc/output/gl_renderer.cc
index b58a4d19e0..9a51d808b0 100644
--- a/cc/output/gl_renderer.cc
+++ b/cc/output/gl_renderer.cc
@@ -63,9 +63,10 @@ namespace {
 class FallbackFence : public ResourceProvider::Fence {
  public:
   explicit FallbackFence(gpu::gles2::GLES2Interface* gl)
-      : gl_(gl), has_passed_(false) {}
+      : gl_(gl), has_passed_(true) {}
 
   // Overridden from ResourceProvider::Fence:
+  virtual void Set() OVERRIDE { has_passed_ = false; }
   virtual bool HasPassed() OVERRIDE {
     if (!has_passed_) {
       has_passed_ = true;
@@ -229,32 +230,60 @@ struct GLRenderer::PendingAsyncReadPixels {
 class GLRenderer::SyncQuery {
  public:
   explicit SyncQuery(gpu::gles2::GLES2Interface* gl)
-      : gl_(gl), query_id_(0u), weak_ptr_factory_(this) {
+      : gl_(gl), query_id_(0u), is_pending_(false), weak_ptr_factory_(this) {
     gl_->GenQueriesEXT(1, &query_id_);
   }
   virtual ~SyncQuery() { gl_->DeleteQueriesEXT(1, &query_id_); }
 
   scoped_refptr<ResourceProvider::Fence> Begin() {
-    DCHECK(!weak_ptr_factory_.HasWeakPtrs() || !IsPending());
+    DCHECK(!IsPending());
     // Invalidate weak pointer held by old fence.
     weak_ptr_factory_.InvalidateWeakPtrs();
-    gl_->BeginQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM, query_id_);
+    // Note: In case the set of drawing commands issued before End() do not
+    // depend on the query, defer BeginQueryEXT call until Set() is called and
+    // query is required.
     return make_scoped_refptr<ResourceProvider::Fence>(
         new Fence(weak_ptr_factory_.GetWeakPtr()));
   }
 
-  void End() { gl_->EndQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM); }
+  void Set() {
+    if (is_pending_)
+      return;
+
+    // Note: BeginQueryEXT on GL_COMMANDS_COMPLETED_CHROMIUM is effectively a
+    // noop relative to GL, so it doesn't matter where it happens but we still
+    // make sure to issue this command when Set() is called (prior to issuing
+    // any drawing commands that depend on query), in case some future extension
+    // can take advantage of this.
+    gl_->BeginQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM, query_id_);
+    is_pending_ = true;
+  }
+
+  void End() {
+    if (!is_pending_)
+      return;
+
+    gl_->EndQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM);
+  }
 
   bool IsPending() {
-    unsigned available = 1;
+    if (!is_pending_)
+      return false;
+
+    unsigned result_available = 1;
     gl_->GetQueryObjectuivEXT(
-        query_id_, GL_QUERY_RESULT_AVAILABLE_EXT, &available);
-    return !available;
+        query_id_, GL_QUERY_RESULT_AVAILABLE_EXT, &result_available);
+    is_pending_ = !result_available;
+    return is_pending_;
   }
 
   void Wait() {
+    if (!is_pending_)
+      return;
+
     unsigned result = 0;
     gl_->GetQueryObjectuivEXT(query_id_, GL_QUERY_RESULT_EXT, &result);
+    is_pending_ = false;
   }
 
  private:
@@ -264,6 +293,10 @@ class GLRenderer::SyncQuery {
         : query_(query) {}
 
     // Overridden from ResourceProvider::Fence:
+    virtual void Set() OVERRIDE {
+      DCHECK(query_);
+      query_->Set();
+    }
     virtual bool HasPassed() OVERRIDE {
       return !query_ || !query_->IsPending();
     }
@@ -278,6 +311,7 @@ class GLRenderer::SyncQuery {
 
   gpu::gles2::GLES2Interface* gl_;
   unsigned query_id_;
+  bool is_pending_;
   base::WeakPtrFactory<SyncQuery> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncQuery);
diff --git a/cc/resources/image_raster_worker_pool.cc b/cc/resources/image_raster_worker_pool.cc
index 007c1ed83f..79cfe37e94 100644
--- a/cc/resources/image_raster_worker_pool.cc
+++ b/cc/resources/image_raster_worker_pool.cc
@@ -155,7 +155,7 @@ void ImageRasterWorkerPool::ReleaseCanvasForRaster(RasterTask* task) {
   // Map/UnmapImageRasterBuffer provides direct access to the memory used by the
   // GPU. Read lock fences are required to ensure that we're not trying to map a
   // resource that is currently in-use by the GPU.
-  resource_provider_->EnableReadLockFences(task->resource()->id(), true);
+  resource_provider_->EnableReadLockFences(task->resource()->id());
 }
 
 void ImageRasterWorkerPool::OnRasterFinished() {
diff --git a/cc/resources/pixel_buffer_raster_worker_pool.cc b/cc/resources/pixel_buffer_raster_worker_pool.cc
index cf18f7510b..860b1fcd08 100644
--- a/cc/resources/pixel_buffer_raster_worker_pool.cc
+++ b/cc/resources/pixel_buffer_raster_worker_pool.cc
@@ -406,7 +406,7 @@ void PixelBufferRasterWorkerPool::CheckForCompletedUploads() {
     // Async set pixels commands are not necessarily processed in-sequence with
     // drawing commands. Read lock fences are required to ensure that async
     // commands don't access the resource while used for drawing.
-    resource_provider_->EnableReadLockFences(task->resource()->id(), true);
+    resource_provider_->EnableReadLockFences(task->resource()->id());
 
     DCHECK(std::find(completed_raster_tasks_.begin(),
                      completed_raster_tasks_.end(),
diff --git a/cc/resources/resource_provider.cc b/cc/resources/resource_provider.cc
index a3cded504a..ff1e65ae8f 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -197,6 +197,7 @@ class QueryFence : public ResourceProvider::Fence {
       : gl_(gl), query_id_(query_id) {}
 
   // Overridden from ResourceProvider::Fence:
+  virtual void Set() OVERRIDE {}
   virtual bool HasPassed() OVERRIDE {
     unsigned available = 1;
     gl_->GetQueryObjectuivEXT(
@@ -232,7 +233,7 @@ ResourceProvider::Resource::Resource()
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
-      enable_read_lock_fences(false),
+      read_lock_fences_enabled(false),
       has_shared_bitmap_id(false),
       allow_overlay(false),
       read_lock_fence(NULL),
@@ -248,7 +249,8 @@ ResourceProvider::Resource::Resource()
       hint(TextureUsageAny),
       type(InvalidType),
       format(RGBA_8888),
-      shared_bitmap(NULL) {}
+      shared_bitmap(NULL) {
+}
 
 ResourceProvider::Resource::~Resource() {}
 
@@ -277,7 +279,7 @@ ResourceProvider::Resource::Resource(GLuint texture_id,
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
-      enable_read_lock_fences(false),
+      read_lock_fences_enabled(false),
       has_shared_bitmap_id(false),
       allow_overlay(false),
       read_lock_fence(NULL),
@@ -320,7 +322,7 @@ ResourceProvider::Resource::Resource(uint8_t* pixels,
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
-      enable_read_lock_fences(false),
+      read_lock_fences_enabled(false),
       has_shared_bitmap_id(!!bitmap),
       allow_overlay(false),
       read_lock_fence(NULL),
@@ -364,7 +366,7 @@ ResourceProvider::Resource::Resource(const SharedBitmapId& bitmap_id,
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
-      enable_read_lock_fences(false),
+      read_lock_fences_enabled(false),
       has_shared_bitmap_id(true),
       allow_overlay(false),
       read_lock_fence(NULL),
@@ -1089,8 +1091,11 @@ const ResourceProvider::Resource* ResourceProvider::LockForRead(ResourceId id) {
   }
 
   resource->lock_for_read_count++;
-  if (resource->enable_read_lock_fences)
+  if (resource->read_lock_fences_enabled) {
+    if (current_read_lock_fence_)
+      current_read_lock_fence_->Set();
     resource->read_lock_fence = current_read_lock_fence_;
+  }
 
   return resource;
 }
@@ -1549,8 +1554,11 @@ void ResourceProvider::ReceiveReturnsFromParent(
 
     // Need to wait for the current read lock fence to pass before we can
     // recycle this resource.
-    if (resource->enable_read_lock_fences)
+    if (resource->read_lock_fences_enabled) {
+      if (current_read_lock_fence_)
+        current_read_lock_fence_->Set();
       resource->read_lock_fence = current_read_lock_fence_;
+    }
 
     if (returned.sync_point) {
       DCHECK(!resource->has_shared_bitmap_id);
@@ -2129,10 +2137,9 @@ void ResourceProvider::BindImageForSampling(Resource* resource) {
   resource->dirty_image = false;
 }
 
-void ResourceProvider::EnableReadLockFences(ResourceProvider::ResourceId id,
-                                            bool enable) {
+void ResourceProvider::EnableReadLockFences(ResourceProvider::ResourceId id) {
   Resource* resource = GetResource(id);
-  resource->enable_read_lock_fences = enable;
+  resource->read_lock_fences_enabled = true;
 }
 
 void ResourceProvider::AcquireImage(Resource* resource) {
diff --git a/cc/resources/resource_provider.h b/cc/resources/resource_provider.h
index 1d991e2b14..cd634483f4 100644
--- a/cc/resources/resource_provider.h
+++ b/cc/resources/resource_provider.h
@@ -309,6 +309,8 @@ class CC_EXPORT ResourceProvider {
   class Fence : public base::RefCounted<Fence> {
    public:
     Fence() {}
+
+    virtual void Set() = 0;
     virtual bool HasPassed() = 0;
 
    protected:
@@ -364,7 +366,7 @@ class CC_EXPORT ResourceProvider {
   void SetReadLockFence(Fence* fence) { current_read_lock_fence_ = fence; }
 
   // Enable read lock fences for a specific resource.
-  void EnableReadLockFences(ResourceProvider::ResourceId id, bool enable);
+  void EnableReadLockFences(ResourceProvider::ResourceId id);
 
   // Indicates if we can currently lock this resource for write.
   bool CanLockForWrite(ResourceId id);
@@ -426,7 +428,7 @@ class CC_EXPORT ResourceProvider {
     bool pending_set_pixels : 1;
     bool set_pixels_completion_forced : 1;
     bool allocated : 1;
-    bool enable_read_lock_fences : 1;
+    bool read_lock_fences_enabled : 1;
     bool has_shared_bitmap_id : 1;
     bool allow_overlay : 1;
     scoped_refptr<Fence> read_lock_fence;