Changeset 284888 in webkit

Timestamp:

Oct 26, 2021, 12:42:43 PM (5 years ago)

Author:

timothy_horton@apple.com

Message:

DisplayList::Recorder's clipBounds() becomes empty if a flip is applied to the CTM
​https://bugs.webkit.org/show_bug.cgi?id=232134

Reviewed by Darin Adler.

Source/WebCore:

New test: BifurcatedGraphicsContextTests.TransformedClip

• platform/graphics/displaylists/DisplayListRecorder.cpp:

(WebCore::DisplayList::Recorder::clip):
(WebCore::DisplayList::Recorder::clipPath):
(WebCore::DisplayList::Recorder::clipBounds const):
Instead of updating clipBounds any time the CTM changes, store
clipBounds in base coordinates and map through the CTM when retrieved.

This matches CG's behavior and makes the clipBounds much sturdier.
For example, previously, applying a scale(1, -1) to the context
would immediately result in the clipBounds' height becoming negative,
making the bounds empty and confusing anything that reads from it.

(WebCore::DisplayList::Recorder::ContextState::translate):
(WebCore::DisplayList::Recorder::ContextState::rotate):
(WebCore::DisplayList::Recorder::ContextState::scale):
(WebCore::DisplayList::Recorder::ContextState::setCTM):
(WebCore::DisplayList::Recorder::ContextState::concatCTM):
Stop updating the clipBounds when the CTM changes, this is no longer necessary.

• platform/graphics/displaylists/DisplayListRecorderImpl.cpp:

(WebCore::DisplayList::RecorderImpl::extentFromLocalBounds):
Since the clipBounds is now in base space, map the display list
item bounds to base space /before/ intersecting it with clipBounds.

• platform/graphics/displaylists/DisplayListRecorder.h:

Drive-by add a default parameter so getCTM can be called on the subclass the same way it can on GraphicsContext.

• platform/graphics/transforms/AffineTransform.h: Fix a typo.

Tools:

• TestWebKitAPI/Tests/WebCore/cg/BifurcatedGraphicsContextTestsCG.cpp:

(TestWebKitAPI::TEST):
Add a test ensuring that our clipBounds and CG's match through a series
of simple transforms.

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp (modified) (7 diffs)
• Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h (modified) (1 diff)
• Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp (modified) (1 diff)
• Source/WebCore/platform/graphics/transforms/AffineTransform.h (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebCore/cg/BifurcatedGraphicsContextTestsCG.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-10-26  Tim Horton  <timothy_horton@apple.com>
+
+        DisplayList::Recorder's clipBounds() becomes empty if a flip is applied to the CTM
+        https://bugs.webkit.org/show_bug.cgi?id=232134
+
+        Reviewed by Darin Adler.
+
+        New test: BifurcatedGraphicsContextTests.TransformedClip
+
+        * platform/graphics/displaylists/DisplayListRecorder.cpp:
+        (WebCore::DisplayList::Recorder::clip):
+        (WebCore::DisplayList::Recorder::clipPath):
+        (WebCore::DisplayList::Recorder::clipBounds const):
+        Instead of updating clipBounds any time the CTM changes, store
+        clipBounds in base coordinates and map through the CTM when retrieved.
+
+        This matches CG's behavior and makes the clipBounds much sturdier.
+        For example, previously, applying a `scale(1, -1)` to the context
+        would immediately result in the clipBounds' height becoming negative,
+        making the bounds empty and confusing anything that reads from it.
+
+        (WebCore::DisplayList::Recorder::ContextState::translate):
+        (WebCore::DisplayList::Recorder::ContextState::rotate):
+        (WebCore::DisplayList::Recorder::ContextState::scale):
+        (WebCore::DisplayList::Recorder::ContextState::setCTM):
+        (WebCore::DisplayList::Recorder::ContextState::concatCTM):
+        Stop updating the clipBounds when the CTM changes, this is no longer necessary.
+
+        * platform/graphics/displaylists/DisplayListRecorderImpl.cpp:
+        (WebCore::DisplayList::RecorderImpl::extentFromLocalBounds):
+        Since the clipBounds is now in base space, map the display list
+        item bounds to base space /before/ intersecting it with clipBounds.
+
+        * platform/graphics/displaylists/DisplayListRecorder.h:
+        Drive-by add a default parameter so getCTM can be called on the subclass the same way it can on GraphicsContext.
+
+        * platform/graphics/transforms/AffineTransform.h: Fix a typo.
+
 2021-10-26  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp

@@ -425 +425 @@
 void Recorder::clip(const FloatRect& rect)
 {
-    currentState().clipBounds.intersect(rect);
+    currentState().clipBounds.intersect(currentState().ctm.mapRect(rect));
     recordClip(rect);
 }
@@ -441 +441 @@
 void Recorder::clipPath(const Path& path, WindRule windRule)
 {
-    currentState().clipBounds.intersect(path.fastBoundingRect());
+    currentState().clipBounds.intersect(currentState().ctm.mapRect(path.fastBoundingRect()));
     recordClipPath(path, windRule);
 }
@@ -447 +447 @@
 IntRect Recorder::clipBounds() const
 {
+    if (auto inverse = currentState().ctm.inverse())
+        return enclosingIntRect(inverse->mapRect(currentState().clipBounds));
+
+    // If the CTM is not invertible, return the original rect.
+    // This matches CGRectApplyInverseAffineTransform behavior.
     return enclosingIntRect(currentState().clipBounds);
 }
@@ -522 +527 @@
 {
     ctm.translate(x, y);
-    clipBounds.move(-x, -y);
 }
 
@@ -532 +536 @@
     AffineTransform rotation;
     rotation.rotate(angleInDegrees);
-
-    if (std::optional<AffineTransform> inverse = rotation.inverse())
-        clipBounds = inverse.value().mapRect(clipBounds);
 }
 
@@ -540 +541 @@
 {
     ctm.scale(size);
-    clipBounds.scale(1 / size.width(), 1 / size.height());
 }
 
 void Recorder::ContextState::setCTM(const AffineTransform& matrix)
 {
-    std::optional<AffineTransform> inverseTransformForClipBounds;
-    if (auto originalCTMInverse = ctm.inverse())
-        inverseTransformForClipBounds = originalCTMInverse->multiply(matrix).inverse();
-
     ctm = matrix;
-
-    if (inverseTransformForClipBounds)
-        clipBounds = inverseTransformForClipBounds->mapRect(clipBounds);
 }
 
@@ -558 +551 @@
 {
     ctm *= matrix;
-
-    if (std::optional<AffineTransform> inverse = matrix.inverse())
-        clipBounds = inverse.value().mapRect(clipBounds);
 }
 

trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h

@@ -256 +256 @@
     WEBCORE_EXPORT void concatCTM(const AffineTransform&) final;
     WEBCORE_EXPORT void setCTM(const AffineTransform&) final;
-    WEBCORE_EXPORT AffineTransform getCTM(GraphicsContext::IncludeDeviceScale) const final;
+    WEBCORE_EXPORT AffineTransform getCTM(GraphicsContext::IncludeDeviceScale = PossiblyIncludeDeviceScale) const final;
 
     WEBCORE_EXPORT void beginTransparencyLayer(float opacity) final;

trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp

@@ -459 +459 @@
     }
 
-    FloatRect clippedExtent = intersection(state.clipBounds, bounds);
-    return state.ctm.mapRect(clippedExtent);
+    return intersection(state.clipBounds, state.ctm.mapRect(bounds));
 }
 

trunk/Source/WebCore/platform/graphics/transforms/AffineTransform.h

@@ -133 +133 @@
     WEBCORE_EXPORT double yScale() const;
 
-    bool isInvertible() const; // If you call this this, you're probably doing it wrong.
+    bool isInvertible() const; // If you call this, you're probably doing it wrong.
     WEBCORE_EXPORT std::optional<AffineTransform> inverse() const;
 

trunk/Tools/ChangeLog

@@ +1 @@
+2021-10-26  Tim Horton  <timothy_horton@apple.com>
+
+        DisplayList::Recorder's clipBounds() becomes empty if a flip is applied to the CTM
+        https://bugs.webkit.org/show_bug.cgi?id=232134
+
+        Reviewed by Darin Adler.
+
+        * TestWebKitAPI/Tests/WebCore/cg/BifurcatedGraphicsContextTestsCG.cpp:
+        (TestWebKitAPI::TEST):
+        Add a test ensuring that our clipBounds and CG's match through a series
+        of simple transforms.
+
 2021-10-26  Brady Eidson  <beidson@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/BifurcatedGraphicsContextTestsCG.cpp

@@ -220 +220 @@
 }
 
+TEST(BifurcatedGraphicsContextTests, TransformedClip)
+{
+    auto colorSpace = DestinationColorSpace::SRGB();
+    auto primaryCGContext = adoptCF(CGBitmapContextCreate(nullptr, 100, 100, 8, 4 * 100, colorSpace.platformColorSpace(), kCGImageAlphaPremultipliedLast));
+
+    GraphicsContextCG primaryContextCG(primaryCGContext.get());
+    GraphicsContext& primaryContext = primaryContextCG;
+
+    InMemoryDisplayList displayList;
+    RecorderImpl secondaryContextDL(displayList, { }, FloatRect(0, 0, 100, 100), { });
+    GraphicsContext& secondaryContext = secondaryContextDL;
+
+    BifurcatedGraphicsContext ctx(primaryContext, secondaryContext);
+
+    ctx.clip(FloatRect(25, 25, 50, 50));
+
+    EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+    EXPECT_EQ(primaryContext.clipBounds(), FloatRect(25, 25, 50, 50));
+
+    ctx.scale({ 1, -1 });
+    ctx.translate(0, -100);
+
+    EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+    EXPECT_EQ(primaryContext.clipBounds(), FloatRect(25, 25, 50, 50));
+
+    ctx.scale(2);
+
+    EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+    EXPECT_EQ(primaryContext.clipBounds(), FloatRect(12, 12, 26, 26));
+
+    {
+        GraphicsContextStateSaver saver(ctx);
+
+        ctx.translate(12, 12);
+
+        EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+        EXPECT_EQ(primaryContext.clipBounds(), FloatRect(0, 0, 26, 26));
+
+        ctx.clip(FloatRect(0, 0, 10, 10));
+
+        EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+        EXPECT_EQ(primaryContext.clipBounds(), FloatRect(0, 0, 10, 10));
+
+        ctx.rotate(M_PI / 6);
+
+        EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+        EXPECT_EQ(primaryContext.clipBounds(), FloatRect(0, -5, 14, 14));
+    }
+
+    EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+    EXPECT_EQ(primaryContext.clipBounds(), FloatRect(12, 12, 26, 26));
+
+    // Make the CTM non-invertible.
+    ctx.scale({ 0, 1 });
+
+    EXPECT_EQ(primaryContext.clipBounds(), secondaryContext.clipBounds());
+    EXPECT_EQ(primaryContext.clipBounds(), FloatRect(25, 25, 50, 50));
+}
+
 } // namespace TestWebKitAPI