Reland of 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..f37d68bae8c412b5358e45dbadc9bde4ead650a3 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -1150,38 +1150,31 @@ 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;
-  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());
-    }
 
-    ++resources_.find(*it)->second.exported_count;
-    list->push_back(resource);
-  }
-
-  // Fill out unverified sync tokens array.
+  // Lazily create any mailboxes and verify all unverified sync tokens.
   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());
+  std::vector<Resource*> need_synchronization_resources;
+  for (const ResourceId it : resources) {

[vmpstr, 2016/02/08 20:02:56]

nit: can you rename "it" to "id" or something like that here and whenever the
type is ResourceId in this function. 

[David Yen, 2016/02/08 21:49:24]

Done.

+    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_resources.push_back(resource);
+    } else if (resource->mailbox.HasSyncToken() &&
+               !resource->mailbox.sync_token().verified_flush()) {
+      unverified_sync_tokens.push_back(resource->mailbox.GetSyncTokenData());
+    }
   }
 
-  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_resources.empty()) {
     const uint64_t fence_sync = gl->InsertFenceSyncCHROMIUM();
     gl->OrderingBarrierCHROMIUM();
     gl->GenUnverifiedSyncTokenCHROMIUM(fence_sync, new_sync_token.GetData());
@@ -1193,13 +1186,32 @@ 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 (Resource* resource_iter : need_synchronization_resources) {

[vmpstr, 2016/02/08 20:02:55]

I'd prefer if we reserved "iter" and "it" for iterators... I think this can just
be "resource"

[David Yen, 2016/02/08 21:49:24]

Done.

+    resource_iter->mailbox.set_sync_token(new_sync_token);
+  }
+
+  // Transfer Resources
+  for (const ResourceId it : resources) {

[piman, 2016/02/08 21:09:24]

nit: it->id here too.

[David Yen, 2016/02/08 21:49:24]

Done.

+    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;

[vmpstr, 2016/02/08 20:02:55]

nit: usually, we'd do something like

DCHECK(resources_.find(it) != resources_.end());
++resources_[it].exported_count;

I find it easier to read. However, that code was there before, so it's up to
you.

[David Yen, 2016/02/08 21:49:24]

I just tried this but the Resource struct doesn't have the necessary constructor
to do bracket operator it looks like. I'll just keep it as is instead of adding
one for this purpose.

[vmpstr, 2016/02/08 23:36:52]

Ah makes sense. Thanks.

+    list->push_back(resource);
+  }
+
+  // TODO(dyen): This is necessary because we do not guarantee all resource

[David Yen, 2016/02/08 19:24:17]

This surprised me a bit but resources can appear in the resource list twice? In
particular I saw this in the delegated renderer. Is that a bug? I can
investigate if so, otherwise this is how it needs to be done to support
duplicate resources (that or filtering it with a set as a pre-process).

[piman, 2016/02/08 21:09:24]

Nothing really says they shouldn't, and in particular I think UI resources can
be used in multiple layers.

[David Yen, 2016/02/08 21:49:24]

Acknowledged.

+  // updates will insert sync tokens currently. Otherwise there is no way to
+  // know if a sync token is out of date or not. This can also be removed once
+  // proper resource state tracking is completed. Note that this used to be done
+  // in the TransferResource step, but will now fail if a resource is used in
+  // the same resource list twice.
+  for (const ResourceId it : resources) {
+    Resource* resource = GetResource(it);
+    resource->mailbox.set_sync_token(gpu::SyncToken());

[vmpstr, 2016/02/08 20:02:56]

So is this just temporary here? It seems that it's resetting whatever sync token
we set on line 1190 isn't it? (Sorry if it's a naive question, I'm just trying
to understand).

[David Yen, 2016/02/08 21:49:24]

This is the other side of line 1163 where we add missing sync tokens for
resources which do not have any sync tokens. Once the resources are transferred
it no longer needs to be on the source resource. However if the resource is
reused we need to be sure no stale sync tokens are there so we know to fill in
any missing ones.

[vmpstr, 2016/02/08 23:36:52]

Gotcha.

   }
 }
 
@@ -1334,6 +1346,36 @@ 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 +1395,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());
   }
 }