Bug 1918778 - Correct ownership/recycling issues with MacIOSurface. r=bradwerth

This patch corrects a few different issues related to recycling
MacIOSurface objects.

1) When recycling a surface, we must check that the cached surfaces
   match all of the requested parameters, not just the size. If we do
   not, we should just flush the whole cache immediately since they
   should all be created with the same parameters.

2) Allocations can fail, and we should check for failing to get a
   surface from the allocator and fall back if so.

3) Locking can fail, and we should check that return value at all of the
   call sites.

This may help resolve a number of otherwise difficult to understand
crash signatures. It may also solve display corruption issues in rare
cases where the parameters that changed were roughly equivalent such
that everything appears to work, but they differ enough to change the
presentation.

Differential Revision: https://phabricator.services.mozilla.com/D222205

gfx/2d/MacIOSurface.cpp

@@ -127,7 +127,7 @@ size_t CreatePlaneDictionary(CFTypeRefPtr<CFMutableDictionaryRef>& aDict,
 }
 
 // Helper function to set common color IOSurface properties.
-void SetIOSurfaceCommonProperties(
+static void SetIOSurfaceCommonProperties(
     CFTypeRefPtr<IOSurfaceRef> surfaceRef,
     MacIOSurface::YUVColorSpace aColorSpace,
     MacIOSurface::TransferFunction aTransferFunction) {
@@ -394,11 +394,16 @@ void MacIOSurface::DecrementUseCount() {
   ::IOSurfaceDecrementUseCount(mIOSurfaceRef.get());
 }
 
-void MacIOSurface::Lock(bool aReadOnly) {
+bool MacIOSurface::Lock(bool aReadOnly) {
   MOZ_RELEASE_ASSERT(!mIsLocked, "double MacIOSurface lock");
-  ::IOSurfaceLock(mIOSurfaceRef.get(), aReadOnly ? kIOSurfaceLockReadOnly : 0,
+  kern_return_t rv = ::IOSurfaceLock(
-                  nullptr);
+      mIOSurfaceRef.get(), aReadOnly ? kIOSurfaceLockReadOnly : 0, nullptr);
+  if (NS_WARN_IF(rv != KERN_SUCCESS)) {
+    gfxCriticalNoteOnce << "MacIOSurface::Lock failed " << gfx::hexa(rv);
+    return false;
+  }
   mIsLocked = true;
+  return true;
 }
 
 void MacIOSurface::Unlock(bool aReadOnly) {
@@ -420,14 +425,22 @@ static void MacIOSurfaceBufferDeallocator(void* aClosure) {
 }
 
 already_AddRefed<SourceSurface> MacIOSurface::GetAsSurface() {
-  Lock();
+  if (NS_WARN_IF(!Lock())) {
+    return nullptr;
+  }
+
   size_t bytesPerRow = GetBytesPerRow();
   size_t ioWidth = GetDevicePixelWidth();
   size_t ioHeight = GetDevicePixelHeight();
 
   unsigned char* ioData = (unsigned char*)GetBaseAddress();
-  auto* dataCpy =
+  auto* dataCpy = new (
-      new unsigned char[bytesPerRow * ioHeight / sizeof(unsigned char)];
+      fallible) unsigned char[bytesPerRow * ioHeight / sizeof(unsigned char)];
+  if (NS_WARN_IF(!dataCpy)) {
+    Unlock();
+    return nullptr;
+  }
+
   for (size_t i = 0; i < ioHeight; i++) {
     memcpy(dataCpy + i * bytesPerRow, ioData + i * bytesPerRow, ioWidth * 4);
   }
@@ -502,6 +515,68 @@ ColorDepth MacIOSurface::GetColorDepth() const {
   }
 }
 
+/* static */ Maybe<OSType> MacIOSurface::ChoosePixelFormat(
+    ChromaSubsampling aChromaSubsampling, ColorRange aColorRange,
+    ColorDepth aColorDepth) {
+  switch (aChromaSubsampling) {
+    case ChromaSubsampling::FULL:
+      if (aColorDepth == ColorDepth::COLOR_10) {
+        switch (aColorRange) {
+          case ColorRange::LIMITED:
+            return Some(kCVPixelFormatType_422YpCbCr10BiPlanarVideoRange);
+          case ColorRange::FULL:
+            return Some(kCVPixelFormatType_422YpCbCr10BiPlanarFullRange);
+        }
+      }
+      break;
+    case ChromaSubsampling::HALF_WIDTH:
+      switch (aColorDepth) {
+        case ColorDepth::COLOR_8:
+          switch (aColorRange) {
+            case ColorRange::LIMITED:
+              return Some(kCVPixelFormatType_422YpCbCr8_yuvs);
+            case ColorRange::FULL:
+              return Some(kCVPixelFormatType_422YpCbCr8FullRange);
+          }
+          break;
+        case ColorDepth::COLOR_10:
+          switch (aColorRange) {
+            case ColorRange::LIMITED:
+              return Some(kCVPixelFormatType_422YpCbCr10BiPlanarVideoRange);
+            case ColorRange::FULL:
+              return Some(kCVPixelFormatType_422YpCbCr10BiPlanarFullRange);
+          }
+          break;
+        default:
+          break;
+      }
+      break;
+    case ChromaSubsampling::HALF_WIDTH_AND_HEIGHT:
+      switch (aColorDepth) {
+        case ColorDepth::COLOR_8:
+          switch (aColorRange) {
+            case ColorRange::LIMITED:
+              return Some(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange);
+            case ColorRange::FULL:
+              return Some(kCVPixelFormatType_420YpCbCr8BiPlanarFullRange);
+          }
+          break;
+        case ColorDepth::COLOR_10:
+        case ColorDepth::COLOR_12:
+        case ColorDepth::COLOR_16:
+          switch (aColorRange) {
+            case ColorRange::LIMITED:
+              return Some(kCVPixelFormatType_420YpCbCr10BiPlanarVideoRange);
+            case ColorRange::FULL:
+              return Some(kCVPixelFormatType_420YpCbCr10BiPlanarFullRange);
+          }
+          break;
+      }
+      break;
+  }
+  return Nothing();
+}
+
 bool MacIOSurface::BindTexImage(mozilla::gl::GLContext* aGL, size_t aPlane,
                                 mozilla::gfx::SurfaceFormat* aOutReadFormat) {
 #ifdef XP_MACOSX

gfx/2d/MacIOSurface.h

@@ -13,6 +13,7 @@
 #  include <dlfcn.h>
 
 #  include "mozilla/gfx/Types.h"
+#  include "mozilla/Maybe.h"
 #  include "CFTypeRefPtr.h"
 
 namespace mozilla {
@@ -99,7 +100,7 @@ class MacIOSurface final
   size_t GetDevicePixelHeight(size_t plane = 0) const;
   size_t GetBytesPerRow(size_t plane = 0) const;
   size_t GetAllocSize() const;
-  void Lock(bool aReadOnly = true);
+  bool Lock(bool aReadOnly = true);
   void Unlock(bool aReadOnly = true);
   bool IsLocked() const { return mIsLocked; }
   void IncrementUseCount();
@@ -149,6 +150,11 @@ class MacIOSurface final
 
   static size_t GetMaxWidth();
   static size_t GetMaxHeight();
+  static mozilla::Maybe<OSType> ChoosePixelFormat(
+      mozilla::gfx::ChromaSubsampling aChromaSubsampling,
+      mozilla::gfx::ColorRange aColorRange,
+      mozilla::gfx::ColorDepth aColorDepth);
+
   CFTypeRefPtr<IOSurfaceRef> GetIOSurfaceRef() { return mIOSurfaceRef; }
 
   void SetColorSpace(mozilla::gfx::ColorSpace2) const;

gfx/layers/MacIOSurfaceHelpers.cpp

@@ -158,7 +158,10 @@ static nsresult CopyFromLockedMacIOSurface(MacIOSurface* aSurface,
 
 already_AddRefed<SourceSurface> CreateSourceSurfaceFromMacIOSurface(
     MacIOSurface* aSurface, gfx::DataSourceSurface* aDataSurface) {
-  aSurface->Lock();
+  if (NS_WARN_IF(!aSurface->Lock())) {
+    return nullptr;
+  }
+
   auto scopeExit = MakeScopeExit([&]() { aSurface->Unlock(); });
 
   size_t ioWidth = aSurface->GetDevicePixelWidth();
@@ -206,7 +209,10 @@ nsresult CreateSurfaceDescriptorBufferFromMacIOSurface(
     MacIOSurface* aSurface, SurfaceDescriptorBuffer& aSdBuffer,
     Image::BuildSdbFlags aFlags,
     const std::function<MemoryOrShmem(uint32_t)>& aAllocate) {
-  aSurface->Lock();
+  if (NS_WARN_IF(!aSurface->Lock())) {
+    return NS_ERROR_FAILURE;
+  }
+
   auto scopeExit = MakeScopeExit([&]() { aSurface->Unlock(); });
 
   size_t ioWidth = aSurface->GetDevicePixelWidth();

gfx/layers/MacIOSurfaceImage.cpp

@@ -91,7 +91,9 @@ bool MacIOSurfaceImage::SetData(ImageContainer* aContainer,
       ySize, cbcrSize, aData.mChromaSubsampling, aData.mYUVColorSpace,
       aData.mTransferFunction, aData.mColorRange, aData.mColorDepth);
 
-  surf->Lock(false);
+  if (NS_WARN_IF(!surf) || NS_WARN_IF(!surf->Lock(false))) {
+    return false;
+  }
 
   if (surf->GetFormat() == SurfaceFormat::YUY2) {
     // If the CbCrSize's height is half of the YSize's height, then we'll
@@ -264,49 +266,75 @@ already_AddRefed<MacIOSurface> MacIOSurfaceRecycleAllocator::Allocate(
     gfx::ChromaSubsampling aChromaSubsampling,
     gfx::YUVColorSpace aYUVColorSpace, gfx::TransferFunction aTransferFunction,
     gfx::ColorRange aColorRange, gfx::ColorDepth aColorDepth) {
-  nsTArray<CFTypeRefPtr<IOSurfaceRef>> surfaces = std::move(mSurfaces);
+  // To avoid checking every property of every surface, we just cache the
-  RefPtr<MacIOSurface> result;
+  // parameters used during the last allocation. If any of these have changed,
-  for (auto& surf : surfaces) {
+  // dump the cached surfaces and update our cached parameters.
-    // If the surface size has changed, then discard any surfaces of the old
+  if (mYSize != aYSize || mCbCrSize != aCbCrSize ||
-    // size.
+      mChromaSubsampling != aChromaSubsampling ||
-    if (::IOSurfaceGetWidthOfPlane(surf.get(), 0) != (size_t)aYSize.width ||
+      mYUVColorSpace != aYUVColorSpace ||
-        ::IOSurfaceGetHeightOfPlane(surf.get(), 0) != (size_t)aYSize.height) {
+      mTransferFunction != aTransferFunction || mColorRange != aColorRange ||
+      mColorDepth != aColorDepth) {
+    mSurfaces.Clear();
+    mYSize = aYSize;
+    mCbCrSize = aCbCrSize;
+    mChromaSubsampling = aChromaSubsampling;
+    mYUVColorSpace = aYUVColorSpace;
+    mTransferFunction = aTransferFunction;
+    mColorRange = aColorRange;
+    mColorDepth = aColorDepth;
+  }
+
+  // Scan for an unused surface, and reuse that if one is available.
+  for (auto& surf : mSurfaces) {
+    if (::IOSurfaceIsInUse(surf.get())) {
       continue;
     }
 
-    // Only construct a MacIOSurface object when we find one that isn't
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
-    // in-use, since the constructor adds a usage ref.
+    Maybe<OSType> pixelFormat = MacIOSurface::ChoosePixelFormat(
-    if (!result && !::IOSurfaceIsInUse(surf.get())) {
+        aChromaSubsampling, aColorRange, aColorDepth);
-      result = new MacIOSurface(surf, false, aYUVColorSpace);
+    MOZ_DIAGNOSTIC_ASSERT(pixelFormat.isSome());
+    MOZ_DIAGNOSTIC_ASSERT(::IOSurfaceGetPixelFormat(surf.get()) ==
+                          *pixelFormat);
+    MOZ_DIAGNOSTIC_ASSERT(::IOSurfaceGetWidthOfPlane(surf.get(), 0) ==
+                          (size_t)aYSize.width);
+    MOZ_DIAGNOSTIC_ASSERT(::IOSurfaceGetHeightOfPlane(surf.get(), 0) ==
+                          (size_t)aYSize.height);
+    if (*pixelFormat != kCVPixelFormatType_422YpCbCr8_yuvs &&
+        *pixelFormat != kCVPixelFormatType_422YpCbCr8FullRange) {
+      MOZ_DIAGNOSTIC_ASSERT(::IOSurfaceGetWidthOfPlane(surf.get(), 1) ==
+                            (size_t)aCbCrSize.width);
+      MOZ_DIAGNOSTIC_ASSERT(::IOSurfaceGetHeightOfPlane(surf.get(), 1) ==
+                            (size_t)aCbCrSize.height);
     }
+#endif
 
-    mSurfaces.AppendElement(surf);
+    return MakeAndAddRef<MacIOSurface>(surf, false, aYUVColorSpace);
   }
 
-  if (!result) {
+  // Time to decide if we are creating a single planar or bi-planar surface.
-    // Time to decide if we are creating a single planar or bi-planar surface.
+  // We limit ourselves to macOS's single planar and bi-planar formats for
-    // We limit ourselves to macOS's single planar and bi-planar formats for
+  // simplicity reasons, possibly gaining some small memory or performance
-    // simplicity reasons, possibly gaining some small memory or performance
+  // benefit relative to the tri-planar formats. We try and use as few
-    // benefit relative to the tri-planar formats. We try and use as few
+  // planes as possible.
-    // planes as possible.
+  // 4:2:0 formats are always bi-planar, because there is no 4:2:0 single
-    // 4:2:0 formats are always bi-planar, because there is no 4:2:0 single
+  // planar format.
-    // planar format.
+  // 4:2:2 formats with 8 bit color are single planar, otherwise bi-planar.
-    // 4:2:2 formats with 8 bit color are single planar, otherwise bi-planar.
 
-    if (aChromaSubsampling == gfx::ChromaSubsampling::HALF_WIDTH &&
+  RefPtr<MacIOSurface> result;
-        aColorDepth == gfx::ColorDepth::COLOR_8) {
+  if (aChromaSubsampling == gfx::ChromaSubsampling::HALF_WIDTH &&
-      result = MacIOSurface::CreateSinglePlanarSurface(
+      aColorDepth == gfx::ColorDepth::COLOR_8) {
-          aYSize, aYUVColorSpace, aTransferFunction, aColorRange);
+    result = MacIOSurface::CreateSinglePlanarSurface(
-    } else {
+        aYSize, aYUVColorSpace, aTransferFunction, aColorRange);
-      result = MacIOSurface::CreateBiPlanarSurface(
+  } else {
-          aYSize, aCbCrSize, aChromaSubsampling, aYUVColorSpace,
+    result = MacIOSurface::CreateBiPlanarSurface(
-          aTransferFunction, aColorRange, aColorDepth);
+        aYSize, aCbCrSize, aChromaSubsampling, aYUVColorSpace,
-    }
+        aTransferFunction, aColorRange, aColorDepth);
+  }
 
-    if (mSurfaces.Length() <
+  if (result &&
-        StaticPrefs::layers_iosurfaceimage_recycle_limit()) {
+      mSurfaces.Length() < StaticPrefs::layers_iosurfaceimage_recycle_limit()) {
-      mSurfaces.AppendElement(result->GetIOSurfaceRef());
+    mSurfaces.AppendElement(result->GetIOSurfaceRef());
-    }
   }
 
   return result.forget();

gfx/layers/MacIOSurfaceImage.h

@@ -71,6 +71,15 @@ class MacIOSurfaceRecycleAllocator {
   ~MacIOSurfaceRecycleAllocator() = default;
 
   nsTArray<CFTypeRefPtr<IOSurfaceRef>> mSurfaces;
+
+  // Cached parameters used for allocations stored in mSurfaces.
+  gfx::IntSize mYSize;
+  gfx::IntSize mCbCrSize;
+  gfx::ChromaSubsampling mChromaSubsampling = gfx::ChromaSubsampling::FULL;
+  gfx::YUVColorSpace mYUVColorSpace = gfx::YUVColorSpace::BT709;
+  gfx::TransferFunction mTransferFunction = gfx::TransferFunction::BT709;
+  gfx::ColorRange mColorRange = gfx::ColorRange::FULL;
+  gfx::ColorDepth mColorDepth = gfx::ColorDepth::COLOR_8;
 };
 
 }  // namespace layers

gfx/layers/NativeLayerCA.mm

@@ -1198,22 +1198,23 @@ void NativeLayerCA::DumpLayer(std::ostream& aOutputStream) {
     // Attempt to render the surface as a PNG. Skia can do this for RGB
     // surfaces.
     RefPtr<MacIOSurface> surf = new MacIOSurface(surface);
-    surf->Lock(true);
+    if (surf->Lock(true)) {
-    SurfaceFormat format = surf->GetFormat();
+      SurfaceFormat format = surf->GetFormat();
-    if (format == SurfaceFormat::B8G8R8A8 ||
+      if (format == SurfaceFormat::B8G8R8A8 ||
-        format == SurfaceFormat::B8G8R8X8) {
+          format == SurfaceFormat::B8G8R8X8) {
-      RefPtr<gfx::DrawTarget> dt =
+        RefPtr<gfx::DrawTarget> dt =
-          surf->GetAsDrawTargetLocked(gfx::BackendType::SKIA);
+            surf->GetAsDrawTargetLocked(gfx::BackendType::SKIA);
-      if (dt) {
+        if (dt) {
-        RefPtr<gfx::SourceSurface> sourceSurf = dt->Snapshot();
+          RefPtr<gfx::SourceSurface> sourceSurf = dt->Snapshot();
-        nsCString dataUrl;
+          nsCString dataUrl;
-        gfxUtils::EncodeSourceSurface(sourceSurf, ImageType::PNG, u""_ns,
+          gfxUtils::EncodeSourceSurface(sourceSurf, ImageType::PNG, u""_ns,
-                                      gfxUtils::eDataURIEncode, nullptr,
+                                        gfxUtils::eDataURIEncode, nullptr,
-                                      &dataUrl);
+                                        &dataUrl);
-        aOutputStream << dataUrl.get();
+          aOutputStream << dataUrl.get();
+        }
       }
+      surf->Unlock(true);
     }
-    surf->Unlock(true);
   }
 
   aOutputStream << "\"/></div>\n";
@@ -1326,8 +1327,13 @@ RefPtr<gfx::DrawTarget> NativeLayerCA::NextSurfaceAsDrawTarget(
     return nullptr;
   }
 
-  mInProgressLockedIOSurface = new MacIOSurface(mInProgressSurface->mSurface);
+  auto surf = MakeRefPtr<MacIOSurface>(mInProgressSurface->mSurface);
-  mInProgressLockedIOSurface->Lock(false);
+  if (NS_WARN_IF(!surf->Lock(false))) {
+    gfxCriticalError() << "NextSurfaceAsDrawTarget lock surface failed.";
+    return nullptr;
+  }
+
+  mInProgressLockedIOSurface = std::move(surf);
   RefPtr<gfx::DrawTarget> dt =
       mInProgressLockedIOSurface->GetAsDrawTargetLocked(aBackendType);
 
@@ -1336,8 +1342,7 @@ RefPtr<gfx::DrawTarget> NativeLayerCA::NextSurfaceAsDrawTarget(
       [&](CFTypeRefPtr<IOSurfaceRef> validSource,
           const gfx::IntRegion& copyRegion) {
         RefPtr<MacIOSurface> source = new MacIOSurface(validSource);
-        source->Lock(true);
+        if (source->Lock(true)) {
-        {
           RefPtr<gfx::DrawTarget> sourceDT =
               source->GetAsDrawTargetLocked(aBackendType);
           RefPtr<gfx::SourceSurface> sourceSurface = sourceDT->Snapshot();
@@ -1346,8 +1351,10 @@ RefPtr<gfx::DrawTarget> NativeLayerCA::NextSurfaceAsDrawTarget(
             const gfx::IntRect& r = iter.Get();
             dt->CopySurface(sourceSurface, r, r.TopLeft());
           }
+          source->Unlock(true);
+        } else {
+          gfxCriticalError() << "HandlePartialUpdate lock surface failed.";
         }
-        source->Unlock(true);
       });
 
   return dt;

gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp

@@ -73,10 +73,7 @@ void MacIOSurfaceTextureData::FillInfo(TextureData::Info& aInfo) const {
   aInfo.canExposeMappedData = false;
 }
 
-bool MacIOSurfaceTextureData::Lock(OpenMode) {
+bool MacIOSurfaceTextureData::Lock(OpenMode) { return mSurface->Lock(false); }
-  mSurface->Lock(false);
-  return true;
-}
 
 void MacIOSurfaceTextureData::Unlock() { mSurface->Unlock(false); }
 

gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp

@@ -147,7 +147,9 @@ bool RenderMacIOSurfaceTextureHost::MapPlane(RenderCompositor* aCompositor,
                                              uint8_t aChannelIndex,
                                              PlaneInfo& aPlaneInfo) {
   if (!aChannelIndex) {
-    mSurface->Lock();
+    if (NS_WARN_IF(!mSurface->Lock())) {
+      return false;
+    }
   }
   aPlaneInfo.mData = mSurface->GetBaseAddressOfPlane(aChannelIndex);
   aPlaneInfo.mStride = mSurface->GetBytesPerRow(aChannelIndex);