gpu: Reuse transfer buffers more aggresively

gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
 #include "gpu/command_buffer/common/gles2_cmd_format.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
@@ skipping 8205 matching lines @@
   // Set a mock Async delegate
   StrictMock<gpu::MockAsyncPixelTransferManager>* manager =
       new StrictMock<gpu::MockAsyncPixelTransferManager>;
   manager->Initialize(group().texture_manager());
   decoder_->SetAsyncPixelTransferManagerForTest(manager);
   StrictMock<gpu::MockAsyncPixelTransferDelegate>* delegate = NULL;
 
   // 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(
       decoder_->GetAsyncPixelTransferManager()->GetPixelTransferDelegate(
           texture_ref));
 
   base::Closure bind_callback;
 
   // AsyncTexImage2D
   {
@@ skipping 111 matching lines @@
     DoDeleteTexture(client_texture_id_, kServiceTextureId);
     EXPECT_FALSE(
         decoder_->GetAsyncPixelTransferManager()->GetPixelTransferDelegate(
             texture_ref));
     delegate = NULL;
   }
 
   // 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);
     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.
     EXPECT_CALL(*delegate, WaitForTransferCompletion());
     EXPECT_CALL(*manager, BindCompletedAsyncTransfers());
     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) {
   InitDecoder(
       "GL_CHROMIUM_async_pixel_transfers",  // extensions
       "3.0",   // gl version
       false, false, false,  // has alpha/depth/stencil
       false, false, false,  // request alpha/depth/stencil
       true);   // bind generates resource
 
   // Set up the texture.
   DoBindTexture(GL_TEXTURE_2D, client_texture_id_, kServiceTextureId);
   TextureRef* texture_ref = GetTexture(client_texture_id_);
 
   // Set a mock Async delegate.
   StrictMock<gpu::MockAsyncPixelTransferManager>* manager =
       new StrictMock<gpu::MockAsyncPixelTransferManager>;
   manager->Initialize(group().texture_manager());
   decoder_->SetAsyncPixelTransferManagerForTest(manager);
   StrictMock<gpu::MockAsyncPixelTransferDelegate>* delegate = NULL;
 
   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(
       decoder_->GetAsyncPixelTransferManager()->GetPixelTransferDelegate(
           texture_ref));
 
   // Create delegate on AsyncTexImage2D.
   {
     EXPECT_CALL(*manager, CreatePixelTransferDelegateImpl(texture_ref, _))
         .WillOnce(Return(
@@ skipping 807 matching lines @@
 // TODO(gman): TexImage2DImmediate
 
 // TODO(gman): TexSubImage2DImmediate
 
 // TODO(gman): UseProgram
 
 // TODO(gman): SwapBuffers
 
 }  // namespace gles2
 }  // namespace gpu