Uber Comp: never change mailbox name associated with a texture object.

Uber Comp: never change mailbox name associated with a texture object.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=166995

[piman, 2012-11-07 00:49:55]

https://codereview.chromium.org/11366123/diff/1/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11366123/diff/1/cc/resource_provider.cc#newco...
cc/resource_provider.cc:184: GLC(context3d,
context3d->genMailboxCHROMIUM(mailbox.name));
I've been thinking about this more, and I'm leaning towards doing this (lazily)
in prepareSendToParent :
1- we don't pay the cost when not sending the resource - meaning, in particular,
we don't pay the cost when not using ubercomp.
2- we can batch the gen since we can know the number of mailboxes we'll need.

https://codereview.chromium.org/11366123/diff/1/cc/resource_provider.cc#newco...
cc/resource_provider.cc:211: GLC(context3d,
context3d->genMailboxCHROMIUM(mailbox.name));
We don't have to do this at all here, since we won't send these resources.

[apatrick_chromium, 2012-11-08 21:36:09]

PTAL. cc_unittests still passes.

https://codereview.chromium.org/11366123/diff/2002/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11366123/diff/2002/cc/resource_provider.cc#ne...
cc/resource_provider.cc:673: GLC(context3d,
context3d->genMailboxCHROMIUM(name));
Counting the needed mailboxes is obviously possible but I think I prefer to have
a pool that asynchronously tries to keep itself filled up to a certain level.

[piman, 2012-11-08 23:56:58]

lgtm

[commit-bot: I haz the power, 2012-11-09 00:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/11366123/2002

[commit-bot: I haz the power, 2012-11-09 00:00:16]

Failed to apply patch for cc/resource_provider.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file cc/resource_provider.cc
  Hunk #1 succeeded at 177 (offset -1 lines).
  Hunk #2 succeeded at 538 (offset -1 lines).
  Hunk #3 succeeded at 620 (offset -1 lines).
  Hunk #4 succeeded at 643 (offset -1 lines).
  Hunk #5 FAILED at 655.
  Hunk #6 succeeded at 665 (offset -1 lines).
  1 out of 6 hunks FAILED -- saving rejects to file cc/resource_provider.cc.rej

Patch:       cc/resource_provider.cc
Index: cc/resource_provider.cc
===================================================================
--- cc/resource_provider.cc	(revision 166046)
+++ cc/resource_provider.cc	(working copy)
@@ -178,6 +178,7 @@
         GLC(context3d, context3d->texStorage2DEXT(GL_TEXTURE_2D, 1,
storageFormat, size.width(), size.height()));
     } else
         GLC(context3d, context3d->texImage2D(GL_TEXTURE_2D, 0, format,
size.width(), size.height(), 0, format, GL_UNSIGNED_BYTE, 0));
+
     ResourceId id = m_nextId++;
     Resource resource(textureId, pool, size, format);
     m_resources[id] = resource;
@@ -538,7 +539,6 @@
     DCHECK(it != m_children.end());
     deleteOwnedResources(it->second.pool);
     m_children.erase(it);
-    trimMailboxDeque();
 }
 
 const ResourceProvider::ResourceIdMap&
ResourceProvider::getChildToParentMap(int child) const
@@ -621,8 +621,8 @@
         GLC(context3d, context3d->consumeTextureCHROMIUM(GL_TEXTURE_2D,
it->mailbox.name));
         ResourceId id = m_nextId++;
         Resource resource(textureId, childInfo.pool, it->size, it->format);
+        resource.mailbox.setName(it->mailbox.name);
         m_resources[id] = resource;
-        m_mailboxes.push_back(it->mailbox);
         childInfo.parentToChildMap[id] = it->id;
         childInfo.childToParentMap[it->id] = id;
     }
@@ -644,9 +644,9 @@
         Resource* resource = &mapIterator->second;
         DCHECK(resource->exported);
         resource->exported = false;
+        resource->mailbox.setName(it->mailbox.name);
         GLC(context3d, context3d->bindTexture(GL_TEXTURE_2D, resource->glId));
         GLC(context3d, context3d->consumeTextureCHROMIUM(GL_TEXTURE_2D,
it->mailbox.name));
-        m_mailboxes.push_back(it->mailbox);
         if (resource->markedForDeletion)
             deleteResourceInternal(mapIterator);
     }
@@ -655,9 +655,10 @@
 bool ResourceProvider::transferResource(WebGraphicsContext3D* context,
ResourceId id, TransferableResource* resource)
 {
     DCHECK(Proxy::isImplThread());
-    ResourceMap::const_iterator it = m_resources.find(id);
+    WebGraphicsContext3D* context3d = m_context->context3D();
+    ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
-    const Resource* source = &it->second;
+    Resource* source = &it->second;
     DCHECK(!source->lockedForWrite);
     DCHECK(!source->lockForReadCount);
     DCHECK(!source->external);
@@ -666,43 +667,19 @@
     resource->id = id;
     resource->format = source->format;
     resource->size = source->size;
-    if (!m_mailboxes.empty()) {
-        resource->mailbox = m_mailboxes.front();
-        m_mailboxes.pop_front();
+
+    if (source->mailbox.isZero()) {
+      GLbyte name[GL_MAILBOX_SIZE_CHROMIUM];
+      GLC(context3d, context3d->genMailboxCHROMIUM(name));
+      source->mailbox.setName(name);
     }
-    else
-        GLC(context, context->genMailboxCHROMIUM(resource->mailbox.name));
+
+    resource->mailbox = source->mailbox;
     GLC(context, context->bindTexture(GL_TEXTURE_2D, source->glId));
     GLC(context, context->produceTextureCHROMIUM(GL_TEXTURE_2D,
resource->mailbox.name));
     return true;
 }
 
-void ResourceProvider::trimMailboxDeque()
-{
-    // Trim the mailbox deque to the maximum number of resources we may need to
-    // send.
-    // If we have a parent, any non-external resource not already transfered is
-    // eligible to be sent to the parent. Otherwise, all resources belonging to
-    // a child might need to be sent back to the child.
-    size_t maxMailboxCount = 0;
-    if (m_context->capabilities().hasParentCompositor) {
-        for (ResourceMap::iterator it = m_resources.begin(); it !=
m_resources.end(); ++it) {
-            if (!it->second.exported && !it->second.external)
-                ++maxMailboxCount;
-        }
-    } else {
-        base::hash_set<int> childPoolSet;
-        for (ChildMap::iterator it = m_children.begin(); it !=
m_children.end(); ++it)
-            childPoolSet.insert(it->second.pool);
-        for (ResourceMap::iterator it = m_resources.begin(); it !=
m_resources.end(); ++it) {
-            if (ContainsKey(childPoolSet, it->second.pool))
-                ++maxMailboxCount;
-        }
-    }
-    while (m_mailboxes.size() > maxMailboxCount)
-        m_mailboxes.pop_front();
-}
-
 void ResourceProvider::debugNotifyEnterZone(unsigned int zone)
 {
     g_debugZone = zone;

[commit-bot: I haz the power, 2012-11-09 19:09:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/11366123/2002

[commit-bot: I haz the power, 2012-11-09 19:09:46]

Failed to apply patch for cc/resource_provider.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file cc/resource_provider.cc
  Hunk #1 succeeded at 177 (offset -1 lines).
  Hunk #2 succeeded at 538 (offset -1 lines).
  Hunk #3 succeeded at 620 (offset -1 lines).
  Hunk #4 succeeded at 643 (offset -1 lines).
  Hunk #5 FAILED at 655.
  Hunk #6 succeeded at 665 (offset -1 lines).
  1 out of 6 hunks FAILED -- saving rejects to file cc/resource_provider.cc.rej

Patch:       cc/resource_provider.cc
Index: cc/resource_provider.cc
===================================================================
--- cc/resource_provider.cc	(revision 166046)
+++ cc/resource_provider.cc	(working copy)
@@ -178,6 +178,7 @@
         GLC(context3d, context3d->texStorage2DEXT(GL_TEXTURE_2D, 1,
storageFormat, size.width(), size.height()));
     } else
         GLC(context3d, context3d->texImage2D(GL_TEXTURE_2D, 0, format,
size.width(), size.height(), 0, format, GL_UNSIGNED_BYTE, 0));
+
     ResourceId id = m_nextId++;
     Resource resource(textureId, pool, size, format);
     m_resources[id] = resource;
@@ -538,7 +539,6 @@
     DCHECK(it != m_children.end());
     deleteOwnedResources(it->second.pool);
     m_children.erase(it);
-    trimMailboxDeque();
 }
 
 const ResourceProvider::ResourceIdMap&
ResourceProvider::getChildToParentMap(int child) const
@@ -621,8 +621,8 @@
         GLC(context3d, context3d->consumeTextureCHROMIUM(GL_TEXTURE_2D,
it->mailbox.name));
         ResourceId id = m_nextId++;
         Resource resource(textureId, childInfo.pool, it->size, it->format);
+        resource.mailbox.setName(it->mailbox.name);
         m_resources[id] = resource;
-        m_mailboxes.push_back(it->mailbox);
         childInfo.parentToChildMap[id] = it->id;
         childInfo.childToParentMap[it->id] = id;
     }
@@ -644,9 +644,9 @@
         Resource* resource = &mapIterator->second;
         DCHECK(resource->exported);
         resource->exported = false;
+        resource->mailbox.setName(it->mailbox.name);
         GLC(context3d, context3d->bindTexture(GL_TEXTURE_2D, resource->glId));
         GLC(context3d, context3d->consumeTextureCHROMIUM(GL_TEXTURE_2D,
it->mailbox.name));
-        m_mailboxes.push_back(it->mailbox);
         if (resource->markedForDeletion)
             deleteResourceInternal(mapIterator);
     }
@@ -655,9 +655,10 @@
 bool ResourceProvider::transferResource(WebGraphicsContext3D* context,
ResourceId id, TransferableResource* resource)
 {
     DCHECK(Proxy::isImplThread());
-    ResourceMap::const_iterator it = m_resources.find(id);
+    WebGraphicsContext3D* context3d = m_context->context3D();
+    ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
-    const Resource* source = &it->second;
+    Resource* source = &it->second;
     DCHECK(!source->lockedForWrite);
     DCHECK(!source->lockForReadCount);
     DCHECK(!source->external);
@@ -666,43 +667,19 @@
     resource->id = id;
     resource->format = source->format;
     resource->size = source->size;
-    if (!m_mailboxes.empty()) {
-        resource->mailbox = m_mailboxes.front();
-        m_mailboxes.pop_front();
+
+    if (source->mailbox.isZero()) {
+      GLbyte name[GL_MAILBOX_SIZE_CHROMIUM];
+      GLC(context3d, context3d->genMailboxCHROMIUM(name));
+      source->mailbox.setName(name);
     }
-    else
-        GLC(context, context->genMailboxCHROMIUM(resource->mailbox.name));
+
+    resource->mailbox = source->mailbox;
     GLC(context, context->bindTexture(GL_TEXTURE_2D, source->glId));
     GLC(context, context->produceTextureCHROMIUM(GL_TEXTURE_2D,
resource->mailbox.name));
     return true;
 }
 
-void ResourceProvider::trimMailboxDeque()
-{
-    // Trim the mailbox deque to the maximum number of resources we may need to
-    // send.
-    // If we have a parent, any non-external resource not already transfered is
-    // eligible to be sent to the parent. Otherwise, all resources belonging to
-    // a child might need to be sent back to the child.
-    size_t maxMailboxCount = 0;
-    if (m_context->capabilities().hasParentCompositor) {
-        for (ResourceMap::iterator it = m_resources.begin(); it !=
m_resources.end(); ++it) {
-            if (!it->second.exported && !it->second.external)
-                ++maxMailboxCount;
-        }
-    } else {
-        base::hash_set<int> childPoolSet;
-        for (ChildMap::iterator it = m_children.begin(); it !=
m_children.end(); ++it)
-            childPoolSet.insert(it->second.pool);
-        for (ResourceMap::iterator it = m_resources.begin(); it !=
m_resources.end(); ++it) {
-            if (ContainsKey(childPoolSet, it->second.pool))
-                ++maxMailboxCount;
-        }
-    }
-    while (m_mailboxes.size() > maxMailboxCount)
-        m_mailboxes.pop_front();
-}
-
 void ResourceProvider::debugNotifyEnterZone(unsigned int zone)
 {
     g_debugZone = zone;