Re-land: Mark async texture uploads as completed from the upload thread.

ui/gl/async_pixel_transfer_delegate_android.cc

Index: ui/gl/async_pixel_transfer_delegate_android.cc
diff --git a/ui/gl/async_pixel_transfer_delegate_android.cc b/ui/gl/async_pixel_transfer_delegate_android.cc
index 68c329f162ab4df9febe1f351f73ba2e492a8f55..3185b44c0055ea84c99f2829a87844824d638a0a 100644
--- a/ui/gl/async_pixel_transfer_delegate_android.cc
+++ b/ui/gl/async_pixel_transfer_delegate_android.cc
@@ -15,6 +15,7 @@
 #include "base/shared_memory.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
+#include "gpu/command_buffer/common/gles2_cmd_format.h"
 #include "ui/gl/async_pixel_transfer_delegate.h"
 #include "ui/gl/async_pixel_transfer_delegate_stub.h"
 #include "ui/gl/egl_util.h"
@@ -32,6 +33,34 @@ namespace gfx {
 
 namespace {
 
+class TextureUploadStats

[epenner, 2013/02/07 20:36:34]

Can this be another change? I don't understand how upload stats are related to
this change. And it adds another lock, each instance of which requires a lot of
justification, IMO.

[reveman, 2013/02/07 21:30:52]

I can't get rid of the replies and keep collecting upload stats correctly
without this. we can of course remove the stats but that breaks the benchmarks
extension.

[epenner, 2013/02/07 21:59:47]

I see, let's not get rid of the reply then? We should minimize the impact of
this latency savings optimization on design cleanliness.

Could the query code also benefit from the reply to reduce a lot of the changes
there? Could we do *only* the query completion on the upload thread and handle
the rest in a reply to the GPU thread?  It appears a lot of GPU side code was
written to work around not having the reply task.

+    : public base::RefCountedThreadSafe<TextureUploadStats> {
+ public:
+  TextureUploadStats() : texture_upload_count_(0) {}
+
+  void AddUpload(base::TimeDelta transfer_time) {
+    base::AutoLock scoped_lock(lock_);
+    texture_upload_count_++;
+    total_texture_upload_time_ += transfer_time;
+  }
+
+  int GetStats(base::TimeDelta* total_texture_upload_time) {
+    base::AutoLock scoped_lock(lock_);
+    if (total_texture_upload_time)
+      *total_texture_upload_time = total_texture_upload_time_;
+    return texture_upload_count_;
+  }
+
+ private:
+  friend class RefCountedThreadSafe<TextureUploadStats>;
+
+  ~TextureUploadStats() {}
+
+  int texture_upload_count_;
+  base::TimeDelta total_texture_upload_time_;
+  base::Lock lock_;
+};
+
 const char kAsyncTransferThreadName[] = "AsyncTransferThread";
 
 bool CheckErrors(const char* file, int line) {
@@ -152,6 +181,7 @@ class TransferStateInternal
 
   // Implement AsyncPixelTransferState:
   bool TransferIsInProgress() {
+    base::AutoLock scoped_lock(transfer_in_progress_lock_);
     return transfer_in_progress_;
   }
 
@@ -233,6 +263,12 @@ class TransferStateInternal
     }
   }
 
+  void MarkAsCompleted() {
+    base::AutoLock scoped_lock(transfer_in_progress_lock_);
+    DCHECK(transfer_in_progress_);
+    transfer_in_progress_ = false;
+  }
+
  protected:
   friend class base::RefCountedThreadSafe<TransferStateInternal>;
   friend class AsyncPixelTransferDelegateAndroid;
@@ -267,15 +303,13 @@ class TransferStateInternal
 
   // Indicates that an async transfer is in progress.
   bool transfer_in_progress_;
+  base::Lock transfer_in_progress_lock_;

[reveman, 2013/02/07 19:59:02]

we could use base::subtle::Atomic32 here instead if performance is a concern.

[epenner, 2013/02/07 20:22:56]

Can you explain a bit why we need a lock or atomic int at all?

[reveman, 2013/02/07 21:30:52]

Both threads will access this. Main thread to determine when transfer is done.
Upload thread sets it when done.

[epenner, 2013/02/07 21:59:47]

That's what atomic ints are used for yes, but I'm asking why it's required from
a design perspective. Could this benefit from the reply as well?

If the reply is posted to the GPU before we mark the completion, it's impossible
for the renderer to do something on the GPU thread before the GPU handles the
reply.

 
   // It would be nice if we could just create a new EGLImage for
   // every upload, but I found that didn't work, so this stores
   // one for the lifetime of the texture.
   EGLImageKHR egl_image_;
 
-  // Time spent performing last transfer.
-  base::TimeDelta last_transfer_time_;
-
   // Customize when we block on fences (these are work-arounds).
   bool wait_for_uploads_;
   bool wait_for_egl_images_;
@@ -304,16 +338,15 @@ class AsyncTransferStateAndroid : public AsyncPixelTransferState {
 
 // Class which handles async pixel transfers on Android (using
 // EGLImageKHR and another upload thread)
-class AsyncPixelTransferDelegateAndroid
-    : public AsyncPixelTransferDelegate,
-      public base::SupportsWeakPtr<AsyncPixelTransferDelegateAndroid> {
+class AsyncPixelTransferDelegateAndroid : public AsyncPixelTransferDelegate {
  public:
   AsyncPixelTransferDelegateAndroid();
   virtual ~AsyncPixelTransferDelegateAndroid();
 
   // implement AsyncPixelTransferDelegate:
   virtual void AsyncNotifyCompletion(
-      const base::Closure& task) OVERRIDE;
+      const AsyncMemoryParams& mem_params,
+      uint32 submit_count) OVERRIDE;
   virtual void AsyncTexImage2D(
       AsyncPixelTransferState* state,
       const AsyncTexImage2DParams& tex_params,
@@ -330,9 +363,6 @@ class AsyncPixelTransferDelegateAndroid
   virtual AsyncPixelTransferState*
       CreateRawPixelTransferState(GLuint texture_id) OVERRIDE;
 
-  void AsyncTexImage2DCompleted(scoped_refptr<TransferStateInternal> state);
-  void AsyncTexSubImage2DCompleted(scoped_refptr<TransferStateInternal> state);
-
   static void PerformAsyncTexImage2D(
       TransferStateInternal* state,
       AsyncTexImage2DParams tex_params,
@@ -342,7 +372,12 @@ class AsyncPixelTransferDelegateAndroid
       TransferStateInternal* state,
       AsyncTexSubImage2DParams tex_params,
       base::SharedMemory* shared_memory,
-      uint32 shared_memory_data_offset);
+      uint32 shared_memory_data_offset,
+      scoped_refptr<TextureUploadStats> texture_upload_stats);
+  static void PerformNotifyCompletion(
+      base::SharedMemory* shared_memory,
+      uint32 shared_memory_data_offset,
+      uint32 submit_count);
 
   // Returns true if a work-around was used.
   bool WorkAroundAsyncTexImage2D(
@@ -354,8 +389,7 @@ class AsyncPixelTransferDelegateAndroid
       const AsyncTexSubImage2DParams& tex_params,
       const AsyncMemoryParams& mem_params);
 
-  int texture_upload_count_;
-  base::TimeDelta total_texture_upload_time_;
+  scoped_refptr<TextureUploadStats> texture_upload_stats_;
   bool is_imagination_;
   bool is_qualcomm_;
 
@@ -395,12 +429,13 @@ scoped_ptr<AsyncPixelTransferDelegate>
   }
 }
 
-AsyncPixelTransferDelegateAndroid::AsyncPixelTransferDelegateAndroid()
-    : texture_upload_count_(0) {
+AsyncPixelTransferDelegateAndroid::AsyncPixelTransferDelegateAndroid() {
   std::string vendor;
   vendor = reinterpret_cast<const char*>(glGetString(GL_VENDOR));
   is_imagination_ = vendor.find("Imagination") != std::string::npos;
   is_qualcomm_ = vendor.find("Qualcomm") != std::string::npos;
+  // TODO(reveman): Skip this if --enable-gpu-benchmarking is not present.
+  texture_upload_stats_ = make_scoped_refptr(new TextureUploadStats);
 }
 
 AsyncPixelTransferDelegateAndroid::~AsyncPixelTransferDelegateAndroid() {
@@ -422,19 +457,22 @@ AsyncPixelTransferState*
                                     wait_for_egl_images));
 }
 
-namespace {
-// Dummy function to measure completion on
-// the upload thread.
-void NoOp() {}
-} // namespace
-
 void AsyncPixelTransferDelegateAndroid::AsyncNotifyCompletion(
-      const base::Closure& task) {
-  // Post a no-op task to the upload thread followed
-  // by a reply to the callback. The reply will then occur after
-  // all async transfers are complete.
-  transfer_message_loop_proxy()->PostTaskAndReply(FROM_HERE,
-      base::Bind(&NoOp), task);
+    const AsyncMemoryParams& mem_params,
+    uint32 submit_count) {
+  DCHECK(mem_params.shared_memory);
+  DCHECK_LE(mem_params.shm_data_offset + mem_params.shm_data_size,
+            mem_params.shm_size);
+  // Post a PerformNotifyCompletion task to the upload thread. This task
+  // will run after all async transfers are complete and mark the QuerySync as
+  // completed.
+  transfer_message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&AsyncPixelTransferDelegateAndroid::PerformNotifyCompletion,
+                 base::Owned(DuplicateSharedMemory(mem_params.shared_memory,
+                                                   mem_params.shm_size)),
+                 mem_params.shm_data_offset,
+                 submit_count));
 }
 
 void AsyncPixelTransferDelegateAndroid::AsyncTexImage2D(
@@ -458,23 +496,19 @@ void AsyncPixelTransferDelegateAndroid::AsyncTexImage2D(
     return;
 
   // Mark the transfer in progress and save define params for lazy binding.
+  state->needs_late_bind_ = true;
   state->transfer_in_progress_ = true;
   state->late_bind_define_params_ = tex_params;
 
   // Duplicate the shared memory so there are no way we can get
   // a use-after-free of the raw pixels.
-  transfer_message_loop_proxy()->PostTaskAndReply(FROM_HERE,
-      base::Bind(
-          &AsyncPixelTransferDelegateAndroid::PerformAsyncTexImage2D,
-          base::Unretained(state.get()),  // This is referenced in reply below.
-          tex_params,
-          base::Owned(DuplicateSharedMemory(mem_params.shared_memory,
-                                            mem_params.shm_size)),
-          mem_params.shm_data_offset),
-      base::Bind(
-          &AsyncPixelTransferDelegateAndroid::AsyncTexImage2DCompleted,
-          AsWeakPtr(),
-          state));
+  transfer_message_loop_proxy()->PostTask(FROM_HERE,
+      base::Bind(&AsyncPixelTransferDelegateAndroid::PerformAsyncTexImage2D,
+                 state,
+                 tex_params,
+                 base::Owned(DuplicateSharedMemory(mem_params.shared_memory,
+                                                   mem_params.shm_size)),
+                 mem_params.shm_data_offset));
 
   DCHECK(CHECK_GL());
 }
@@ -508,41 +542,28 @@ void AsyncPixelTransferDelegateAndroid::AsyncTexSubImage2D(
 
   // Duplicate the shared memory so there are no way we can get
   // a use-after-free of the raw pixels.
-  transfer_message_loop_proxy()->PostTaskAndReply(FROM_HERE,
-      base::Bind(
-          &AsyncPixelTransferDelegateAndroid::PerformAsyncTexSubImage2D,
-          base::Unretained(state.get()),  // This is referenced in reply below.
-          tex_params,
-          base::Owned(DuplicateSharedMemory(mem_params.shared_memory,
-                                            mem_params.shm_size)),
-          mem_params.shm_data_offset),
-      base::Bind(
-          &AsyncPixelTransferDelegateAndroid::AsyncTexSubImage2DCompleted,
-          AsWeakPtr(),
-          state));
+  transfer_message_loop_proxy()->PostTask(FROM_HERE,
+      base::Bind(&AsyncPixelTransferDelegateAndroid::PerformAsyncTexSubImage2D,
+                 state,
+                 tex_params,
+                 base::Owned(DuplicateSharedMemory(mem_params.shared_memory,
+                                                   mem_params.shm_size)),
+                 mem_params.shm_data_offset,
+                 texture_upload_stats_));
 
   DCHECK(CHECK_GL());
 }
 
 uint32 AsyncPixelTransferDelegateAndroid::GetTextureUploadCount() {
-  return texture_upload_count_;
+  CHECK(texture_upload_stats_);
+  return texture_upload_stats_->GetStats(NULL);
 }
 
 base::TimeDelta AsyncPixelTransferDelegateAndroid::GetTotalTextureUploadTime() {
-  return total_texture_upload_time_;
-}
-
-void AsyncPixelTransferDelegateAndroid::AsyncTexImage2DCompleted(
-    scoped_refptr<TransferStateInternal> state) {
-  state->needs_late_bind_ = true;
-  state->transfer_in_progress_ = false;
-}
-
-void AsyncPixelTransferDelegateAndroid::AsyncTexSubImage2DCompleted(
-    scoped_refptr<TransferStateInternal> state) {
-  state->transfer_in_progress_ = false;
-  texture_upload_count_++;
-  total_texture_upload_time_ += state->last_transfer_time_;
+  CHECK(texture_upload_stats_);
+  base::TimeDelta total_texture_upload_time;
+  texture_upload_stats_->GetStats(&total_texture_upload_time);
+  return total_texture_upload_time;
 }
 
 namespace {
@@ -610,6 +631,8 @@ void AsyncPixelTransferDelegateAndroid::PerformAsyncTexImage2D(
   }
 
   state->WaitForLastUpload();
+  state->MarkAsCompleted();
+
   DCHECK(CHECK_GL());
 }
 
@@ -617,7 +640,8 @@ void AsyncPixelTransferDelegateAndroid::PerformAsyncTexSubImage2D(
     TransferStateInternal* state,
     AsyncTexSubImage2DParams tex_params,
     base::SharedMemory* shared_memory,
-    uint32 shared_memory_data_offset) {
+    uint32 shared_memory_data_offset,
+    scoped_refptr<TextureUploadStats> texture_upload_stats) {
   TRACE_EVENT2("gpu", "PerformAsyncTexSubImage2D",
                "width", tex_params.width,
                "height", tex_params.height);
@@ -630,7 +654,10 @@ void AsyncPixelTransferDelegateAndroid::PerformAsyncTexSubImage2D(
 
   void* data = GetAddress(shared_memory, shared_memory_data_offset);
 
-  base::TimeTicks begin_time(base::TimeTicks::HighResNow());
+  base::TimeTicks begin_time;
+  if (texture_upload_stats)
+    begin_time = base::TimeTicks::HighResNow();
+
   if (!state->thread_texture_id_) {
     TRACE_EVENT0("gpu", "glEGLImageTargetTexture2DOES");
     glGenTextures(1, &state->thread_texture_id_);
@@ -655,11 +682,28 @@ void AsyncPixelTransferDelegateAndroid::PerformAsyncTexSubImage2D(
         data);
   }
   state->WaitForLastUpload();
+  state->MarkAsCompleted();
 
   DCHECK(CHECK_GL());
-  state->last_transfer_time_ = base::TimeTicks::HighResNow() - begin_time;
+
+  if (texture_upload_stats) {
+    texture_upload_stats->AddUpload(
+        base::TimeTicks::HighResNow() - begin_time);
+  }
 }
 
+void AsyncPixelTransferDelegateAndroid::PerformNotifyCompletion(
+    base::SharedMemory* shared_memory,
+    uint32 shared_memory_data_offset,
+    uint32 submit_count) {
+  TRACE_EVENT0("gpu", "PerformNotifyCompletion");
+
+  gpu::gles2::QuerySync* sync = static_cast<gpu::gles2::QuerySync*>(
+      GetAddress(shared_memory, shared_memory_data_offset));
+  if (!sync)
+    return;
+  sync->process_count = submit_count;
+}
 
 namespace {
 bool IsPowerOfTwo (unsigned int x) {
@@ -754,7 +798,9 @@ bool AsyncPixelTransferDelegateAndroid::WorkAroundAsyncTexSubImage2D(
   // Fall back on a synchronous stub as we don't have a known fast path.
   void* data = GetAddress(mem_params.shared_memory,
                           mem_params.shm_data_offset);
-  base::TimeTicks begin_time(base::TimeTicks::HighResNow());
+  base::TimeTicks begin_time;
+  if (texture_upload_stats_)
+    begin_time = base::TimeTicks::HighResNow();
   {
     TRACE_EVENT0("gpu", "glTexSubImage2D");
     glTexSubImage2D(
@@ -768,8 +814,10 @@ bool AsyncPixelTransferDelegateAndroid::WorkAroundAsyncTexSubImage2D(
         tex_params.type,
         data);
   }
-  texture_upload_count_++;
-  total_texture_upload_time_ += base::TimeTicks::HighResNow() - begin_time;
+  if (texture_upload_stats_) {
+    texture_upload_stats_->AddUpload(
+        base::TimeTicks::HighResNow() - begin_time);
+  }
 
   DCHECK(CHECK_GL());
   return true;