From 13ec012990a24fc16c2e3fd7ec4c2061ee35284d Mon Sep 17 00:00:00 2001 From: Andrew Osmond Date: Sat, 14 Sep 2024 16:23:54 +0000 Subject: [PATCH] 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 | 89 +++++++++++++++-- gfx/2d/MacIOSurface.h | 8 +- gfx/layers/MacIOSurfaceHelpers.cpp | 10 +- gfx/layers/MacIOSurfaceImage.cpp | 98 ++++++++++++------- gfx/layers/MacIOSurfaceImage.h | 9 ++ gfx/layers/NativeLayerCA.mm | 45 +++++---- .../opengl/MacIOSurfaceTextureClientOGL.cpp | 5 +- .../RenderMacIOSurfaceTextureHost.cpp | 4 +- 8 files changed, 199 insertions(+), 69 deletions(-) diff --git a/gfx/2d/MacIOSurface.cpp b/gfx/2d/MacIOSurface.cpp index 24f6be32be4e..ea64a3264c26 100644 --- a/gfx/2d/MacIOSurface.cpp +++ b/gfx/2d/MacIOSurface.cpp @@ -127,7 +127,7 @@ size_t CreatePlaneDictionary(CFTypeRefPtr& aDict, } // Helper function to set common color IOSurface properties. -void SetIOSurfaceCommonProperties( +static void SetIOSurfaceCommonProperties( CFTypeRefPtr 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, - nullptr); + kern_return_t rv = ::IOSurfaceLock( + 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 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 = - new unsigned char[bytesPerRow * ioHeight / sizeof(unsigned char)]; + auto* dataCpy = new ( + 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 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 diff --git a/gfx/2d/MacIOSurface.h b/gfx/2d/MacIOSurface.h index 4309e154a877..f0e454adf55c 100644 --- a/gfx/2d/MacIOSurface.h +++ b/gfx/2d/MacIOSurface.h @@ -13,6 +13,7 @@ # include # 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 ChoosePixelFormat( + mozilla::gfx::ChromaSubsampling aChromaSubsampling, + mozilla::gfx::ColorRange aColorRange, + mozilla::gfx::ColorDepth aColorDepth); + CFTypeRefPtr GetIOSurfaceRef() { return mIOSurfaceRef; } void SetColorSpace(mozilla::gfx::ColorSpace2) const; diff --git a/gfx/layers/MacIOSurfaceHelpers.cpp b/gfx/layers/MacIOSurfaceHelpers.cpp index b551d5e8dfdd..fc389b80c16c 100644 --- a/gfx/layers/MacIOSurfaceHelpers.cpp +++ b/gfx/layers/MacIOSurfaceHelpers.cpp @@ -158,7 +158,10 @@ static nsresult CopyFromLockedMacIOSurface(MacIOSurface* aSurface, already_AddRefed 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& aAllocate) { - aSurface->Lock(); + if (NS_WARN_IF(!aSurface->Lock())) { + return NS_ERROR_FAILURE; + } + auto scopeExit = MakeScopeExit([&]() { aSurface->Unlock(); }); size_t ioWidth = aSurface->GetDevicePixelWidth(); diff --git a/gfx/layers/MacIOSurfaceImage.cpp b/gfx/layers/MacIOSurfaceImage.cpp index 2c98df94bf37..ed1a7413a8a6 100644 --- a/gfx/layers/MacIOSurfaceImage.cpp +++ b/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 MacIOSurfaceRecycleAllocator::Allocate( gfx::ChromaSubsampling aChromaSubsampling, gfx::YUVColorSpace aYUVColorSpace, gfx::TransferFunction aTransferFunction, gfx::ColorRange aColorRange, gfx::ColorDepth aColorDepth) { - nsTArray> surfaces = std::move(mSurfaces); - RefPtr result; - for (auto& surf : surfaces) { - // If the surface size has changed, then discard any surfaces of the old - // size. - if (::IOSurfaceGetWidthOfPlane(surf.get(), 0) != (size_t)aYSize.width || - ::IOSurfaceGetHeightOfPlane(surf.get(), 0) != (size_t)aYSize.height) { + // To avoid checking every property of every surface, we just cache the + // parameters used during the last allocation. If any of these have changed, + // dump the cached surfaces and update our cached parameters. + if (mYSize != aYSize || mCbCrSize != aCbCrSize || + mChromaSubsampling != aChromaSubsampling || + mYUVColorSpace != aYUVColorSpace || + 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 - // in-use, since the constructor adds a usage ref. - if (!result && !::IOSurfaceIsInUse(surf.get())) { - result = new MacIOSurface(surf, false, aYUVColorSpace); +#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED + Maybe pixelFormat = MacIOSurface::ChoosePixelFormat( + aChromaSubsampling, aColorRange, aColorDepth); + 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(surf, false, aYUVColorSpace); } - if (!result) { - // 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 - // simplicity reasons, possibly gaining some small memory or performance - // benefit relative to the tri-planar formats. We try and use as few - // planes as possible. - // 4:2:0 formats are always bi-planar, because there is no 4:2:0 single - // planar format. - // 4:2:2 formats with 8 bit color are single planar, otherwise bi-planar. + // 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 + // simplicity reasons, possibly gaining some small memory or performance + // benefit relative to the tri-planar formats. We try and use as few + // planes as possible. + // 4:2:0 formats are always bi-planar, because there is no 4:2:0 single + // planar format. + // 4:2:2 formats with 8 bit color are single planar, otherwise bi-planar. - if (aChromaSubsampling == gfx::ChromaSubsampling::HALF_WIDTH && - aColorDepth == gfx::ColorDepth::COLOR_8) { - result = MacIOSurface::CreateSinglePlanarSurface( - aYSize, aYUVColorSpace, aTransferFunction, aColorRange); - } else { - result = MacIOSurface::CreateBiPlanarSurface( - aYSize, aCbCrSize, aChromaSubsampling, aYUVColorSpace, - aTransferFunction, aColorRange, aColorDepth); - } + RefPtr result; + if (aChromaSubsampling == gfx::ChromaSubsampling::HALF_WIDTH && + aColorDepth == gfx::ColorDepth::COLOR_8) { + result = MacIOSurface::CreateSinglePlanarSurface( + aYSize, aYUVColorSpace, aTransferFunction, aColorRange); + } else { + result = MacIOSurface::CreateBiPlanarSurface( + aYSize, aCbCrSize, aChromaSubsampling, aYUVColorSpace, + aTransferFunction, aColorRange, aColorDepth); + } - if (mSurfaces.Length() < - StaticPrefs::layers_iosurfaceimage_recycle_limit()) { - mSurfaces.AppendElement(result->GetIOSurfaceRef()); - } + if (result && + mSurfaces.Length() < StaticPrefs::layers_iosurfaceimage_recycle_limit()) { + mSurfaces.AppendElement(result->GetIOSurfaceRef()); } return result.forget(); diff --git a/gfx/layers/MacIOSurfaceImage.h b/gfx/layers/MacIOSurfaceImage.h index 32279b2287b5..fb2d8737c4da 100644 --- a/gfx/layers/MacIOSurfaceImage.h +++ b/gfx/layers/MacIOSurfaceImage.h @@ -71,6 +71,15 @@ class MacIOSurfaceRecycleAllocator { ~MacIOSurfaceRecycleAllocator() = default; nsTArray> 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 diff --git a/gfx/layers/NativeLayerCA.mm b/gfx/layers/NativeLayerCA.mm index 84f370192120..6fc67b9052d8 100644 --- a/gfx/layers/NativeLayerCA.mm +++ b/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 surf = new MacIOSurface(surface); - surf->Lock(true); - SurfaceFormat format = surf->GetFormat(); - if (format == SurfaceFormat::B8G8R8A8 || - format == SurfaceFormat::B8G8R8X8) { - RefPtr dt = - surf->GetAsDrawTargetLocked(gfx::BackendType::SKIA); - if (dt) { - RefPtr sourceSurf = dt->Snapshot(); - nsCString dataUrl; - gfxUtils::EncodeSourceSurface(sourceSurf, ImageType::PNG, u""_ns, - gfxUtils::eDataURIEncode, nullptr, - &dataUrl); - aOutputStream << dataUrl.get(); + if (surf->Lock(true)) { + SurfaceFormat format = surf->GetFormat(); + if (format == SurfaceFormat::B8G8R8A8 || + format == SurfaceFormat::B8G8R8X8) { + RefPtr dt = + surf->GetAsDrawTargetLocked(gfx::BackendType::SKIA); + if (dt) { + RefPtr sourceSurf = dt->Snapshot(); + nsCString dataUrl; + gfxUtils::EncodeSourceSurface(sourceSurf, ImageType::PNG, u""_ns, + gfxUtils::eDataURIEncode, nullptr, + &dataUrl); + aOutputStream << dataUrl.get(); + } } + surf->Unlock(true); } - surf->Unlock(true); } aOutputStream << "\"/>\n"; @@ -1326,8 +1327,13 @@ RefPtr NativeLayerCA::NextSurfaceAsDrawTarget( return nullptr; } - mInProgressLockedIOSurface = new MacIOSurface(mInProgressSurface->mSurface); - mInProgressLockedIOSurface->Lock(false); + auto surf = MakeRefPtr(mInProgressSurface->mSurface); + if (NS_WARN_IF(!surf->Lock(false))) { + gfxCriticalError() << "NextSurfaceAsDrawTarget lock surface failed."; + return nullptr; + } + + mInProgressLockedIOSurface = std::move(surf); RefPtr dt = mInProgressLockedIOSurface->GetAsDrawTargetLocked(aBackendType); @@ -1336,8 +1342,7 @@ RefPtr NativeLayerCA::NextSurfaceAsDrawTarget( [&](CFTypeRefPtr validSource, const gfx::IntRegion& copyRegion) { RefPtr source = new MacIOSurface(validSource); - source->Lock(true); - { + if (source->Lock(true)) { RefPtr sourceDT = source->GetAsDrawTargetLocked(aBackendType); RefPtr sourceSurface = sourceDT->Snapshot(); @@ -1346,8 +1351,10 @@ RefPtr 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; diff --git a/gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp b/gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp index 9a14daacc7db..dc601a1b0631 100644 --- a/gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp +++ b/gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp @@ -73,10 +73,7 @@ void MacIOSurfaceTextureData::FillInfo(TextureData::Info& aInfo) const { aInfo.canExposeMappedData = false; } -bool MacIOSurfaceTextureData::Lock(OpenMode) { - mSurface->Lock(false); - return true; -} +bool MacIOSurfaceTextureData::Lock(OpenMode) { return mSurface->Lock(false); } void MacIOSurfaceTextureData::Unlock() { mSurface->Unlock(false); } diff --git a/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp b/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp index 4572cf25ab03..54cc845b1c49 100644 --- a/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp +++ b/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);