REGRESSION (264004@main): Images MotionMark subtest shows incorrect r…

…endering on Intel

https://bugs.webkit.org/show_bug.cgi?id=258560
rdar://111197581

Reviewed by Simon Fraser.

ImageBufferShareableMappedIOSurfaceBitmapBackend would lock the
IOSurface and draw to it as a bitmap.

The code was written with the expectation was that after layer backing
store flush, ImageBuffer::releaseGraphicsContext() was called and this
would release the GraphicsContext, the underlying CGContext and the
IOSurface lock. This was a misunderstanding, releaseGraphicsContext() is
never called. This would cause MotionMark Images test to render
incorrectly on Intel-based Macs, since the IOSurface unlock is needed to
copy the main memory IOSurface cache back to the real IOSurface memory.

After each layer backing store update, the corresponding layer
ImageBuffers are flushed. Use the flush as the cue to unlock the
IOSurface that ImageBufferShareableMappedIOSurfaceBitmapBackend draws
to, before external access via the UI process starts.

Related but not strictly needed, make sure IOSurface locking errors are
handled gracefully. This bug is not about IOSurface locking errors, but
they were ignored before, and as such the behavior was part of the
investigation of this bug.

* Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:
(WebCore::ImageBufferIOSurfaceBackend::getPixelBuffer):
(WebCore::ImageBufferIOSurfaceBackend::putPixelBuffer):
* Source/WebCore/platform/graphics/cocoa/IOSurface.h:
(WebCore::IOSurface::lock):
* Source/WebCore/platform/graphics/cocoa/IOSurface.mm:
(WebCore::IOSurface::createBitmapPlatformContext):
* Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableMappedIOSurfaceBitmapBackend.cpp:
(WebKit::ImageBufferShareableMappedIOSurfaceBitmapBackend::~ImageBufferShareableMappedIOSurfaceBitmapBackend):
(WebKit::ImageBufferShareableMappedIOSurfaceBitmapBackend::context):
(WebKit::ImageBufferShareableMappedIOSurfaceBitmapBackend::flushContext):
* Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableMappedIOSurfaceBitmapBackend.h:

Canonical link: https://commits.webkit.org/265712@main

Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp

@@ -199,15 +199,15 @@ RefPtr<NativeImage> ImageBufferIOSurfaceBackend::sinkIntoNativeImage()
 void ImageBufferIOSurfaceBackend::getPixelBuffer(const IntRect& srcRect, PixelBuffer& destination)
 {
     const_cast<ImageBufferIOSurfaceBackend*>(this)->prepareForExternalRead();
-    IOSurface::Locker lock(*m_surface);
-    ImageBufferBackend::getPixelBuffer(srcRect, lock.surfaceBaseAddress(), destination);
+    if (auto lock = m_surface->lock<IOSurface::AccessMode::ReadOnly>())
+        ImageBufferBackend::getPixelBuffer(srcRect, lock->surfaceBaseAddress(), destination);
 }
 
 void ImageBufferIOSurfaceBackend::putPixelBuffer(const PixelBuffer& pixelBuffer, const IntRect& srcRect, const IntPoint& destPoint, AlphaPremultiplication destFormat)
 {
     prepareForExternalWrite();
-    IOSurface::Locker lock(*m_surface, IOSurface::Locker::AccessMode::ReadWrite);
-    ImageBufferBackend::putPixelBuffer(pixelBuffer, srcRect, destPoint, destFormat, lock.surfaceBaseAddress());
+    if (auto lock = m_surface->lock<IOSurface::AccessMode::ReadWrite>())
+        ImageBufferBackend::putPixelBuffer(pixelBuffer, srcRect, destPoint, destFormat, lock->surfaceBaseAddress());
 }
 
 IOSurface* ImageBufferIOSurfaceBackend::surface()

Source/WebCore/platform/graphics/cocoa/IOSurface.h

@@ -82,49 +82,49 @@ class IOSurface final {
     };
 
     WEBCORE_EXPORT static IOSurface::Format formatForPixelFormat(WebCore::PixelFormat);
-
+
+    enum class AccessMode : uint32_t {
+        ReadWrite = 0,
+        ReadOnly = kIOSurfaceLockReadOnly
+    };
+    template <AccessMode Mode>
     class Locker {
     public:
-        enum class AccessMode : uint32_t {
-            ReadWrite = 0,
-            ReadOnly = kIOSurfaceLockReadOnly,
-        };
-
-        explicit Locker(IOSurface& surface, AccessMode mode = AccessMode::ReadOnly)
-            : m_surface(surface.surface())
-            , m_flags(static_cast<uint32_t>(mode))
+        static Locker adopt(RetainPtr<IOSurfaceRef> surface)
         {
-            IOSurfaceLock(m_surface, m_flags, nullptr);
+            return Locker { WTFMove(surface) };
         }
 
         Locker(Locker&& other)
             : m_surface(std::exchange(other.m_surface, nullptr))
-            , m_flags(other.m_flags)
         {
         }
 
         ~Locker()
         {
             if (!m_surface)
                 return;
-            IOSurfaceUnlock(m_surface, m_flags, nullptr);
+            IOSurfaceUnlock(m_surface.get(), static_cast<uint32_t>(Mode), nullptr);
         }
 
         Locker& operator=(Locker&& other)
         {
             m_surface = std::exchange(other.m_surface, nullptr);
-            m_flags = other.m_flags;
             return *this;
         }
 
-        void * surfaceBaseAddress() const
+        void* surfaceBaseAddress() const
         {
-            return IOSurfaceGetBaseAddress(m_surface);
+            return IOSurfaceGetBaseAddress(m_surface.get());
         }
 
     private:
-        IOSurfaceRef m_surface;
-        uint32_t m_flags;
+        explicit Locker(RetainPtr<IOSurfaceRef> surface)
+            : m_surface(WTFMove(surface))
+        {
+        }
+
+        RetainPtr<IOSurfaceRef> m_surface;
     };
 
     WEBCORE_EXPORT static std::unique_ptr<IOSurface> create(IOSurfacePool*, IntSize, const DestinationColorSpace&, Name = Name::Default, Format = Format::BGRA);
@@ -166,10 +166,11 @@ class IOSurface final {
     WEBCORE_EXPORT RetainPtr<CGContextRef> createPlatformContext(PlatformDisplayID = 0);
 
     struct LockAndContext {
-        IOSurface::Locker lock;
+        IOSurface::Locker<AccessMode::ReadWrite> lock;
         RetainPtr<CGContextRef> context;
     };
     WEBCORE_EXPORT std::optional<LockAndContext> createBitmapPlatformContext();
+    template<AccessMode Mode> std::optional<Locker<Mode>> lock();
 
     // Querying volatility can be expensive, so in cases where the surface is
     // going to be used immediately, use the return value of setVolatile to
@@ -234,6 +235,14 @@ class IOSurface final {
     WEBCORE_EXPORT friend WTF::TextStream& operator<<(WTF::TextStream&, const WebCore::IOSurface&);
 };
 
+template<IOSurface::AccessMode Mode>
+std::optional<IOSurface::Locker<Mode>> IOSurface::lock()
+{
+    if (IOSurfaceLock(m_surface.get(), static_cast<uint32_t>(Mode), nullptr) != kIOReturnSuccess)
+        return std::nullopt;
+    return IOSurface::Locker<Mode>::adopt(m_surface);
+}
+
 WEBCORE_EXPORT WTF::TextStream& operator<<(WTF::TextStream&, WebCore::IOSurface::Format);
 
 } // namespace WebCore

Source/WebCore/platform/graphics/cocoa/IOSurface.mm

@@ -462,13 +462,15 @@ static IntSize computeMaximumSurfaceSize()
 
 std::optional<IOSurface::LockAndContext> IOSurface::createBitmapPlatformContext()
 {
-    IOSurface::Locker locker { *this, IOSurface::Locker::AccessMode::ReadWrite };
+    auto locker = lock<AccessMode::ReadWrite>();
+    if (!locker)
+        return std::nullopt;
     auto configuration = bitmapConfiguration();
     auto size = this->size();
-    auto context = adoptCF(CGBitmapContextCreate(locker.surfaceBaseAddress(), size.width(), size.height(), configuration.bitsPerComponent, bytesPerRow(), colorSpace().platformColorSpace(), configuration.bitmapInfo));
+    auto context = adoptCF(CGBitmapContextCreate(locker->surfaceBaseAddress(), size.width(), size.height(), configuration.bitsPerComponent, bytesPerRow(), colorSpace().platformColorSpace(), configuration.bitmapInfo));
     if (!context)
         return std::nullopt;
-    return LockAndContext { WTFMove(locker), WTFMove(context) };
+    return LockAndContext { WTFMove(*locker), WTFMove(context) };
 }
 
 SetNonVolatileResult IOSurface::state() const

Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableMappedIOSurfaceBitmapBackend.cpp

@@ -67,22 +67,50 @@ ImageBufferShareableMappedIOSurfaceBitmapBackend::ImageBufferShareableMappedIOSu
     applyBaseTransform(*m_context);
 }
 
+ImageBufferShareableMappedIOSurfaceBitmapBackend::~ImageBufferShareableMappedIOSurfaceBitmapBackend()
+{
+    releaseGraphicsContext();
+    IOSurface::moveToPool(WTFMove(m_surface), m_ioSurfacePool.get());
+}
+
 ImageBufferBackendHandle ImageBufferShareableMappedIOSurfaceBitmapBackend::createBackendHandle(SharedMemory::Protection) const
 {
     return ImageBufferBackendHandle(m_surface->createSendRight());
 }
 
 GraphicsContext& ImageBufferShareableMappedIOSurfaceBitmapBackend::context()
 {
-    if (!m_context) {
+    if (m_context) {
+        CGContextRef cgContext = m_context->platformContext();
+        if (m_lock || !cgContext) {
+            // The existing context is a valid context and the IOSurface is locked, or alternatively
+            // the existing context is an invalid context, for some reason we ran into an error previously.
+            return *m_context;
+        }
+
+        // The IOSurface is unlocked for every flush to prepare for external access by the compositor.
+        // Re-lock on first context() request after the external access has ended and new update starts.
+        if (auto lock = m_surface->lock<IOSurface::AccessMode::ReadWrite>()) {
+            if (lock->surfaceBaseAddress() == CGBitmapContextGetData(cgContext)) {
+                m_lock = WTFMove(lock);
+                return *m_context;
+            }
+        }
+        m_context = nullptr;
+    } else {
         auto lockAndContext = m_surface->createBitmapPlatformContext();
         if (lockAndContext) {
             m_lock = WTFMove(lockAndContext->lock);
             m_context = makeUnique<GraphicsContextCG>(lockAndContext->context.get());
-        } else
-            m_context = makeUnique<GraphicsContextCG>(nullptr);
-        applyBaseTransform(*m_context);
+            applyBaseTransform(*m_context);
+            return *m_context;
+        }
     }
+    // For some reason we ran into an error. Construct an invalid context, with current API we must
+    // return an object.
+    RELEASE_LOG(RemoteLayerBuffers, "ImageBufferShareableMappedIOSurfaceBitmapBackend::context() - failed to create or update the context");
+    m_context = makeUnique<GraphicsContextCG>(nullptr);
+    applyBaseTransform(*m_context);
     return *m_context;
 }
 
@@ -171,6 +199,12 @@ void ImageBufferShareableMappedIOSurfaceBitmapBackend::putPixelBuffer(const Pixe
     ASSERT_NOT_REACHED(); // Not applicable for LayerBacking.
 }
 
+void ImageBufferShareableMappedIOSurfaceBitmapBackend::flushContext()
+{
+    // Flush means external access by the compositor. Unlock the IOSurface so that the compositor sees the updates to the bitmap.
+    m_lock = std::nullopt;
+}
+
 } // namespace WebKit
 
 #endif // ENABLE(GPU_PROCESS) && HAVE(IOSURFACE)

Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableMappedIOSurfaceBitmapBackend.h

@@ -47,6 +47,7 @@ class ImageBufferShareableMappedIOSurfaceBitmapBackend final : public WebCore::I
     static size_t calculateMemoryCost(const Parameters& parameters) { return WebCore::ImageBufferIOSurfaceBackend::calculateMemoryCost(parameters); }
 
     ImageBufferShareableMappedIOSurfaceBitmapBackend(const Parameters&, std::unique_ptr<WebCore::IOSurface>, WebCore::IOSurface::LockAndContext&&, WebCore::IOSurfacePool*);
+    ~ImageBufferShareableMappedIOSurfaceBitmapBackend();
 
     static constexpr WebCore::RenderingMode renderingMode = WebCore::RenderingMode::Accelerated;
     static constexpr bool isOriginAtBottomLeftCorner = true;
@@ -74,9 +75,10 @@ class ImageBufferShareableMappedIOSurfaceBitmapBackend final : public WebCore::I
     void transferToNewContext(const WebCore::ImageBufferCreationContext&) final;
     void getPixelBuffer(const WebCore::IntRect&, WebCore::PixelBuffer&) final;
     void putPixelBuffer(const WebCore::PixelBuffer&, const WebCore::IntRect&, const WebCore::IntPoint&, WebCore::AlphaPremultiplication) final;
+    void flushContext() final;
 
     std::unique_ptr<WebCore::IOSurface> m_surface;
-    std::optional<WebCore::IOSurface::Locker> m_lock;
+    std::optional<WebCore::IOSurface::Locker<WebCore::IOSurface::AccessMode::ReadWrite>> m_lock;
     WebCore::VolatilityState m_volatilityState { WebCore::VolatilityState::NonVolatile };
     RefPtr<WebCore::IOSurfacePool> m_ioSurfacePool;
 };