Update texture outside of lock

The glEGLImageTargetTexture2DOES call is expensive on some
drivers. Move these calls in PullTextureUpdates outside of
the global lock to reduce lock contention.

Also change makes glFlush to onces per PullTextureUpdates
instead of once per texture update.

BUG=492315

Review URL: https://codereview.chromium.org/1153313003

Cr-Commit-Position: refs/heads/master@{#331874}

4 files changed, 72 insertions, 27 deletions

diff --git a/gpu/command_buffer/service/mailbox_manager_sync.cc b/gpu/command_buffer/service/mailbox_manager_sync.cc
index 4cdc80d..00c9abaa 100644
--- a/gpu/command_buffer/service/mailbox_manager_sync.cc
+++ b/gpu/command_buffer/service/mailbox_manager_sync.cc
@@ -312,19 +312,29 @@ void MailboxManagerSync::PushTextureUpdates(uint32 sync_point) {
 }
 
 void MailboxManagerSync::PullTextureUpdates(uint32 sync_point) {
-  base::AutoLock lock(g_lock.Get());
-  AcquireFenceLocked(sync_point);
+  using TextureUpdatePair = std::pair<Texture*, TextureDefinition>;
+  std::vector<TextureUpdatePair> needs_update;
+  {
+    base::AutoLock lock(g_lock.Get());
+    AcquireFenceLocked(sync_point);
+
+    for (TextureToGroupMap::iterator it = texture_to_group_.begin();
+         it != texture_to_group_.end(); it++) {
+      const TextureDefinition& definition = it->second.group->GetDefinition();
+      Texture* texture = it->first;
+      unsigned& texture_version = it->second.version;
+      if (texture_version == definition.version() ||
+          definition.IsOlderThan(texture_version))
+        continue;
+      needs_update.push_back(TextureUpdatePair(texture, definition));
+    }
+  }
 
-  for (TextureToGroupMap::iterator it = texture_to_group_.begin();
-       it != texture_to_group_.end(); it++) {
-    const TextureDefinition& definition = it->second.group->GetDefinition();
-    Texture* texture = it->first;
-    unsigned& texture_version = it->second.version;
-    if (texture_version == definition.version() ||
-        definition.IsOlderThan(texture_version))
-      continue;
-    texture_version = definition.version();
-    definition.UpdateTexture(texture);
+  if (!needs_update.empty()) {
+    ScopedUpdateTexture scoped_update_texture;
+    for (const TextureUpdatePair& pair : needs_update) {
+      pair.second.UpdateTexture(pair.first);
+    }
   }
 }
 
diff --git a/gpu/command_buffer/service/mailbox_manager_unittest.cc b/gpu/command_buffer/service/mailbox_manager_unittest.cc
index 06c0f22..0e064d4 100644
--- a/gpu/command_buffer/service/mailbox_manager_unittest.cc
+++ b/gpu/command_buffer/service/mailbox_manager_unittest.cc
@@ -254,10 +254,10 @@ class MailboxManagerSyncTest : public MailboxManagerTest {
     EXPECT_CALL(*gl_, TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, wrap_t))
         .Times(1)
         .RetiresOnSaturation();
-    EXPECT_CALL(*gl_, Flush())
+    EXPECT_CALL(*gl_, BindTexture(GL_TEXTURE_2D, kCurrentTexture))
         .Times(1)
         .RetiresOnSaturation();
-    EXPECT_CALL(*gl_, BindTexture(GL_TEXTURE_2D, kCurrentTexture))
+    EXPECT_CALL(*gl_, Flush())
         .Times(1)
         .RetiresOnSaturation();
   }
diff --git a/gpu/command_buffer/service/texture_definition.cc b/gpu/command_buffer/service/texture_definition.cc
index d4c474b..7b38f86 100644
--- a/gpu/command_buffer/service/texture_definition.cc
+++ b/gpu/command_buffer/service/texture_definition.cc
@@ -6,9 +6,11 @@
 
 #include <list>
 
+#include "base/lazy_instance.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread_local.h"
 #include "gpu/command_buffer/service/texture_manager.h"
 #include "ui/gl/gl_image.h"
 #include "ui/gl/gl_implementation.h"
@@ -127,10 +129,10 @@ class NativeImageBufferEGL : public NativeImageBuffer {
   void AddClient(gfx::GLImage* client) override;
   void RemoveClient(gfx::GLImage* client) override;
   bool IsClient(gfx::GLImage* client) override;
-  void BindToTexture(GLenum target) override;
+  void BindToTexture(GLenum target) const override;
 
-  EGLDisplay egl_display_;
-  EGLImageKHR egl_image_;
+  const EGLDisplay egl_display_;
+  const EGLImageKHR egl_image_;
 
   base::Lock lock_;
 
@@ -229,7 +231,7 @@ bool NativeImageBufferEGL::IsClient(gfx::GLImage* client) {
   return false;
 }
 
-void NativeImageBufferEGL::BindToTexture(GLenum target) {
+void NativeImageBufferEGL::BindToTexture(GLenum target) const {
   DCHECK(egl_image_ != EGL_NO_IMAGE_KHR);
   glEGLImageTargetTexture2DOES(target, egl_image_);
   DCHECK_EQ(static_cast<EGLint>(EGL_SUCCESS), eglGetError());
@@ -247,13 +249,17 @@ class NativeImageBufferStub : public NativeImageBuffer {
   void AddClient(gfx::GLImage* client) override {}
   void RemoveClient(gfx::GLImage* client) override {}
   bool IsClient(gfx::GLImage* client) override { return true; }
-  void BindToTexture(GLenum target) override {}
+  void BindToTexture(GLenum target) const override {}
 
   DISALLOW_COPY_AND_ASSIGN(NativeImageBufferStub);
 };
 
 bool g_avoid_egl_target_texture_reuse = false;
 
+#if DCHECK_IS_ON()
+base::LazyInstance<base::ThreadLocalBoolean> g_inside_scoped_update_texture;
+#endif
+
 }  // anonymous namespace
 
 // static
@@ -276,6 +282,27 @@ void TextureDefinition::AvoidEGLTargetTextureReuse() {
   g_avoid_egl_target_texture_reuse = true;
 }
 
+ScopedUpdateTexture::ScopedUpdateTexture() {
+#if DCHECK_IS_ON()
+  DCHECK(!g_inside_scoped_update_texture.Get().Get());
+  g_inside_scoped_update_texture.Get().Set(true);
+#endif
+}
+
+ScopedUpdateTexture::~ScopedUpdateTexture() {
+#if DCHECK_IS_ON()
+  DCHECK(g_inside_scoped_update_texture.Get().Get());
+  g_inside_scoped_update_texture.Get().Set(false);
+#endif
+  // We have to make sure the changes are visible to other clients in this share
+  // group. As far as the clients are concerned, the mailbox semantics only
+  // demand a single flush from the client after changes are first made,
+  // and it is not visible to them when another share group boundary is crossed.
+  // We could probably track this and be a bit smarter about when to flush
+  // though.
+  glFlush();
+}
+
 TextureDefinition::LevelInfo::LevelInfo()
     : target(0),
       internal_format(0),
@@ -363,12 +390,16 @@ Texture* TextureDefinition::CreateTexture() const {
   glGenTextures(1, &texture_id);
 
   Texture* texture(new Texture(texture_id));
+  ScopedUpdateTexture scoped_update_texture;
   UpdateTextureInternal(texture);
 
   return texture;
 }
 
 void TextureDefinition::UpdateTextureInternal(Texture* texture) const {
+#if DCHECK_IS_ON()
+  DCHECK(g_inside_scoped_update_texture.Get().Get());
+#endif
   gfx::ScopedTextureBinder texture_binder(target_, texture->service_id());
   glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, min_filter_);
   glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, mag_filter_);
@@ -376,13 +407,6 @@ void TextureDefinition::UpdateTextureInternal(Texture* texture) const {
   glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, wrap_t_);
   if (image_buffer_.get())
     image_buffer_->BindToTexture(target_);
-  // We have to make sure the changes are visible to other clients in this share
-  // group. As far as the clients are concerned, the mailbox semantics only
-  // demand a single flush from the client after changes are first made,
-  // and it is not visible to them when another share group boundary is crossed.
-  // We could probably track this and be a bit smarter about when to flush
-  // though.
-  glFlush();
 
   if (defined_) {
     texture->face_infos_.resize(1);
diff --git a/gpu/command_buffer/service/texture_definition.h b/gpu/command_buffer/service/texture_definition.h
index 65df72f..e531184 100644
--- a/gpu/command_buffer/service/texture_definition.h
+++ b/gpu/command_buffer/service/texture_definition.h
@@ -26,7 +26,7 @@ class NativeImageBuffer : public base::RefCountedThreadSafe<NativeImageBuffer> {
   virtual void AddClient(gfx::GLImage* client) = 0;
   virtual void RemoveClient(gfx::GLImage* client) = 0;
   virtual bool IsClient(gfx::GLImage* client) = 0;
-  virtual void BindToTexture(GLenum target) = 0;
+  virtual void BindToTexture(GLenum target) const = 0;
 
  protected:
   friend class base::RefCountedThreadSafe<NativeImageBuffer>;
@@ -36,6 +36,15 @@ class NativeImageBuffer : public base::RefCountedThreadSafe<NativeImageBuffer> {
   DISALLOW_COPY_AND_ASSIGN(NativeImageBuffer);
 };
 
+class ScopedUpdateTexture {
+ public:
+  ScopedUpdateTexture();
+  ~ScopedUpdateTexture();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScopedUpdateTexture);
+};
+
 // An immutable description that can be used to create a texture that shares
 // the underlying image buffer(s).
 class TextureDefinition {
@@ -49,6 +58,8 @@ class TextureDefinition {
   virtual ~TextureDefinition();
 
   Texture* CreateTexture() const;
+
+  // Must be wrapped with ScopedUpdateTexture.
   void UpdateTexture(Texture* texture) const;
 
   unsigned int version() const { return version_; }