ui/gl: Fix some assumptions around TransactionAcks in SurfaceControl.

This change fixes a couple of incorrect assumptions we were making with
respect to the behaviour of transaction acks on SurfaceControl.

TBR=nyquist@chromium.org
R=piman@chromium.org

Change-Id: I4c1997997c158e99eca17b013ed9db8d9776ef1b
Bug: 942614
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1526638
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Auto-Submit: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#641683}

base/android/scoped_hardware_buffer_fence_sync.h

@@ -27,9 +27,7 @@ class BASE_EXPORT ScopedHardwareBufferFenceSync {
   ScopedFD TakeFence();
 
   // Provides fence which is signaled when the reads for this buffer are done
-  // and it can be reused. The method assumes a current GLContext and will only
-  // synchronize the reads with this context.
-  // Must only be called once.
+  // and it can be reused. Must only be called once.
   virtual void SetReadFence(base::ScopedFD fence_fd) = 0;
 
  private:

media/gpu/BUILD.gn

@@ -189,7 +189,6 @@ component("gpu") {
     libs += [ "android" ]
     deps += [
       "//gpu/ipc/common:android_image_reader_utils",
-      "//third_party/libsync",
 
       # TODO(crbug.com/789435): This can be removed once CdmManager is removed.
       "//gpu/ipc/common:ipc_common_sources",

media/gpu/android/image_reader_gl_owner.cc

@@ -19,27 +19,12 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "gpu/command_buffer/service/abstract_texture.h"
 #include "gpu/ipc/common/android/android_image_reader_utils.h"
-#include "third_party/libsync/src/include/sync/sync.h"
 #include "ui/gl/gl_fence_android_native_fence_sync.h"
+#include "ui/gl/gl_utils.h"
 #include "ui/gl/scoped_binders.h"
 #include "ui/gl/scoped_make_current.h"
 
 namespace media {
-namespace {
-
-base::ScopedFD MergeFDs(base::ScopedFD a, base::ScopedFD b) {
-  if (!a.is_valid())
-    return b;
-  if (!b.is_valid())
-    return a;
-
-  base::ScopedFD merged(HANDLE_EINTR(sync_merge("", a.get(), b.get())));
-  if (!merged.is_valid())
-    LOG(ERROR) << "Failed to merge fences.";
-  return merged;
-}
-
-}  // namespace
 
 // FrameAvailableEvent_ImageReader is a RefCounted wrapper for a WaitableEvent
 // (it's not possible to put one in RefCountedData). This lets us safely signal
@@ -313,7 +298,7 @@ void ImageReaderGLOwner::ReleaseRefOnImage(AImage* image,
   DCHECK_GT(image_ref.count, 0u);
   image_ref.count--;
   image_ref.release_fence_fd =
-      MergeFDs(std::move(image_ref.release_fence_fd), std::move(fence_fd));
+      gl::MergeFDs(std::move(image_ref.release_fence_fd), std::move(fence_fd));
 
   if (image_ref.count > 0)
     return;

ui/gl/gl_image_ahardwarebuffer.cc

@@ -8,32 +8,11 @@
 #include "base/android/scoped_hardware_buffer_fence_sync.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_fence_android_native_fence_sync.h"
+#include "ui/gl/gl_utils.h"
 
 namespace gl {
 namespace {
 
-class ScopedHardwareBufferFenceSyncImpl
-    : public base::android::ScopedHardwareBufferFenceSync {
- public:
-  ScopedHardwareBufferFenceSyncImpl(
-      base::android::ScopedHardwareBufferHandle handle,
-      base::ScopedFD fence_fd)
-      : ScopedHardwareBufferFenceSync(std::move(handle), std::move(fence_fd)) {}
-  ~ScopedHardwareBufferFenceSyncImpl() override = default;
-
-  void SetReadFence(base::ScopedFD fence_fd) override {
-    // Insert a service wait for this fence to ensure any resource reuse is
-    // after it is signaled.
-    gfx::GpuFenceHandle handle;
-    handle.type = gfx::GpuFenceHandleType::kAndroidNativeFenceSync;
-    handle.native_fd =
-        base::FileDescriptor(fence_fd.release(), /*auto_close=*/true);
-    gfx::GpuFence gpu_fence(handle);
-    auto gl_fence = GLFence::CreateFromGpuFence(gpu_fence);
-    gl_fence->ServerWait();
-  }
-};
-
 uint32_t GetBufferFormat(const AHardwareBuffer* buffer) {
   AHardwareBuffer_Desc desc = {};
   base::AndroidHardwareBufferCompat::GetInstance().Describe(buffer, &desc);
@@ -56,8 +35,41 @@ unsigned int GLInternalFormat(uint32_t buffer_format) {
   }
 }
 
+void InsertGLFence(base::ScopedFD fence_fd) {
+  if (!fence_fd.is_valid())
+    return;
+
+  gfx::GpuFenceHandle handle;
+  handle.type = gfx::GpuFenceHandleType::kAndroidNativeFenceSync;
+  handle.native_fd =
+      base::FileDescriptor(fence_fd.release(), /*auto_close=*/true);
+  gfx::GpuFence gpu_fence(handle);
+  auto gl_fence = GLFence::CreateFromGpuFence(gpu_fence);
+  gl_fence->ServerWait();
+}
+
 }  // namespace
 
+class GLImageAHardwareBuffer::ScopedHardwareBufferFenceSyncImpl
+    : public base::android::ScopedHardwareBufferFenceSync {
+ public:
+  ScopedHardwareBufferFenceSyncImpl(
+      scoped_refptr<GLImageAHardwareBuffer> image,
+      base::android::ScopedHardwareBufferHandle handle,
+      base::ScopedFD fence_fd)
+      : ScopedHardwareBufferFenceSync(std::move(handle), std::move(fence_fd)),
+        image_(std::move(image)) {}
+  ~ScopedHardwareBufferFenceSyncImpl() override = default;
+
+  void SetReadFence(base::ScopedFD fence_fd) override {
+    image_->release_fence_ =
+        MergeFDs(std::move(image_->release_fence_), std::move(fence_fd));
+  }
+
+ private:
+  scoped_refptr<GLImageAHardwareBuffer> image_;
+};
+
 GLImageAHardwareBuffer::GLImageAHardwareBuffer(const gfx::Size& size)
     : GLImageEGL(size) {}
 
@@ -79,6 +91,11 @@ unsigned GLImageAHardwareBuffer::GetInternalFormat() {
   return internal_format_;
 }
 
+bool GLImageAHardwareBuffer::BindTexImage(unsigned target) {
+  InsertGLFence(std::move(release_fence_));
+  return GLImageEGL::BindTexImage(target);
+}
+
 bool GLImageAHardwareBuffer::CopyTexImage(unsigned target) {
   return false;
 }
@@ -110,8 +127,8 @@ void GLImageAHardwareBuffer::OnMemoryDump(
 std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
 GLImageAHardwareBuffer::GetAHardwareBuffer() {
   return std::make_unique<ScopedHardwareBufferFenceSyncImpl>(
-      base::android::ScopedHardwareBufferHandle::Create(handle_.get()),
-      base::ScopedFD());
+      this, base::android::ScopedHardwareBufferHandle::Create(handle_.get()),
+      std::move(release_fence_));
 }
 
 }  // namespace gl

ui/gl/gl_image_ahardwarebuffer.h

@@ -30,6 +30,7 @@ class GL_EXPORT GLImageAHardwareBuffer : public GLImageEGL {
 
   // Overridden from GLImage:
   unsigned GetInternalFormat() override;
+  bool BindTexImage(unsigned target) override;
   bool CopyTexImage(unsigned target) override;
   bool CopyTexSubImage(unsigned target,
                        const gfx::Point& offset,
@@ -52,6 +53,9 @@ class GL_EXPORT GLImageAHardwareBuffer : public GLImageEGL {
   ~GLImageAHardwareBuffer() override;
 
  private:
+  class ScopedHardwareBufferFenceSyncImpl;
+
+  base::ScopedFD release_fence_;
   base::android::ScopedHardwareBufferHandle handle_;
   unsigned internal_format_ = GL_RGBA;
 

ui/gl/gl_surface_egl_surface_control.cc

@@ -13,6 +13,7 @@
 #include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gl/gl_context.h"
 #include "ui/gl/gl_image_ahardwarebuffer.h"
+#include "ui/gl/gl_utils.h"
 
 namespace gl {
 namespace {
@@ -137,6 +138,14 @@ void GLSurfaceEGLSurfaceControl::CommitPendingTransaction(
                                         surface_damage_rect);
   }
 
+  // Surfaces which are present in the current frame but not in the next frame
+  // need to be explicitly updated in order to get a release fence for them in
+  // the next transaction.
+  for (size_t i = pending_surfaces_count_; i < surface_list_.size(); ++i) {
+    pending_transaction_->SetBuffer(*surface_list_[i].surface, nullptr,
+                                    base::ScopedFD());
+  }
+
   // Release resources for the current frame once the next frame is acked.
   ResourceRefs resources_to_release;
   resources_to_release.swap(current_frame_resources_);
@@ -152,6 +161,7 @@ void GLSurfaceEGLSurfaceControl::CommitPendingTransaction(
       std::move(present_callback), std::move(resources_to_release));
   pending_transaction_->SetOnCompleteCb(std::move(callback), gpu_task_runner_);
 
+  pending_transaction_acks_++;
   pending_transaction_->Apply();
   pending_transaction_.reset();
 
@@ -218,11 +228,12 @@ bool GLSurfaceEGLSurfaceControl::ScheduleOverlayPlane(
   if (surface_state.buffer_updated_in_pending_transaction) {
     surface_state.hardware_buffer = hardware_buffer;
 
-    if (!fence_fd.is_valid() && gpu_fence && surface_state.hardware_buffer) {
+    if (gpu_fence && surface_state.hardware_buffer) {
       auto fence_handle =
           gfx::CloneHandleForIPC(gpu_fence->GetGpuFenceHandle());
       DCHECK(!fence_handle.is_null());
-      fence_fd = base::ScopedFD(fence_handle.native_fd.fd);
+      fence_fd = MergeFDs(std::move(fence_fd),
+                          base::ScopedFD(fence_handle.native_fd.fd));
     }
 
     pending_transaction_->SetBuffer(*surface_state.surface,
@@ -300,7 +311,9 @@ void GLSurfaceEGLSurfaceControl::OnTransactionAckOnGpuThread(
     ResourceRefs released_resources,
     SurfaceControl::TransactionStats transaction_stats) {
   DCHECK(gpu_task_runner_->BelongsToCurrentThread());
-  context_->MakeCurrent(this);
+  DCHECK_GT(pending_transaction_acks_, 0u);
+
+  pending_transaction_acks_--;
 
   // The presentation feedback callback must run after swap completion.
   std::move(completion_callback).Run(gfx::SwapResult::SWAP_ACK, nullptr);
@@ -314,10 +327,26 @@ void GLSurfaceEGLSurfaceControl::OnTransactionAckOnGpuThread(
 
   for (auto& surface_stat : transaction_stats.surface_stats) {
     auto it = released_resources.find(surface_stat.surface);
-    DCHECK(it != released_resources.end());
+
+    // The transaction ack includes data for all surfaces updated in this
+    // transaction. So the following condition can occur if a new surface was
+    // added in this transaction with a buffer. It'll be included in the ack
+    // with no fence, since its not being released and so shouldn't be in
+    // |released_resources| either.
+    if (it == released_resources.end()) {
+      DCHECK(!surface_stat.fence.is_valid());
+      continue;
+    }
+
     if (surface_stat.fence.is_valid())
       it->second.scoped_buffer->SetReadFence(std::move(surface_stat.fence));
   }
+
+  // Note that we may not see |surface_stats| for every resource above. This is
+  // because we take a ref on every buffer used in a frame, even if it is not
+  // updated in that frame. Since the transaction ack only includes surfaces
+  // which were updated in that transaction, the surfaces with no buffer updates
+  // won't be present in the ack.
   released_resources.clear();
 }
 

ui/gl/gl_surface_egl_surface_control.h

@@ -166,6 +166,9 @@ class GL_EXPORT GLSurfaceEGLSurfaceControl : public GLSurfaceEGL {
   // The last context made current with this surface.
   scoped_refptr<GLContext> context_;
 
+  // Number of transaction which have been applied and are awaiting an ack.
+  size_t pending_transaction_acks_ = 0;
+
   scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_;
   base::WeakPtrFactory<GLSurfaceEGLSurfaceControl> weak_factory_;
 };

ui/gl/gl_utils.cc

@@ -10,6 +10,11 @@
 #include "ui/gfx/color_space.h"
 #include "ui/gl/gl_bindings.h"
 
+#if defined(OS_ANDROID)
+#include "base/posix/eintr_wrapper.h"
+#include "third_party/libsync/src/include/sync/sync.h"
+#endif
+
 namespace gl {
 
 int GetGLColorSpace(const gfx::ColorSpace& color_space) {
@@ -26,4 +31,18 @@ void Crash() {
   volatile int* it_s_the_end_of_the_world_as_we_know_it = nullptr;
   *it_s_the_end_of_the_world_as_we_know_it = 0xdead;
 }
+
+#if defined(OS_ANDROID)
+base::ScopedFD MergeFDs(base::ScopedFD a, base::ScopedFD b) {
+  if (!a.is_valid())
+    return b;
+  if (!b.is_valid())
+    return a;
+
+  base::ScopedFD merged(HANDLE_EINTR(sync_merge("", a.get(), b.get())));
+  if (!merged.is_valid())
+    LOG(ERROR) << "Failed to merge fences.";
+  return merged;
+}
+#endif
 }

ui/gl/gl_utils.h

@@ -7,8 +7,13 @@
 #ifndef UI_GL_GL_UTILS_H_
 #define UI_GL_GL_UTILS_H_
 
+#include "build/build_config.h"
 #include "ui/gl/gl_export.h"
 
+#if defined(OS_ANDROID)
+#include "base/files/scoped_file.h"
+#endif
+
 namespace gfx {
 class ColorSpace;
 }  // namespace gfx
@@ -16,6 +21,11 @@ class ColorSpace;
 namespace gl {
 GL_EXPORT int GetGLColorSpace(const gfx::ColorSpace& color_space);
 GL_EXPORT void Crash();
+
+#if defined(OS_ANDROID)
+GL_EXPORT base::ScopedFD MergeFDs(base::ScopedFD a, base::ScopedFD b);
+#endif
+
 }  // namespace gl
 
 #endif  // UI_GL_GL_UTILS_H_