Use IOSurface directly in gfx::BufferHandle

Change gfx::BufferHandle to use a gfx::ScopedIOSurface instead of a
gfx::ScopedRefCountedIOSurfaceMachPort.

Reason for using a mach port: It's the thing that goes over all forms
of IPC.

Reason for not using a mach port: The mach port's existence makes the
IOSurface report that it is in use. This makes it so that things like
CVPixelBufferPools will not re-use that IOSurface. The capture pipeline
keeps lots of gfx::BufferHandles around, which causes the underlying
CVPixelBufferPool to go haywire (in this case growing without bound,
but another valid way to blow up is to just hang and never capture
any more frames).

Also remove some debugging code.

Bug: 1139105
Change-Id: I1ffa11a325b6a8e1811a23b1c4d7dac00fad49db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485992Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818727}

gpu/ipc/common/gpu_memory_buffer_impl_io_surface.cc

@@ -67,13 +67,12 @@ GpuMemoryBufferImplIOSurface::CreateFromHandle(
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
     DestructionCallback callback) {
-  if (!handle.mach_port) {
-    LOG(ERROR) << "Invalid IOSurface mach port returned to client.";
+  if (!handle.io_surface) {
+    LOG(ERROR) << "Invalid IOSurface returned to client.";
     return nullptr;
   }
 
-  base::ScopedCFTypeRef<IOSurfaceRef> io_surface(
-      IOSurfaceLookupFromMachPort(handle.mach_port.get()));
+  gfx::ScopedIOSurface io_surface = handle.io_surface;
   if (!io_surface) {
     LOG(ERROR) << "Failed to open IOSurface via mach port returned to client.";
     static int dump_counter = kMaxCrashDumps;
@@ -101,13 +100,11 @@ base::OnceClosure GpuMemoryBufferImplIOSurface::AllocateForTesting(
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
     gfx::GpuMemoryBufferHandle* handle) {
-  base::ScopedCFTypeRef<IOSurfaceRef> io_surface(
-      gfx::CreateIOSurface(size, format));
-  DCHECK(io_surface);
   gfx::GpuMemoryBufferId kBufferId(1);
   handle->type = gfx::IO_SURFACE_BUFFER;
   handle->id = kBufferId;
-  handle->mach_port.reset(IOSurfaceCreateMachPort(io_surface));
+  handle->io_surface.reset(gfx::CreateIOSurface(size, format));
+  DCHECK(handle->io_surface);
   return base::DoNothing();
 }
 
@@ -152,7 +149,7 @@ gfx::GpuMemoryBufferHandle GpuMemoryBufferImplIOSurface::CloneHandle() const {
   gfx::GpuMemoryBufferHandle handle;
   handle.type = gfx::IO_SURFACE_BUFFER;
   handle.id = id_;
-  handle.mach_port.reset(IOSurfaceCreateMachPort(io_surface_));
+  handle.io_surface = io_surface_;
   return handle;
 }
 

gpu/ipc/service/gpu_memory_buffer_factory_io_surface.cc

@@ -19,12 +19,10 @@
 namespace gpu {
 
 namespace {
+
 // A GpuMemoryBuffer with client_id = 0 behaves like anonymous shared memory.
 const int kAnonymousClientId = 0;
 
-// The maximum number of times to dump before throttling (to avoid sending
-// thousands of crash dumps).
-const int kMaxCrashDumps = 10;
 }  // namespace
 
 GpuMemoryBufferFactoryIOSurface::GpuMemoryBufferFactoryIOSurface() {
@@ -56,27 +54,7 @@ GpuMemoryBufferFactoryIOSurface::CreateGpuMemoryBuffer(
   gfx::GpuMemoryBufferHandle handle;
   handle.type = gfx::IO_SURFACE_BUFFER;
   handle.id = id;
-  handle.mach_port.reset(IOSurfaceCreateMachPort(io_surface));
-  CHECK(handle.mach_port);
-
-  // This IOSurface will be opened via mach port in the client process. It has
-  // been observed in https://crbug.com/574014 that these ports sometimes fail
-  // to be opened in the client process. It has further been observed in
-  // https://crbug.com/795649#c30 that these ports fail to be opened in creating
-  // process. To determine if these failures are independent, attempt to open
-  // the creating process first (and don't not return those that fail).
-  base::ScopedCFTypeRef<IOSurfaceRef> io_surface_from_mach_port(
-      IOSurfaceLookupFromMachPort(handle.mach_port.get()));
-  if (!io_surface_from_mach_port) {
-    LOG(ERROR) << "Failed to locally open IOSurface from mach port to be "
-                  "returned to client, not returning to client.";
-    static int dump_counter = kMaxCrashDumps;
-    if (dump_counter) {
-      dump_counter -= 1;
-      base::debug::DumpWithoutCrashing();
-    }
-    return gfx::GpuMemoryBufferHandle();
-  }
+  handle.io_surface = io_surface;
 
   {
     base::AutoLock lock(io_surfaces_lock_);
@@ -127,8 +105,8 @@ GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer(
     IOSurfaceMap::iterator it = io_surfaces_.find(key);
     if (it != io_surfaces_.end())
       io_surface = it->second;
-  } else if (handle.mach_port) {
-    io_surface.reset(IOSurfaceLookupFromMachPort(handle.mach_port.get()));
+  } else if (handle.io_surface) {
+    io_surface = handle.io_surface;
     if (!io_surface) {
       DLOG(ERROR) << "Failed to open IOSurface from handle.";
       return nullptr;
@@ -181,28 +159,6 @@ GpuMemoryBufferFactoryIOSurface::CreateAnonymousImage(
     return nullptr;
   }
 
-  // This IOSurface does not require passing via a mach port, but attempt to
-  // locally open via a mach port to gather data to include in a Radar about
-  // this failure.
-  // https://crbug.com/795649
-  gfx::ScopedRefCountedIOSurfaceMachPort mach_port(
-      IOSurfaceCreateMachPort(io_surface));
-  if (mach_port) {
-    base::ScopedCFTypeRef<IOSurfaceRef> io_surface_from_mach_port(
-        IOSurfaceLookupFromMachPort(mach_port.get()));
-    if (!io_surface_from_mach_port) {
-      LOG(ERROR) << "Failed to locally open anonymous IOSurface mach port "
-                    "(ignoring failure).";
-      static int dump_counter = kMaxCrashDumps;
-      if (dump_counter) {
-        dump_counter -= 1;
-        base::debug::DumpWithoutCrashing();
-      }
-    }
-  } else {
-    LOG(ERROR) << "Failed to create IOSurface mach port.";
-  }
-
   unsigned internalformat = gl::BufferFormatToGLInternalFormat(format);
   scoped_refptr<gl::GLImageIOSurface> image(
       gl::GLImageIOSurface::Create(size, internalformat));

media/base/mac/video_frame_mac.cc

@@ -48,8 +48,7 @@ WrapVideoFrameInCVPixelBuffer(const VideoFrame& frame) {
     gfx::GpuMemoryBufferHandle handle =
         frame.GetGpuMemoryBuffer()->CloneHandle();
     if (handle.type == gfx::GpuMemoryBufferType::IO_SURFACE_BUFFER) {
-      base::ScopedCFTypeRef<IOSurfaceRef> io_surface =
-          gfx::IOSurfaceMachPortToIOSurface(std::move(handle.mach_port));
+      gfx::ScopedIOSurface io_surface = handle.io_surface;
       if (io_surface) {
         const CVReturn cv_return = CVPixelBufferCreateWithIOSurface(
             nullptr, io_surface, nullptr, pixel_buffer.InitializeInto());

media/base/video_frame.cc

@@ -697,8 +697,7 @@ scoped_refptr<VideoFrame> VideoFrame::WrapIOSurface(
     DLOG(ERROR) << "Non-IOSurface handle.";
     return nullptr;
   }
-  base::ScopedCFTypeRef<IOSurfaceRef> io_surface =
-      gfx::IOSurfaceMachPortToIOSurface(std::move(handle.mach_port));
+  gfx::ScopedIOSurface io_surface = handle.io_surface;
   if (!io_surface) {
     return nullptr;
   }

media/capture/video/mac/video_capture_device_avfoundation_mac.mm

@@ -606,9 +606,7 @@ AVCaptureDeviceFormat* FindBestCaptureFormat(
   gfx::GpuMemoryBufferHandle handle;
   handle.id.id = -1;
   handle.type = gfx::GpuMemoryBufferType::IO_SURFACE_BUFFER;
-  handle.mach_port.reset(IOSurfaceCreateMachPort(ioSurface));
-  if (!handle.mach_port)
-    return NO;
+  handle.io_surface.reset(ioSurface, base::scoped_policy::RETAIN);
   _lock.AssertAcquired();
   _frameReceiver->ReceiveExternalGpuMemoryBufferFrame(
       std::move(handle),

ui/gfx/gpu_memory_buffer.cc

@@ -37,7 +37,7 @@ GpuMemoryBufferHandle GpuMemoryBufferHandle::Clone() const {
 #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_FUCHSIA)
   handle.native_pixmap_handle = CloneHandleForIPC(native_pixmap_handle);
 #elif defined(OS_MAC)
-  handle.mach_port = mach_port;
+  handle.io_surface = io_surface;
 #elif defined(OS_WIN)
   NOTIMPLEMENTED();
 #elif defined(OS_ANDROID)

ui/gfx/gpu_memory_buffer.h

@@ -72,7 +72,7 @@ struct GFX_EXPORT GpuMemoryBufferHandle {
 #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_FUCHSIA)
   NativePixmapHandle native_pixmap_handle;
 #elif defined(OS_MAC)
-  ScopedRefCountedIOSurfaceMachPort mach_port;
+  gfx::ScopedIOSurface io_surface;
 #elif defined(OS_WIN)
   base::win::ScopedHandle dxgi_handle;
 #elif defined(OS_ANDROID)

ui/gfx/ipc/BUILD.gn

@@ -22,4 +22,9 @@ component("ipc") {
     "//ui/gfx/ipc/geometry",
     "//ui/gfx/range",
   ]
+
+  frameworks = [
+    "CoreFoundation.framework",
+    "IOSurface.framework",
+  ]
 }

ui/gfx/ipc/gfx_param_traits.cc

@@ -63,6 +63,39 @@ void ParamTraits<gfx::ScopedRefCountedIOSurfaceMachPort>::Log(
   l->append("IOSurface Mach send right: ");
   LogParam(p.get(), l);
 }
+
+void ParamTraits<gfx::ScopedIOSurface>::Write(base::Pickle* m,
+                                              const param_type p) {
+  gfx::ScopedRefCountedIOSurfaceMachPort io_surface_mach_port(
+      IOSurfaceCreateMachPort(p.get()));
+  MachPortMac mach_port_mac(io_surface_mach_port.get());
+  ParamTraits<MachPortMac>::Write(m, mach_port_mac);
+}
+
+bool ParamTraits<gfx::ScopedIOSurface>::Read(const base::Pickle* m,
+                                             base::PickleIterator* iter,
+                                             param_type* r) {
+  MachPortMac mach_port_mac;
+  if (!ParamTraits<MachPortMac>::Read(m, iter, &mach_port_mac))
+    return false;
+  gfx::ScopedRefCountedIOSurfaceMachPort io_surface_mach_port(
+      mach_port_mac.get_mach_port());
+  if (io_surface_mach_port)
+    r->reset(IOSurfaceLookupFromMachPort(io_surface_mach_port.get()));
+  else
+    r->reset();
+  return true;
+}
+
+void ParamTraits<gfx::ScopedIOSurface>::Log(const param_type& p,
+                                            std::string* l) {
+  l->append("IOSurface(");
+  if (p) {
+    uint32_t io_surface_id = IOSurfaceGetID(p.get());
+    LogParam(io_surface_id, l);
+  }
+  l->append(")");
+}
 #endif  // defined(OS_MAC)
 
 void ParamTraits<gfx::SelectionBound>::Write(base::Pickle* m,

ui/gfx/ipc/gfx_param_traits.h

@@ -48,6 +48,16 @@ struct GFX_IPC_EXPORT ParamTraits<gfx::ScopedRefCountedIOSurfaceMachPort> {
                    param_type* r);
   static void Log(const param_type& p, std::string* l);
 };
+
+template <>
+struct GFX_IPC_EXPORT ParamTraits<gfx::ScopedIOSurface> {
+  typedef gfx::ScopedIOSurface param_type;
+  static void Write(base::Pickle* m, const param_type p);
+  static bool Read(const base::Pickle* m,
+                   base::PickleIterator* iter,
+                   param_type* r);
+  static void Log(const param_type& p, std::string* l);
+};
 #endif  // defined(OS_MAC)
 
 template <>

ui/gfx/ipc/gfx_param_traits_macros.h

@@ -51,7 +51,7 @@ IPC_STRUCT_TRAITS_BEGIN(gfx::GpuMemoryBufferHandle)
 #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_FUCHSIA)
   IPC_STRUCT_TRAITS_MEMBER(native_pixmap_handle)
 #elif defined(OS_APPLE)
-  IPC_STRUCT_TRAITS_MEMBER(mach_port)
+  IPC_STRUCT_TRAITS_MEMBER(io_surface)
 #elif defined(OS_WIN)
   IPC_STRUCT_TRAITS_MEMBER(dxgi_handle)
 #elif defined(OS_ANDROID)

ui/gfx/mac/io_surface.h

@@ -62,6 +62,9 @@ using ScopedRefCountedIOSurfaceMachPort =
 using ScopedInUseIOSurface =
     base::ScopedTypeRef<IOSurfaceRef, internal::ScopedInUseIOSurfaceTraits>;
 
+// A scoper for holding a reference to an IOSurface.
+using ScopedIOSurface = base::ScopedCFTypeRef<IOSurfaceRef>;
+
 // Return true if there exists a value for IOSurfaceColorSpace or
 // IOSurfaceICCProfile that will make CoreAnimation render using |color_space|.
 GFX_EXPORT bool IOSurfaceCanSetColorSpace(const gfx::ColorSpace& color_space);

ui/gfx/mojom/BUILD.gn

@@ -300,4 +300,8 @@ component("shared_mojom_traits") {
     ":native_handle_types",
     "//ui/gfx",
   ]
+  frameworks = [
+    "CoreFoundation.framework",
+    "IOSurface.framework",
+  ]
 }

ui/gfx/mojom/buffer_types_mojom_traits.cc

@@ -39,14 +39,17 @@ gfx::mojom::GpuMemoryBufferPlatformHandlePtr StructTraits<
 #else
       break;
 #endif
-    case gfx::IO_SURFACE_BUFFER:
+    case gfx::IO_SURFACE_BUFFER: {
 #if defined(OS_MAC)
+      gfx::ScopedRefCountedIOSurfaceMachPort io_surface_mach_port(
+          IOSurfaceCreateMachPort(handle.io_surface.get()));
       return gfx::mojom::GpuMemoryBufferPlatformHandle::NewMachPort(
           mojo::PlatformHandle(
-              base::mac::RetainMachSendRight(handle.mach_port.get())));
+              base::mac::RetainMachSendRight(io_surface_mach_port.get())));
 #else
       break;
 #endif
+    }
     case gfx::DXGI_SHARED_HANDLE:
 #if defined(OS_WIN)
       DCHECK(handle.dxgi_handle.IsValid());
@@ -121,8 +124,14 @@ bool StructTraits<gfx::mojom::GpuMemoryBufferHandleDataView,
       out->type = gfx::IO_SURFACE_BUFFER;
       if (!platform_handle->get_mach_port().is_mach_send())
         return false;
-      out->mach_port.reset(
+      gfx::ScopedRefCountedIOSurfaceMachPort io_surface_mach_port(
           platform_handle->get_mach_port().ReleaseMachSendRight());
+      if (io_surface_mach_port) {
+        out->io_surface.reset(
+            IOSurfaceLookupFromMachPort(io_surface_mach_port.get()));
+      } else {
+        out->io_surface.reset();
+      }
       return true;
     }
 #elif defined(OS_WIN)