Simplify MailboxManager implementations

After https://crrev.com/c/1089695, while using ProduceTextureDirect, it
is not possible any more for a client to:
1- reassign an existing mailbox to another texture
2- unlink a mailbox from a texture

TakeFrontBuffer can technically still do (1), but ReturnFrontBuffer is a
good place to unlink the mailbox service-side.
This allows some simplification of the MailboxManager implementations,
as well as the decoders.

Bug: 847674
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I422b7ba8bcb762d1a5aea63f59dc833002f3d6b3
Reviewed-on: https://chromium-review.googlesource.com/1097696
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566671}

gpu/command_buffer/service/gles2_cmd_decoder.cc

@@ -1128,10 +1128,6 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
 
   void DoProduceTextureDirectCHROMIUM(GLuint texture,
                                       const volatile GLbyte* key);
-  void ProduceTextureRef(const char* func_name,
-                         bool clear,
-                         TextureRef* texture_ref,
-                         const volatile GLbyte* data);
 
   void DoCreateAndConsumeTextureINTERNAL(GLuint client_id,
                                          const volatile GLbyte* key);
@@ -5274,8 +5270,8 @@ void GLES2DecoderImpl::TakeFrontBuffer(const Mailbox& mailbox) {
 }
 
 void GLES2DecoderImpl::ReturnFrontBuffer(const Mailbox& mailbox, bool is_lost) {
-  Texture* texture =
+  TextureBase* texture = mailbox_manager()->ConsumeTexture(mailbox);
-      static_cast<Texture*>(group_->mailbox_manager()->ConsumeTexture(mailbox));
+  mailbox_manager()->TextureDeleted(texture);
 
   if (offscreen_single_buffer_)
     return;
@@ -18108,40 +18104,19 @@ void GLES2DecoderImpl::DoProduceTextureDirectCHROMIUM(
   TRACE_EVENT2("gpu", "GLES2DecoderImpl::DoProduceTextureDirectCHROMIUM",
       "context", logger_.GetLogPrefix(),
       "mailbox[0]", static_cast<unsigned char>(data[0]));
-
-  ProduceTextureRef("glProduceTextureDirectCHROMIUM", !client_id,
-                    GetTexture(client_id), data);
-}
-
-void GLES2DecoderImpl::ProduceTextureRef(const char* func_name,
-                                         bool clear,
-                                         TextureRef* texture_ref,
-                                         const volatile GLbyte* data) {
   Mailbox mailbox =
       Mailbox::FromVolatile(*reinterpret_cast<const volatile Mailbox*>(data));
   DLOG_IF(ERROR, !mailbox.Verify())
       << "ProduceTextureDirectCHROMIUM was not passed a crypto-random mailbox.";
 
-  if (clear) {
+  TextureRef* texture_ref = GetTexture(client_id);
-    DCHECK(!texture_ref);
-
-    group_->mailbox_manager()->ProduceTexture(mailbox, nullptr);
-    return;
-  }
-
   if (!texture_ref) {
-    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name, "unknown texture");
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glProduceTextureDirectCHROMIUM",
+                       "unknown texture");
     return;
   }
 
-  Texture* produced = texture_manager()->Produce(texture_ref);
+  group_->mailbox_manager()->ProduceTexture(mailbox, texture_ref->texture());
-  if (!produced) {
-    LOCAL_SET_GL_ERROR(
-        GL_INVALID_OPERATION, func_name, "invalid texture");
-    return;
-  }
-
-  group_->mailbox_manager()->ProduceTexture(mailbox, produced);
 }
 
 void GLES2DecoderImpl::DoCreateAndConsumeTextureINTERNAL(

gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc

@@ -1009,8 +1009,8 @@ void GLES2DecoderPassthroughImpl::TakeFrontBuffer(const Mailbox& mailbox) {
 
 void GLES2DecoderPassthroughImpl::ReturnFrontBuffer(const Mailbox& mailbox,
                                                     bool is_lost) {
-  TexturePassthrough* texture = static_cast<TexturePassthrough*>(
+  TextureBase* texture = mailbox_manager_->ConsumeTexture(mailbox);
-      mailbox_manager_->ConsumeTexture(mailbox));
+  mailbox_manager_->TextureDeleted(texture);
 
   if (offscreen_single_buffer_) {
     return;

gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc

@@ -3098,81 +3098,6 @@ TEST_P(GLES2DecoderTest, TextureUsageAngleExtNotEnabledByDefault) {
   EXPECT_EQ(GL_INVALID_ENUM, GetGLError());
 }
 
-TEST_P(GLES2DecoderTest, ProduceAndConsumeTextureCHROMIUM) {
-  Mailbox mailbox = Mailbox::Generate();
-  GLuint new_texture_id = kNewClientId;
-
-  DoBindTexture(GL_TEXTURE_2D, client_texture_id_, kServiceTextureId);
-  DoTexImage2D(
-      GL_TEXTURE_2D, 0, GL_RGBA, 3, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, 0, 0);
-  DoTexImage2D(
-      GL_TEXTURE_2D, 1, GL_RGBA, 2, 4, 0, GL_RGBA, GL_UNSIGNED_BYTE, 0, 0);
-  TextureRef* texture_ref =
-      group().texture_manager()->GetTexture(client_texture_id_);
-  ASSERT_TRUE(texture_ref != nullptr);
-  Texture* texture = texture_ref->texture();
-  EXPECT_EQ(kServiceTextureId, texture->service_id());
-
-  ProduceTextureDirectCHROMIUMImmediate& produce_cmd =
-      *GetImmediateAs<ProduceTextureDirectCHROMIUMImmediate>();
-  produce_cmd.Init(client_texture_id_, mailbox.name);
-  EXPECT_EQ(error::kNoError,
-            ExecuteImmediateCmd(produce_cmd, sizeof(mailbox.name)));
-  EXPECT_EQ(GL_NO_ERROR, GetGLError());
-
-  // Texture didn't change.
-  GLsizei width;
-  GLsizei height;
-  GLenum type;
-  GLenum internal_format;
-
-  EXPECT_TRUE(
-      texture->GetLevelSize(GL_TEXTURE_2D, 0, &width, &height, nullptr));
-  EXPECT_EQ(3, width);
-  EXPECT_EQ(1, height);
-  EXPECT_TRUE(texture->GetLevelType(GL_TEXTURE_2D, 0, &type, &internal_format));
-  EXPECT_EQ(static_cast<GLenum>(GL_RGBA), internal_format);
-  EXPECT_EQ(static_cast<GLenum>(GL_UNSIGNED_BYTE), type);
-
-  EXPECT_TRUE(
-      texture->GetLevelSize(GL_TEXTURE_2D, 1, &width, &height, nullptr));
-  EXPECT_EQ(2, width);
-  EXPECT_EQ(4, height);
-  EXPECT_TRUE(texture->GetLevelType(GL_TEXTURE_2D, 1, &type, &internal_format));
-  EXPECT_EQ(static_cast<GLenum>(GL_RGBA), internal_format);
-  EXPECT_EQ(static_cast<GLenum>(GL_UNSIGNED_BYTE), type);
-
-  // Service ID has not changed.
-  EXPECT_EQ(kServiceTextureId, texture->service_id());
-
-  CreateAndConsumeTextureINTERNALImmediate& consume_cmd =
-      *GetImmediateAs<CreateAndConsumeTextureINTERNALImmediate>();
-  consume_cmd.Init(new_texture_id, mailbox.name);
-  EXPECT_EQ(error::kNoError,
-            ExecuteImmediateCmd(consume_cmd, sizeof(mailbox.name)));
-  EXPECT_EQ(GL_NO_ERROR, GetGLError());
-
-  // Texture is redefined.
-  EXPECT_TRUE(
-      texture->GetLevelSize(GL_TEXTURE_2D, 0, &width, &height, nullptr));
-  EXPECT_EQ(3, width);
-  EXPECT_EQ(1, height);
-  EXPECT_TRUE(texture->GetLevelType(GL_TEXTURE_2D, 0, &type, &internal_format));
-  EXPECT_EQ(static_cast<GLenum>(GL_RGBA), internal_format);
-  EXPECT_EQ(static_cast<GLenum>(GL_UNSIGNED_BYTE), type);
-
-  EXPECT_TRUE(
-      texture->GetLevelSize(GL_TEXTURE_2D, 1, &width, &height, nullptr));
-  EXPECT_EQ(2, width);
-  EXPECT_EQ(4, height);
-  EXPECT_TRUE(texture->GetLevelType(GL_TEXTURE_2D, 1, &type, &internal_format));
-  EXPECT_EQ(static_cast<GLenum>(GL_RGBA), internal_format);
-  EXPECT_EQ(static_cast<GLenum>(GL_UNSIGNED_BYTE), type);
-
-  // Service ID is restored.
-  EXPECT_EQ(kServiceTextureId, texture->service_id());
-}
-
 TEST_P(GLES2DecoderTest, ProduceAndConsumeDirectTextureCHROMIUM) {
   Mailbox mailbox = Mailbox::Generate();
 

gpu/command_buffer/service/mailbox_manager_impl.cc

@@ -30,25 +30,17 @@ TextureBase* MailboxManagerImpl::ConsumeTexture(const Mailbox& mailbox) {
   if (it != mailbox_to_textures_.end())
     return it->second->first;
 
-  return NULL;
+  return nullptr;
 }
 
 void MailboxManagerImpl::ProduceTexture(const Mailbox& mailbox,
                                         TextureBase* texture) {
+  DCHECK(texture);
   MailboxToTextureMap::iterator it = mailbox_to_textures_.find(mailbox);
   if (it != mailbox_to_textures_.end()) {
-    if (it->second->first == texture)
+    DLOG(ERROR) << "Ignored attempt to reassign a mailbox";
-      return;
+    return;
-    TextureToMailboxMap::iterator texture_it = it->second;
-    mailbox_to_textures_.erase(it);
-    textures_to_mailboxes_.erase(texture_it);
   }
-  if (texture)
-    InsertTexture(mailbox, texture);
-}
-
-void MailboxManagerImpl::InsertTexture(const Mailbox& mailbox,
-                                       TextureBase* texture) {
   texture->SetMailboxManager(this);
   TextureToMailboxMap::iterator texture_it =
       textures_to_mailboxes_.insert(std::make_pair(texture, mailbox));

gpu/command_buffer/service/mailbox_manager_impl.h

@@ -33,8 +33,6 @@ class GPU_GLES2_EXPORT MailboxManagerImpl : public MailboxManager {
   void TextureDeleted(TextureBase* texture) override;
 
  private:
-  void InsertTexture(const Mailbox& mailbox, TextureBase* texture);
-
   // This is a bidirectional map between mailbox and textures. We can have
   // multiple mailboxes per texture, but one texture per mailbox. We keep an
   // iterator in the MailboxToTextureMap to be able to manage changes to

gpu/command_buffer/service/mailbox_manager_sync.cc

@@ -222,37 +222,25 @@ Texture* MailboxManagerSync::ConsumeTexture(const Mailbox& mailbox) {
 
 void MailboxManagerSync::ProduceTexture(const Mailbox& mailbox,
                                         TextureBase* texture_base) {
+  DCHECK(texture_base);
   base::AutoLock lock(g_lock.Get());
   // Relax the cross-thread access restriction to non-thread-safe RefCount.
   // The lock above protects non-thread-safe RefCount in TextureGroup.
   base::ScopedAllowCrossThreadRefCountAccess
       scoped_allow_cross_thread_ref_count_access;
+  if (TextureGroup::FromName(mailbox)) {
+    DLOG(ERROR) << "Ignored attempt to reassign a mailbox";
+    return;
+  }
 
   Texture* texture = static_cast<Texture*>(texture_base);
 
   TextureToGroupMap::iterator tex_it = texture_to_group_.find(texture);
-  TextureGroup* group_for_mailbox = TextureGroup::FromName(mailbox);
+  TextureGroup* group_for_texture = nullptr;
-  TextureGroup* group_for_texture = NULL;
 
   if (tex_it != texture_to_group_.end()) {
     group_for_texture = tex_it->second.group.get();
     DCHECK(group_for_texture);
-    if (group_for_mailbox == group_for_texture) {
-      // The texture is already known under this name.
-      return;
-    }
-  }
-
-  if (group_for_mailbox) {
-    // Unlink the mailbox from its current group.
-    group_for_mailbox->RemoveName(mailbox);
-  }
-
-  if (!texture)
-    return;
-
-  if (group_for_texture) {
-    group_for_texture->AddName(mailbox);
   } else {
     // This is a new texture, so create a new group.
     texture->SetMailboxManager(this);
@@ -263,10 +251,10 @@ void MailboxManagerSync::ProduceTexture(const Mailbox& mailbox,
     }
     group_for_texture = new TextureGroup(definition);
     group_for_texture->AddTexture(this, texture);
-    group_for_texture->AddName(mailbox);
     texture_to_group_.insert(std::make_pair(
         texture, TextureGroupRef(kNewTextureVersion, group_for_texture)));
   }
+  group_for_texture->AddName(mailbox);
 
   DCHECK(texture->mailbox_manager() == this);
 }

gpu/command_buffer/service/mailbox_manager_unittest.cc

@@ -143,43 +143,17 @@ TEST_F(MailboxManagerTest, ProduceMultipleTexture) {
   manager_->ProduceTexture(name, texture1);
   EXPECT_EQ(texture1, manager_->ConsumeTexture(name));
 
-  // Can produce a second time with the same mailbox, but different texture.
+  // Producing a second time is ignored.
   manager_->ProduceTexture(name, texture2);
-  EXPECT_EQ(texture2, manager_->ConsumeTexture(name));
+  EXPECT_EQ(texture1, manager_->ConsumeTexture(name));
 
   // Destroying the texture that's under no mailbox shouldn't have an effect.
-  DestroyTexture(texture1);
+  DestroyTexture(texture2);
-  EXPECT_EQ(texture2, manager_->ConsumeTexture(name));
+  EXPECT_EQ(texture1, manager_->ConsumeTexture(name));
 
   // Destroying the texture that's bound should clean up.
-  DestroyTexture(texture2);
-  EXPECT_EQ(NULL, manager_->ConsumeTexture(name));
-}
-
-TEST_F(MailboxManagerTest, ProduceMultipleTextureMailbox) {
-  Texture* texture1 = CreateTexture();
-  Texture* texture2 = CreateTexture();
-  Mailbox name1 = Mailbox::Generate();
-  Mailbox name2 = Mailbox::Generate();
-
-  // Put texture1 on name1 and name2.
-  manager_->ProduceTexture(name1, texture1);
-  manager_->ProduceTexture(name2, texture1);
-  EXPECT_EQ(texture1, manager_->ConsumeTexture(name1));
-  EXPECT_EQ(texture1, manager_->ConsumeTexture(name2));
-
-  // Put texture2 on name2.
-  manager_->ProduceTexture(name2, texture2);
-  EXPECT_EQ(texture1, manager_->ConsumeTexture(name1));
-  EXPECT_EQ(texture2, manager_->ConsumeTexture(name2));
-
-  // Destroy texture1, shouldn't affect name2.
   DestroyTexture(texture1);
-  EXPECT_EQ(NULL, manager_->ConsumeTexture(name1));
+  EXPECT_EQ(NULL, manager_->ConsumeTexture(name));
-  EXPECT_EQ(texture2, manager_->ConsumeTexture(name2));
-
-  DestroyTexture(texture2);
-  EXPECT_EQ(NULL, manager_->ConsumeTexture(name2));
 }
 
 const GLsizei kMaxTextureWidth = 64;
@@ -288,7 +262,7 @@ TEST_F(MailboxManagerSyncTest, ProduceSyncDestroy) {
   EXPECT_EQ(NULL, manager2_->ConsumeTexture(name));
 }
 
-TEST_F(MailboxManagerSyncTest, ProduceSyncClobberDestroy) {
+TEST_F(MailboxManagerSyncTest, ProduceSyncMultipleMailbox) {
   InSequence sequence;
 
   Texture* texture = DefineTexture();
@@ -297,10 +271,11 @@ TEST_F(MailboxManagerSyncTest, ProduceSyncClobberDestroy) {
   manager_->ProduceTexture(name, texture);
   manager_->PushTextureUpdates(g_sync_token);
 
-  // Clobber
+  // Producing a second time with the same mailbox is ignored.
   Texture* old_texture = texture;
   texture = DefineTexture();
   manager_->ProduceTexture(name, texture);
+  EXPECT_EQ(old_texture, manager_->ConsumeTexture(name));
 
   DestroyTexture(old_texture);
   DestroyTexture(texture);
@@ -447,73 +422,6 @@ TEST_F(MailboxManagerSyncTest, ProduceConsumeBidirectional) {
   DestroyTexture(new_texture2);
 }
 
-// If a texture is shared with another manager instance, but the mailbox
-// is then clobbered with a different texture in the source context, this should
-// disconnect the earlier texture from updates.
-TEST_F(MailboxManagerSyncTest, ProduceAndClobber) {
-  const GLuint kNewTextureId = 1234;
-  InSequence sequence;
-
-  Texture* texture = DefineTexture();
-  Mailbox name = Mailbox::Generate();
-
-  manager_->ProduceTexture(name, texture);
-  EXPECT_EQ(texture, manager_->ConsumeTexture(name));
-
-  // Synchronize
-  manager_->PushTextureUpdates(g_sync_token);
-  manager2_->PullTextureUpdates(g_sync_token);
-
-  EXPECT_CALL(*gl_, GenTextures(1, _))
-      .WillOnce(SetArgPointee<1>(kNewTextureId));
-  SetupUpdateTexParamExpectations(
-      kNewTextureId, GL_LINEAR, GL_LINEAR, GL_REPEAT, GL_REPEAT);
-  Texture* new_texture = static_cast<Texture*>(manager2_->ConsumeTexture(name));
-  EXPECT_NE(nullptr, new_texture);
-  EXPECT_NE(texture, new_texture);
-  EXPECT_EQ(kNewTextureId, new_texture->service_id());
-
-  Texture* old_texture = texture;
-  texture = DefineTexture();
-  manager_->ProduceTexture(name, texture);
-
-  // Make a change to the new texture
-  DCHECK_EQ(static_cast<GLuint>(GL_LINEAR), texture->min_filter());
-  EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR),
-            SetParameter(texture, GL_TEXTURE_MIN_FILTER, GL_NEAREST));
-
-  // Synchronize in both directions - no changes, since it's not shared
-  manager_->PushTextureUpdates(g_sync_token);
-  manager2_->PullTextureUpdates(g_sync_token);
-  EXPECT_EQ(static_cast<GLuint>(GL_LINEAR), new_texture->min_filter());
-
-  // Make a change to the previously shared texture
-  DCHECK_EQ(static_cast<GLuint>(GL_LINEAR), old_texture->mag_filter());
-  EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR),
-            SetParameter(old_texture, GL_TEXTURE_MAG_FILTER, GL_NEAREST));
-
-  // Synchronize and expect update
-  manager_->PushTextureUpdates(g_sync_token);
-  SetupUpdateTexParamExpectations(
-      new_texture->service_id(), GL_LINEAR, GL_NEAREST, GL_REPEAT, GL_REPEAT);
-  manager2_->PullTextureUpdates(g_sync_token);
-
-  EXPECT_CALL(*gl_, GenTextures(1, _))
-      .WillOnce(SetArgPointee<1>(kNewTextureId));
-  SetupUpdateTexParamExpectations(
-      kNewTextureId, GL_NEAREST, GL_LINEAR, GL_REPEAT, GL_REPEAT);
-  TextureBase* tmp_texture = manager2_->ConsumeTexture(name);
-  EXPECT_NE(new_texture, tmp_texture);
-  DestroyTexture(tmp_texture);
-
-  DestroyTexture(old_texture);
-  DestroyTexture(texture);
-  DestroyTexture(new_texture);
-
-  EXPECT_EQ(NULL, manager_->ConsumeTexture(name));
-  EXPECT_EQ(NULL, manager2_->ConsumeTexture(name));
-}
-
 TEST_F(MailboxManagerSyncTest, ClearedStateSynced) {
   const GLuint kNewTextureId = 1234;
 

gpu/command_buffer/service/raster_decoder.cc

@@ -2284,34 +2284,17 @@ void RasterDecoderImpl::DoProduceTextureDirect(GLuint client_id,
 
   Mailbox mailbox =
       Mailbox::FromVolatile(*reinterpret_cast<const volatile Mailbox*>(key));
-  DLOG_IF(ERROR, !mailbox.Verify()) << "ProduceTextureDirect was passed a "
+  DLOG_IF(ERROR, !mailbox.Verify())
-                                       "mailbox that was not generated by "
+      << "ProduceTextureDirect was not passed a crypto-random mailbox.";
-                                       "GenMailboxCHROMIUM.";
 
   TextureRef* texture_ref = GetTexture(client_id);
-
-  bool clear = !client_id;
-  if (clear) {
-    DCHECK(!texture_ref);
-
-    group_->mailbox_manager()->ProduceTexture(mailbox, nullptr);
-    return;
-  }
-
   if (!texture_ref) {
     LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "ProduceTextureDirect",
                        "unknown texture");
     return;
   }
 
-  Texture* produced = texture_manager()->Produce(texture_ref);
+  group_->mailbox_manager()->ProduceTexture(mailbox, texture_ref->texture());
-  if (!produced) {
-    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "ProduceTextureDirect",
-                       "invalid texture");
-    return;
-  }
-
-  group_->mailbox_manager()->ProduceTexture(mailbox, produced);
 }
 
 bool RasterDecoderImpl::TexStorage2DImage(

gpu/command_buffer/service/texture_manager.cc

@@ -2186,11 +2186,6 @@ void TextureManager::SetLevelInfo(TextureRef* ref,
                                              texture->estimated_size());
 }
 
-Texture* TextureManager::Produce(TextureRef* ref) {
-  DCHECK(ref);
-  return ref->texture();
-}
-
 TextureRef* TextureManager::Consume(
     GLuint client_id,
     Texture* texture) {

gpu/command_buffer/service/texture_manager.h

@@ -844,8 +844,6 @@ class GPU_GLES2_EXPORT TextureManager
                     GLenum type,
                     const gfx::Rect& cleared_rect);
 
-  Texture* Produce(TextureRef* ref);
-
   // Maps an existing texture into the texture manager, at a given client ID.
   TextureRef* Consume(GLuint client_id, Texture* texture);
 

gpu/command_buffer/service/texture_manager_unittest.cc

@@ -1966,11 +1966,7 @@ class ProduceConsumeTextureTest : public TextureTest,
     return info;
   }
 
-  Texture* Produce(TextureRef* texture_ref) {
+  Texture* Produce(TextureRef* texture_ref) { return texture_ref->texture(); }
-    Texture* texture = manager_->Produce(texture_ref);
-    EXPECT_TRUE(texture != NULL);
-    return texture;
-  }
 
   void Consume(GLuint client_id, Texture* texture) {
     EXPECT_TRUE(manager_->Consume(client_id, texture));