gpu: Reuse transfer buffers more aggresively

gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
index 15fd1d1278575196f048ccf2c202096a6ae59486..2786f88407450916dfaafeb0a095249e2563057a 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
@@ -8223,12 +8223,16 @@ TEST_F(GLES2DecoderManualInitTest, AsyncPixelTransfers) {
   // Tex(Sub)Image2D upload commands.
   AsyncTexImage2DCHROMIUM teximage_cmd;
   teximage_cmd.Init(GL_TEXTURE_2D, 0, GL_RGBA, 8, 8, 0, GL_RGBA,
-                    GL_UNSIGNED_BYTE, kSharedMemoryId, kSharedMemoryOffset);
+                    GL_UNSIGNED_BYTE, kSharedMemoryId, kSharedMemoryOffset,
+                    0, 0, 0);
   AsyncTexSubImage2DCHROMIUM texsubimage_cmd;
   texsubimage_cmd.Init(GL_TEXTURE_2D, 0, 0, 0, 8, 8, GL_RGBA,
-                      GL_UNSIGNED_BYTE, kSharedMemoryId, kSharedMemoryOffset);
+                      GL_UNSIGNED_BYTE, kSharedMemoryId, kSharedMemoryOffset,
+                      0, 0, 0);
   WaitAsyncTexImage2DCHROMIUM wait_cmd;
   wait_cmd.Init(GL_TEXTURE_2D);
+  WaitAllAsyncTexImage2DCHROMIUM wait_all_cmd;
+  wait_all_cmd.Init();
 
   // No transfer state exists initially.
   EXPECT_FALSE(
@@ -8360,7 +8364,7 @@ TEST_F(GLES2DecoderManualInitTest, AsyncPixelTransfers) {
   // WaitAsyncTexImage2D
   {
     // Get a fresh texture since the existing texture cannot be respecified
-    // asynchronously and AsyncTexSubImage2D does not involved binding.
+    // asynchronously and AsyncTexSubImage2D does not involve binding.
     EXPECT_CALL(*gl_, GenTextures(1, _))
         .WillOnce(SetArgumentPointee<1>(kServiceTextureId));
     DoBindTexture(GL_TEXTURE_2D, client_texture_id_, kServiceTextureId);
@@ -8389,6 +8393,45 @@ TEST_F(GLES2DecoderManualInitTest, AsyncPixelTransfers) {
     EXPECT_EQ(error::kNoError, ExecuteCmd(wait_cmd));
     EXPECT_EQ(GL_NO_ERROR, GetGLError());
   }
+
+  // WaitAllAsyncTexImage2D
+  EXPECT_CALL(*delegate, Destroy()).RetiresOnSaturation();
+  DoDeleteTexture(client_texture_id_, kServiceTextureId);
+  EXPECT_FALSE(
+      decoder_->GetAsyncPixelTransferManager()->GetPixelTransferDelegate(
+          texture_ref));

[piman, 2014/03/28 20:04:35]

nit: actually, texture_ref should probably be a scoped_refptr. Otherwise we may
be comparing a pointer to deleted memory. While we don't dereference it, it's in
theory possible that another TextureRef would be allocated at the same address
(actually, with allocators like tcmalloc, it becomes very likely), and inserted
into the map. It probably doesn't happen here given how the test is written, but
if we want to be thorough, that'd be the right thing to do.

[jadahl, 2014/03/28 20:11:38]

Do you want that change in this patch? It'll change texture_ref to
texture_ref.get() in several places spread all over this function.

[piman, 2014/03/28 20:22:49]

Either that or add a comment to indicate that texture_ref is actually deleted,
so one must be extra careful when touching this. I prefer the former though.

[jadahl, 2014/03/28 20:25:00]

Tests also assume no reference is held, and fail because we don't dereference.
If we do (after DoDelete, before GetPixelTransferDelegate) we have to compare to
with the pointer texture_ref had before we dereferenced, which would still have
the same issue as before.

+  delegate = NULL;
+  {
+    // Get a fresh texture since the existing texture cannot be respecified
+    // asynchronously and AsyncTexSubImage2D does not involve binding.
+    EXPECT_CALL(*gl_, GenTextures(1, _))
+        .WillOnce(SetArgumentPointee<1>(kServiceTextureId));
+    DoBindTexture(GL_TEXTURE_2D, client_texture_id_, kServiceTextureId);
+    texture_ref = GetTexture(client_texture_id_);
+    texture = texture_ref->texture();
+    texture->SetImmutable(false);
+    // Create transfer state since it doesn't exist.
+    EXPECT_CALL(*manager, CreatePixelTransferDelegateImpl(texture_ref, _))
+        .WillOnce(Return(
+            delegate = new StrictMock<gpu::MockAsyncPixelTransferDelegate>))
+        .RetiresOnSaturation();
+    EXPECT_CALL(*delegate, AsyncTexImage2D(_, _, _))
+        .RetiresOnSaturation();
+    // Start async transfer.
+    EXPECT_EQ(error::kNoError, ExecuteCmd(teximage_cmd));
+    EXPECT_EQ(GL_NO_ERROR, GetGLError());
+    EXPECT_EQ(
+        delegate,
+        decoder_->GetAsyncPixelTransferManager()->GetPixelTransferDelegate(
+            texture_ref));
+
+    EXPECT_TRUE(texture->IsImmutable());
+    // Wait for completion of all uploads.
+    EXPECT_CALL(*manager, WaitAllAsyncTexImage2D()).RetiresOnSaturation();
+    EXPECT_CALL(*manager, BindCompletedAsyncTransfers());
+    EXPECT_EQ(error::kNoError, ExecuteCmd(wait_all_cmd));
+    EXPECT_EQ(GL_NO_ERROR, GetGLError());
+  }
 }
 
 TEST_F(GLES2DecoderManualInitTest, AsyncPixelTransferManager) {
@@ -8412,7 +8455,8 @@ TEST_F(GLES2DecoderManualInitTest, AsyncPixelTransferManager) {
 
   AsyncTexImage2DCHROMIUM teximage_cmd;
   teximage_cmd.Init(GL_TEXTURE_2D, 0, GL_RGBA, 8, 8, 0, GL_RGBA,
-                    GL_UNSIGNED_BYTE, kSharedMemoryId, kSharedMemoryOffset);
+                    GL_UNSIGNED_BYTE, kSharedMemoryId, kSharedMemoryOffset,
+                    0, 0, 0);
 
   // No transfer delegate exists initially.
   EXPECT_FALSE(