[webkit-changes] [294317] trunk/Source

Tue, 17 May 2022 02:48:08 -0700

Title: [294317] trunk/Source
Revision
294317
Author
commit-qu...@webkit.org
Date
2022-05-17 02:47:30 -0700 (Tue, 17 May 2022)

Log Message

ImageBufferIOSurfaceBackend context setup and teardown functions are redundant and error-prone
https://bugs.webkit.org/show_bug.cgi?id=240452

Patch by Kimmo Kinnunen <kkinnu...@apple.com> on 2022-05-17
Reviewed by Simon Fraser.

Source/WebCore:

Remove ImageBuffer::releaseGraphicsContext().
It only made sense to be used before ImageBuffer::setVolatile().
The call setVolatile only made sense after releaseGraphicsContext().
Instead, make releasing the graphics context part of the
setVolatile() contract.

ImageBufferBackend::applyBaseTransformToContext() invoked a virtual
function context(). This was called from the middle of the inheritance chain,
from a constructor.
If a class later in the inheritance chain would override context(), the
call would not be invoked correctly. Currently no class suffered from this, but
in future some might.
ImageBufferIOSurfaceBackend call to applyBaseTransformToContext() would also
recurse through context() back to applyBaseTransformToContext() in non-intuitive
way through overridden ::context() which would re-enter applyBaseTransformToContext().
Based on current knowledge doubly invoking the logic did not cause any harm.

* platform/graphics/ConcreteImageBuffer.h:
* platform/graphics/ImageBuffer.h:
* platform/graphics/ImageBufferBackend.cpp:
(WebCore::ImageBufferBackend::applyBaseTransform):
(WebCore::ImageBufferBackend::applyBaseTransformToContext const): Deleted.
* platform/graphics/ImageBufferBackend.h:
(WebCore::ImageBufferBackend::isInUse const):
(WebCore::ImageBufferBackend::releaseGraphicsContext): Deleted.
* platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:
(WebCore::ImageBufferCGBitmapBackend::ImageBufferCGBitmapBackend):
* platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:
(WebCore::ImageBufferIOSurfaceBackend::ImageBufferIOSurfaceBackend):
(WebCore::ImageBufferIOSurfaceBackend::context const):
(WebCore::ImageBufferIOSurfaceBackend::setVolatile):
(WebCore::ImageBufferIOSurfaceBackend::setNonVolatile):
(WebCore::ImageBufferIOSurfaceBackend::initializeContext):
(WebCore::ImageBufferIOSurfaceBackend::releaseGraphicsContext): Deleted.
* platform/graphics/cg/ImageBufferIOSurfaceBackend.h:

Source/WebKit:

Adjust after removing ImageBuffer::releaseGraphicsContext().

* GPUProcess/graphics/RemoteRenderingBackend.cpp:
(WebKit::RemoteRenderingBackend::markSurfacesVolatile):
* Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::setBufferVolatile):
* WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (294316 => 294317)


--- trunk/Source/WebCore/ChangeLog	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/ChangeLog	2022-05-17 09:47:30 UTC (rev 294317)
@@ -1,3 +1,46 @@
+2022-05-17  Kimmo Kinnunen  <kkinnu...@apple.com>
+
+        ImageBufferIOSurfaceBackend context setup and teardown functions are redundant and error-prone
+        https://bugs.webkit.org/show_bug.cgi?id=240452
+
+        Reviewed by Simon Fraser.
+
+        Remove ImageBuffer::releaseGraphicsContext().
+        It only made sense to be used before ImageBuffer::setVolatile().
+        The call setVolatile only made sense after releaseGraphicsContext().
+        Instead, make releasing the graphics context part of the
+        setVolatile() contract.
+
+        ImageBufferBackend::applyBaseTransformToContext() invoked a virtual
+        function context(). This was called from the middle of the inheritance chain,
+        from a constructor.
+        If a class later in the inheritance chain would override context(), the
+        call would not be invoked correctly. Currently no class suffered from this, but
+        in future some might.
+        ImageBufferIOSurfaceBackend call to applyBaseTransformToContext() would also
+        recurse through context() back to applyBaseTransformToContext() in non-intuitive
+        way through overridden ::context() which would re-enter applyBaseTransformToContext().
+        Based on current knowledge doubly invoking the logic did not cause any harm.
+
+        * platform/graphics/ConcreteImageBuffer.h:
+        * platform/graphics/ImageBuffer.h:
+        * platform/graphics/ImageBufferBackend.cpp:
+        (WebCore::ImageBufferBackend::applyBaseTransform):
+        (WebCore::ImageBufferBackend::applyBaseTransformToContext const): Deleted.
+        * platform/graphics/ImageBufferBackend.h:
+        (WebCore::ImageBufferBackend::isInUse const):
+        (WebCore::ImageBufferBackend::releaseGraphicsContext): Deleted.
+        * platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:
+        (WebCore::ImageBufferCGBitmapBackend::ImageBufferCGBitmapBackend):
+        * platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:
+        (WebCore::ImageBufferIOSurfaceBackend::ImageBufferIOSurfaceBackend):
+        (WebCore::ImageBufferIOSurfaceBackend::context const):
+        (WebCore::ImageBufferIOSurfaceBackend::setVolatile):
+        (WebCore::ImageBufferIOSurfaceBackend::setNonVolatile):
+        (WebCore::ImageBufferIOSurfaceBackend::initializeContext):
+        (WebCore::ImageBufferIOSurfaceBackend::releaseGraphicsContext): Deleted.
+        * platform/graphics/cg/ImageBufferIOSurfaceBackend.h:
+
 2022-05-16  Antti Koivisto  <an...@apple.com>
 
         Resolve ::first-letter eagerly

Modified: trunk/Source/WebCore/platform/graphics/ConcreteImageBuffer.h (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/ConcreteImageBuffer.h	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/ConcreteImageBuffer.h	2022-05-17 09:47:30 UTC (rev 294317)
@@ -311,12 +311,6 @@
         return false;
     }
 
-    void releaseGraphicsContext() override
-    {
-        if (auto* backend = ensureBackendCreated())
-            return backend->releaseGraphicsContext();
-    }
-
     bool setVolatile() override
     {
         if (auto* backend = ensureBackendCreated())

Modified: trunk/Source/WebCore/platform/graphics/ImageBuffer.h (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/ImageBuffer.h	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/ImageBuffer.h	2022-05-17 09:47:30 UTC (rev 294317)
@@ -115,7 +115,6 @@
     virtual size_t externalMemoryCost() const = 0;
 
     virtual bool isInUse() const = 0;
-    virtual void releaseGraphicsContext() = 0;
     // Returns true on success.
     virtual bool setVolatile() = 0;
     virtual SetNonVolatileResult setNonVolatile() = 0;

Modified: trunk/Source/WebCore/platform/graphics/ImageBufferBackend.cpp (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/ImageBufferBackend.cpp	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/ImageBufferBackend.cpp	2022-05-17 09:47:30 UTC (rev 294317)
@@ -180,9 +180,8 @@
     return baseTransform;
 }
 
-void ImageBufferBackend::applyBaseTransformToContext() const
+void ImageBufferBackend::applyBaseTransform(GraphicsContext& context)
 {
-    auto& context = this->context();
     context.applyDeviceScaleFactor(m_parameters.resolutionScale);
     context.setCTM(calculateBaseTransform(m_parameters, originAtBottomLeftCorner()));
 }

Modified: trunk/Source/WebCore/platform/graphics/ImageBufferBackend.h (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/ImageBufferBackend.h	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/ImageBufferBackend.h	2022-05-17 09:47:30 UTC (rev 294317)
@@ -137,7 +137,6 @@
 #endif
 
     virtual bool isInUse() const { return false; }
-    virtual void releaseGraphicsContext() { ASSERT_NOT_REACHED(); }
 
     // Returns true on success.
     virtual bool setVolatile() { return true; }
@@ -147,7 +146,7 @@
 
     virtual std::unique_ptr<ThreadSafeImageBufferFlusher> createFlusher() { return nullptr; }
 
-    void applyBaseTransformToContext() const;
+    void applyBaseTransform(GraphicsContext&);
 
     static constexpr bool isOriginAtBottomLeftCorner = false;
     virtual bool originAtBottomLeftCorner() const { return isOriginAtBottomLeftCorner; }

Modified: trunk/Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp	2022-05-17 09:47:30 UTC (rev 294317)
@@ -115,7 +115,7 @@
     ASSERT(m_data);
     ASSERT(m_dataProvider);
     ASSERT(m_context);
-    applyBaseTransformToContext();
+    applyBaseTransform(*m_context);
 }
 
 ImageBufferCGBitmapBackend::~ImageBufferCGBitmapBackend() = default;

Modified: trunk/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp	2022-05-17 09:47:30 UTC (rev 294317)
@@ -120,7 +120,7 @@
     , m_ioSurfacePool(ioSurfacePool)
 {
     ASSERT(m_surface);
-    applyBaseTransformToContext();
+    initializeContext();
 }
 
 ImageBufferIOSurfaceBackend::~ImageBufferIOSurfaceBackend()
@@ -131,10 +131,6 @@
 
 GraphicsContext& ImageBufferIOSurfaceBackend::context() const
 {
-    if (!m_context) {
-        m_context.emplace(*m_surface);
-        applyBaseTransformToContext();
-    }
     return *m_context;
 }
 
@@ -236,14 +232,9 @@
     return m_surface->isInUse();
 }
 
-void ImageBufferIOSurfaceBackend::releaseGraphicsContext()
+bool ImageBufferIOSurfaceBackend::setVolatile()
 {
     m_context = std::nullopt;
-    m_surface->releasePlatformGraphicsContext();
-}
-
-bool ImageBufferIOSurfaceBackend::setVolatile()
-{
     if (m_surface->isInUse())
         return false;
 
@@ -255,7 +246,9 @@
 SetNonVolatileResult ImageBufferIOSurfaceBackend::setNonVolatile()
 {
     setVolatilityState(VolatilityState::NonVolatile);
-    return m_surface->setVolatile(false);
+    auto result = m_surface->setVolatile(false);
+    initializeContext();
+    return result;
 }
 
 VolatilityState ImageBufferIOSurfaceBackend::volatilityState() const
@@ -274,6 +267,12 @@
     flushContext();
 }
 
+void ImageBufferIOSurfaceBackend::initializeContext()
+{
+    m_context.emplace(*m_surface);
+    applyBaseTransform(*m_context);
+}
+
 } // namespace WebCore
 
 #endif // HAVE(IOSURFACE)

Modified: trunk/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h (294316 => 294317)


--- trunk/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h	2022-05-17 09:47:30 UTC (rev 294317)
@@ -73,7 +73,6 @@
     void putPixelBuffer(const PixelBuffer&, const IntRect& srcRect, const IntPoint& destPoint, AlphaPremultiplication destFormat) override;
 
     bool isInUse() const override;
-    void releaseGraphicsContext() override;
 
     bool setVolatile() final;
     SetNonVolatileResult setNonVolatile() final;
@@ -90,6 +89,7 @@
 
     void prepareToDrawIntoContext(GraphicsContext& destinationContext) override;
     void invalidateCachedNativeImage() const;
+    void initializeContext();
 
     std::unique_ptr<IOSurface> m_surface;
     IOSurfaceSeed m_lastSeedWhenDrawingImage { 0 };

Modified: trunk/Source/WebKit/ChangeLog (294316 => 294317)


--- trunk/Source/WebKit/ChangeLog	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebKit/ChangeLog	2022-05-17 09:47:30 UTC (rev 294317)
@@ -1,3 +1,18 @@
+2022-05-17  Kimmo Kinnunen  <kkinnu...@apple.com>
+
+        ImageBufferIOSurfaceBackend context setup and teardown functions are redundant and error-prone
+        https://bugs.webkit.org/show_bug.cgi?id=240452
+
+        Reviewed by Simon Fraser.
+
+        Adjust after removing ImageBuffer::releaseGraphicsContext().
+
+        * GPUProcess/graphics/RemoteRenderingBackend.cpp:
+        (WebKit::RemoteRenderingBackend::markSurfacesVolatile):
+        * Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
+        (WebKit::RemoteLayerBackingStore::setBufferVolatile):
+        * WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:
+
 2022-05-16  Wenson Hsieh  <wenson_hs...@apple.com>
 
         Demote -[WKWebView retrieveAccessibilityTreeData:] to SPI

Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp (294316 => 294317)


--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp	2022-05-17 09:47:30 UTC (rev 294317)
@@ -493,16 +493,11 @@
 {
     LOG_WITH_STREAM(RemoteRenderingBufferVolatility, stream << "GPU Process: RemoteRenderingBackend::markSurfacesVolatile " << identifiers);
 
-    auto makeVolatile = [](ImageBuffer& imageBuffer) {
-        imageBuffer.releaseGraphicsContext();
-        return imageBuffer.setVolatile();
-    };
-
     Vector<RenderingResourceIdentifier> markedVolatileBufferIdentifiers;
     for (auto identifier : identifiers) {
         auto imageBuffer = m_remoteResourceCache.cachedImageBuffer({ identifier, m_gpuConnectionToWebProcess->webProcessIdentifier() });
         if (imageBuffer) {
-            if (makeVolatile(*imageBuffer))
+            if (imageBuffer->setVolatile())
                 markedVolatileBufferIdentifiers.append(identifier);
         } else
             LOG_WITH_STREAM(RemoteRenderingBufferVolatility, stream << " failed to find ImageBuffer for identifier " << identifier);

Modified: trunk/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm (294316 => 294317)


--- trunk/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm	2022-05-17 09:47:30 UTC (rev 294317)
@@ -628,7 +628,6 @@
     if (!buffer.imageBuffer || buffer.imageBuffer->volatilityState() == VolatilityState::Volatile)
         return true;
 
-    buffer.imageBuffer->releaseGraphicsContext();
     return buffer.imageBuffer->setVolatile();
 }

Modified: trunk/Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h (294316 => 294317)


--- trunk/Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h	2022-05-17 09:08:43 UTC (rev 294316)
+++ trunk/Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h	2022-05-17 09:47:30 UTC (rev 294317)
@@ -74,7 +74,6 @@
     unsigned bytesPerRow() const final;
 
     ImageBufferBackendSharing* toBackendSharing() final { return this; }
-    void releaseGraphicsContext() final { /* Do nothing. This is only relevant for IOSurface backends */ }
 
     Ref<ShareableBitmap> m_bitmap;
     std::unique_ptr<WebCore::GraphicsContext> m_context;

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to