Separate out resource mailbox creation and fix synchronization issue.

cc/resources/resource_provider.cc

Index: cc/resources/resource_provider.cc
diff --git a/cc/resources/resource_provider.cc b/cc/resources/resource_provider.cc
index f2d79922c67d5af46733b9e222473ce2abbb5de9..02505665fffe3db48f59eb1713a7f979deb08f82 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -1150,38 +1150,33 @@ void ResourceProvider::PrepareSendToParent(const ResourceIdArray& resources,
                                            TransferableResourceArray* list) {
   DCHECK(thread_checker_.CalledOnValidThread());
   GLES2Interface* gl = ContextGL();
-  bool need_sync_token = false;
 
-  gpu::SyncToken new_sync_token;
-  std::vector<size_t> unverified_token_indexes;
+  // Lazily create any mailboxes and verify all unverified sync tokens.
+  std::vector<GLbyte*> unverified_sync_tokens;
+  std::vector<ResourceIdArray::const_iterator> need_synchronization_indexes;
   for (ResourceIdArray::const_iterator it = resources.begin();
        it != resources.end();
        ++it) {
-    TransferableResource resource;
-    TransferResource(gl, *it, &resource);
-    need_sync_token |= (!resource.mailbox_holder.sync_token.HasData() &&
-                        !resource.is_software);
-
-    if (resource.mailbox_holder.sync_token.HasData() &&
-        !resource.mailbox_holder.sync_token.verified_flush()) {
-      unverified_token_indexes.push_back(list->size());
+    Resource* resource = GetResource(*it);
+    bool need_synchronization = CreateMailboxAndBindResource(gl, resource);
+
+    // TODO(dyen): Temporarily add missing sync tokens, eventually this should
+    // be removed as we guarantee all resources have associated sync tokens.
+    need_synchronization |= (resource->type != RESOURCE_TYPE_BITMAP &&
+                             !resource->mailbox.HasSyncToken());
+
+    if (output_surface_->capabilities().delegated_sync_points_required &&
+        need_synchronization) {
+      need_synchronization_indexes.push_back(it);

[David Yen, 2016/02/04 19:09:27]

From piman@:
We don't need to create sync tokens if delegated_sync_points_required is not
set. We could either skip the 2 code paths below, or I think simply not add it
to created_mailbox_indexes here.

The other option, I think, is to call CreateResourceMailbox from LazyCreate. I
don't think there's any point in delaying it any more - it used to require a
sync round-trip to the GPU process, so we needed to batch, but now it's all
client-side and a lot cheaper compared to everything else. Also with surfaces
used for the UI, there's very few cases left, if any, where we ever create a
resource that will not be sent to a parent, so I think delaying the mailbox
creation is a false optimization at this point.

[David Yen, 2016/02/04 19:13:29]

Done. Added delegated_sync_points_required check.

I'm fine with leaving the lazy mailbox creation here, because we still need to
synchronize the lazy texture binding code which was not previously synchronized.

+    } else if (resource->mailbox.HasSyncToken() &&
+               !resource->mailbox.sync_token().verified_flush()) {
+      unverified_sync_tokens.push_back(resource->mailbox.GetSyncTokenData());
     }
-
-    ++resources_.find(*it)->second.exported_count;
-    list->push_back(resource);
-  }
-
-  // Fill out unverified sync tokens array.
-  std::vector<GLbyte*> unverified_sync_tokens;
-  unverified_sync_tokens.reserve(unverified_token_indexes.size() + 1);
-  for (auto it = unverified_token_indexes.begin();
-       it != unverified_token_indexes.end(); ++it) {
-    unverified_sync_tokens.push_back(
-        list->at(*it).mailbox_holder.sync_token.GetData());
   }
 
-  if (need_sync_token &&
-      output_surface_->capabilities().delegated_sync_points_required) {
+  // Insert sync point to synchronize the mailbox creation or bound textures.
+  gpu::SyncToken new_sync_token;
+  if (!need_synchronization_indexes.empty()) {
     const uint64_t fence_sync = gl->InsertFenceSyncCHROMIUM();
     gl->OrderingBarrierCHROMIUM();
     gl->GenUnverifiedSyncTokenCHROMIUM(fence_sync, new_sync_token.GetData());
@@ -1193,13 +1188,22 @@ void ResourceProvider::PrepareSendToParent(const ResourceIdArray& resources,
                                  unverified_sync_tokens.size());
   }
 
-  if (new_sync_token.HasData()) {
-    for (TransferableResourceArray::iterator it = list->begin();
-         it != list->end();
-         ++it) {
-      if (!it->mailbox_holder.sync_token.HasData())
-        it->mailbox_holder.sync_token = new_sync_token;
-    }
+  for (ResourceIdArray::const_iterator it : need_synchronization_indexes) {
+    GetResource(*it)->mailbox.set_sync_token(new_sync_token);
+  }
+
+  // Transfer Resources
+  for (ResourceIdArray::const_iterator it = resources.begin();
+       it != resources.end(); ++it) {
+    TransferableResource resource;
+    TransferResource(gl, *it, &resource);
+
+    DCHECK(!output_surface_->capabilities().delegated_sync_points_required ||
+           resource.mailbox_holder.sync_token.HasData() ||
+           resource.is_software);
+
+    ++resources_.find(*it)->second.exported_count;
+    list->push_back(resource);
   }
 }
 
@@ -1334,6 +1338,35 @@ void ResourceProvider::ReceiveReturnsFromParent(
   }
 }
 
+bool ResourceProvider::CreateMailboxAndBindResource(
+    gpu::gles2::GLES2Interface* gl, Resource* resource) {
+  if (resource->type != RESOURCE_TYPE_BITMAP) {
+    bool did_create_or_bind = false;
+    if (!resource->mailbox.IsValid()) {
+      LazyCreate(resource);
+
+      gpu::MailboxHolder mailbox_holder;
+      mailbox_holder.texture_target = resource->target;
+      gl->GenMailboxCHROMIUM(mailbox_holder.mailbox.name);
+      gl->ProduceTextureDirectCHROMIUM(resource->gl_id,
+                                       mailbox_holder.texture_target,
+                                       mailbox_holder.mailbox.name);
+      resource->mailbox = TextureMailbox(mailbox_holder);
+      did_create_or_bind = true;
+    }
+
+    if (resource->image_id && resource->dirty_image) {
+      DCHECK(resource->gl_id);
+      DCHECK(resource->origin == Resource::INTERNAL);
+      BindImageForSampling(resource);
+      did_create_or_bind = true;
+    }
+
+    return did_create_or_bind;
+  }
+  return false;
+}
+
 void ResourceProvider::TransferResource(GLES2Interface* gl,
                                         ResourceId id,
                                         TransferableResource* resource) {
@@ -1353,33 +1386,14 @@ void ResourceProvider::TransferResource(GLES2Interface* gl,
   if (source->type == RESOURCE_TYPE_BITMAP) {
     resource->mailbox_holder.mailbox = source->shared_bitmap_id;
     resource->is_software = true;
-  } else if (!source->mailbox.IsValid()) {
-    LazyCreate(source);
-    DCHECK(source->gl_id);
-    DCHECK(source->origin == Resource::INTERNAL);
-    if (source->image_id && source->dirty_image)
-      BindImageForSampling(source);
-    // This is a resource allocated by the compositor, we need to produce it.
-    // Don't set a sync point, the caller will do it.
-    gl->GenMailboxCHROMIUM(resource->mailbox_holder.mailbox.name);
-    gl->ProduceTextureDirectCHROMIUM(source->gl_id,
-                                     resource->mailbox_holder.texture_target,
-                                     resource->mailbox_holder.mailbox.name);
-
-    source->mailbox = TextureMailbox(resource->mailbox_holder);
   } else {
+    DCHECK(source->mailbox.IsValid());
     DCHECK(source->mailbox.IsTexture());
-    if (source->image_id && source->dirty_image) {
-      DCHECK(source->gl_id);
-      DCHECK(source->origin == Resource::INTERNAL);
-      BindImageForSampling(source);
-    }
     // This is either an external resource, or a compositor resource that we
     // already exported. Make sure to forward the sync point that we were given.
     resource->mailbox_holder.mailbox = source->mailbox.mailbox();
     resource->mailbox_holder.texture_target = source->mailbox.target();
     resource->mailbox_holder.sync_token = source->mailbox.sync_token();
-    source->mailbox.set_sync_token(gpu::SyncToken());
   }
 }