Title: [269497] trunk
Revision
269497
Author
mmaxfi...@apple.com
Date
2020-11-05 17:45:40 -0800 (Thu, 05 Nov 2020)

Log Message

[Cocoa] REGRESSION(r269211): Text with emoji can trigger drawing corruption
https://bugs.webkit.org/show_bug.cgi?id=218636
<rdar://problem/71066011>

Reviewed by Simon Fraser.

Source/WebCore:

We have an optimization for frequently-painting layers that we will turn text drawing
commands into display lists. r269211 added a bunch of context state introspection and
recreation code to make sure that the state being recorded matches the state during
playback. However, that code is incompatible with this cache idea, where the same DL
might be played back into multiple places.

The solution is to have two kinds of text display list drawing:
1. Deconstruct drawing commands into a series of commands. E.g. emoji will get
deconstructed into DrawImage commands. This kind of drawing must do state introspection
to make sure the emoji is drawn in the right place
2. Don't deconstruct the drawing commands; simply gather up the DrawGlyphs commands
and record them directly. This doesn't do state introspection.

GPU Process display lists use the first kind, while this text cache optimization uses
the second kind.

Test: fast/text/frequent-text.html

* platform/graphics/FontCascade.cpp:
(WebCore::FontCascade::displayListForTextRun const):
* platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h:
* platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:
(WebCore::DisplayList::DrawGlyphsRecorder::DrawGlyphsRecorder):
(WebCore::DisplayList::DrawGlyphsRecorder::drawGlyphs):
* platform/graphics/displaylists/DisplayListDrawGlyphsRecorderHarfBuzz.cpp:
(WebCore::DisplayList::DrawGlyphsRecorder::DrawGlyphsRecorder):
* platform/graphics/displaylists/DisplayListDrawGlyphsRecorderWin.cpp:
(WebCore::DisplayList::DrawGlyphsRecorder::DrawGlyphsRecorder):
* platform/graphics/displaylists/DisplayListRecorder.cpp:
(WebCore::DisplayList::Recorder::Recorder):
* platform/graphics/displaylists/DisplayListRecorder.h:

LayoutTests:

* fast/text/frequent-text-expected.html: Added.
* fast/text/frequent-text.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (269496 => 269497)


--- trunk/LayoutTests/ChangeLog	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/LayoutTests/ChangeLog	2020-11-06 01:45:40 UTC (rev 269497)
@@ -1,3 +1,14 @@
+2020-11-05  Myles C. Maxfield  <mmaxfi...@apple.com>
+
+        [Cocoa] REGRESSION(r269211): Text with emoji can trigger drawing corruption
+        https://bugs.webkit.org/show_bug.cgi?id=218636
+        <rdar://problem/71066011>
+
+        Reviewed by Simon Fraser.
+
+        * fast/text/frequent-text-expected.html: Added.
+        * fast/text/frequent-text.html: Added.
+
 2020-11-05  Chris Dumez  <cdu...@apple.com>
 
         unreviewed, rebaseline a couple tests on iOS after r269477.

Added: trunk/LayoutTests/fast/text/frequent-text-expected.html (0 => 269497)


--- trunk/LayoutTests/fast/text/frequent-text-expected.html	                        (rev 0)
+++ trunk/LayoutTests/fast/text/frequent-text-expected.html	2020-11-06 01:45:40 UTC (rev 269497)
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+</head>
+<body>
+<div id="target">Hello 👋 <span id="inside"></span> World!</div>
+</body>
+</html>

Added: trunk/LayoutTests/fast/text/frequent-text.html (0 => 269497)


--- trunk/LayoutTests/fast/text/frequent-text.html	                        (rev 0)
+++ trunk/LayoutTests/fast/text/frequent-text.html	2020-11-06 01:45:40 UTC (rev 269497)
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+</head>
+<body>
+<div id="target">Hello 👋 <span id="inside"></span> World!</div>
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+window.requestAnimationFrame(function() {
+    let target = document.getElementById("target");
+    let inside = document.getElementById("inside");
+    if (window.internals) {
+        for (let i = 0; i < 200; ++i)
+            internals.incrementFrequentPaintCounter(target);
+    }
+    inside.textContext = "middle";
+    window.requestAnimationFrame(function() {
+        inside.textContext = "";
+        window.requestAnimationFrame(function() {
+            if (window.testRunner)
+                testRunner.notifyDone();
+        });
+    });
+});
+</script>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (269496 => 269497)


--- trunk/Source/WebCore/ChangeLog	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/ChangeLog	2020-11-06 01:45:40 UTC (rev 269497)
@@ -1,3 +1,43 @@
+2020-11-05  Myles C. Maxfield  <mmaxfi...@apple.com>
+
+        [Cocoa] REGRESSION(r269211): Text with emoji can trigger drawing corruption
+        https://bugs.webkit.org/show_bug.cgi?id=218636
+        <rdar://problem/71066011>
+
+        Reviewed by Simon Fraser.
+
+        We have an optimization for frequently-painting layers that we will turn text drawing
+        commands into display lists. r269211 added a bunch of context state introspection and
+        recreation code to make sure that the state being recorded matches the state during
+        playback. However, that code is incompatible with this cache idea, where the same DL
+        might be played back into multiple places.
+
+        The solution is to have two kinds of text display list drawing:
+        1. Deconstruct drawing commands into a series of commands. E.g. emoji will get
+        deconstructed into DrawImage commands. This kind of drawing must do state introspection
+        to make sure the emoji is drawn in the right place
+        2. Don't deconstruct the drawing commands; simply gather up the DrawGlyphs commands
+        and record them directly. This doesn't do state introspection.
+
+        GPU Process display lists use the first kind, while this text cache optimization uses
+        the second kind.
+
+        Test: fast/text/frequent-text.html
+
+        * platform/graphics/FontCascade.cpp:
+        (WebCore::FontCascade::displayListForTextRun const):
+        * platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h:
+        * platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:
+        (WebCore::DisplayList::DrawGlyphsRecorder::DrawGlyphsRecorder):
+        (WebCore::DisplayList::DrawGlyphsRecorder::drawGlyphs):
+        * platform/graphics/displaylists/DisplayListDrawGlyphsRecorderHarfBuzz.cpp:
+        (WebCore::DisplayList::DrawGlyphsRecorder::DrawGlyphsRecorder):
+        * platform/graphics/displaylists/DisplayListDrawGlyphsRecorderWin.cpp:
+        (WebCore::DisplayList::DrawGlyphsRecorder::DrawGlyphsRecorder):
+        * platform/graphics/displaylists/DisplayListRecorder.cpp:
+        (WebCore::DisplayList::Recorder::Recorder):
+        * platform/graphics/displaylists/DisplayListRecorder.h:
+
 2020-11-05  John Wilander  <wilan...@apple.com>
 
         PCM: Switch to JSON report format

Modified: trunk/Source/WebCore/platform/graphics/FontCascade.cpp (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/FontCascade.cpp	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/FontCascade.cpp	2020-11-06 01:45:40 UTC (rev 269497)
@@ -342,7 +342,7 @@
     
     std::unique_ptr<DisplayList::DisplayList> displayList = makeUnique<DisplayList::DisplayList>();
     GraphicsContext recordingContext([&](GraphicsContext& displayListContext) {
-        return makeUnique<DisplayList::Recorder>(displayListContext, *displayList, context.state(), FloatRect(), AffineTransform());
+        return makeUnique<DisplayList::Recorder>(displayListContext, *displayList, context.state(), FloatRect(), AffineTransform(), nullptr, DisplayList::DrawGlyphsRecorder::DrawGlyphsDeconstruction::DontDeconstruct);
     });
     
     FloatPoint startPoint = toFloatPoint(WebCore::size(glyphBuffer.initialAdvance()));

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h	2020-11-06 01:45:40 UTC (rev 269497)
@@ -50,7 +50,11 @@
 
 class DrawGlyphsRecorder {
 public:
-    explicit DrawGlyphsRecorder(Recorder&);
+    enum class DrawGlyphsDeconstruction {
+        Deconstruct,
+        DontDeconstruct
+    };
+    explicit DrawGlyphsRecorder(Recorder&, DrawGlyphsDeconstruction);
 
     void drawGlyphs(const Font&, const GlyphBuffer&, unsigned from, unsigned numGlyphs, const FloatPoint& anchorPoint, FontSmoothingMode);
 
@@ -84,6 +88,7 @@
 #endif
 
     Recorder& m_owner;
+    DrawGlyphsDeconstruction m_drawGlyphsDeconstruction;
     GraphicsContext m_internalContext;
     const Font* m_originalFont { nullptr };
     FontSmoothingMode m_smoothingMode { FontSmoothingMode::AutoSmoothing };

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp	2020-11-06 01:45:40 UTC (rev 269497)
@@ -87,8 +87,9 @@
     return GraphicsContext(context.get());
 }
 
-DrawGlyphsRecorder::DrawGlyphsRecorder(Recorder& owner)
+DrawGlyphsRecorder::DrawGlyphsRecorder(Recorder& owner, DrawGlyphsDeconstruction drawGlyphsDeconstruction)
     : m_owner(owner)
+    , m_drawGlyphsDeconstruction(drawGlyphsDeconstruction)
     , m_internalContext(createInternalContext())
 {
 }
@@ -361,6 +362,12 @@
 
 void DrawGlyphsRecorder::drawGlyphs(const Font& font, const GlyphBuffer& glyphBuffer, unsigned from, unsigned numGlyphs, const FloatPoint& startPoint, FontSmoothingMode smoothingMode)
 {
+    if (m_drawGlyphsDeconstruction == DrawGlyphsDeconstruction::DontDeconstruct) {
+        m_owner.appendItemAndUpdateExtent(DrawGlyphs::create(font, glyphBuffer.glyphs(from), glyphBuffer.advances(from), numGlyphs, startPoint, smoothingMode));
+        return;
+    }
+
+    ASSERT(m_drawGlyphsDeconstruction == DrawGlyphsDeconstruction::Deconstruct);
     prepareInternalContext(font, smoothingMode);
     FontCascade::drawGlyphs(m_internalContext, font, glyphBuffer, from, numGlyphs, startPoint, smoothingMode);
     concludeInternalContext();

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderHarfBuzz.cpp (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderHarfBuzz.cpp	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderHarfBuzz.cpp	2020-11-06 01:45:40 UTC (rev 269497)
@@ -36,7 +36,7 @@
 
 namespace DisplayList {
 
-DrawGlyphsRecorder::DrawGlyphsRecorder(Recorder& owner)
+DrawGlyphsRecorder::DrawGlyphsRecorder(Recorder& owner, DrawGlyphsDeconstruction)
     : m_owner(owner)
 {
 }

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderWin.cpp (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderWin.cpp	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderWin.cpp	2020-11-06 01:45:40 UTC (rev 269497)
@@ -36,7 +36,7 @@
 
 namespace DisplayList {
 
-DrawGlyphsRecorder::DrawGlyphsRecorder(Recorder& owner)
+DrawGlyphsRecorder::DrawGlyphsRecorder(Recorder& owner, DrawGlyphsDeconstruction)
     : m_owner(owner)
 {
 }

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp	2020-11-06 01:45:40 UTC (rev 269497)
@@ -38,11 +38,11 @@
 namespace WebCore {
 namespace DisplayList {
 
-Recorder::Recorder(GraphicsContext& context, DisplayList& displayList, const GraphicsContextState& state, const FloatRect& initialClip, const AffineTransform& initialCTM, Delegate* delegate)
+Recorder::Recorder(GraphicsContext& context, DisplayList& displayList, const GraphicsContextState& state, const FloatRect& initialClip, const AffineTransform& initialCTM, Delegate* delegate, DrawGlyphsRecorder::DrawGlyphsDeconstruction drawGlyphsDeconstruction)
     : GraphicsContextImpl(context, initialClip, AffineTransform())
     , m_displayList(displayList)
     , m_delegate(delegate)
-    , m_drawGlyphsRecorder(*this)
+    , m_drawGlyphsRecorder(*this, drawGlyphsDeconstruction)
 {
     LOG_WITH_STREAM(DisplayLists, stream << "\nRecording with clip " << initialClip);
     m_stateStack.append({ state, initialCTM, initialClip });

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h (269496 => 269497)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h	2020-11-06 01:27:14 UTC (rev 269496)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h	2020-11-06 01:45:40 UTC (rev 269497)
@@ -55,7 +55,7 @@
     WTF_MAKE_NONCOPYABLE(Recorder);
 public:
     class Delegate;
-    WEBCORE_EXPORT Recorder(GraphicsContext&, DisplayList&, const GraphicsContextState&, const FloatRect& initialClip, const AffineTransform&, Delegate* = nullptr);
+    WEBCORE_EXPORT Recorder(GraphicsContext&, DisplayList&, const GraphicsContextState&, const FloatRect& initialClip, const AffineTransform&, Delegate* = nullptr, DrawGlyphsRecorder::DrawGlyphsDeconstruction = DrawGlyphsRecorder::DrawGlyphsDeconstruction::Deconstruct);
     WEBCORE_EXPORT virtual ~Recorder();
 
     WEBCORE_EXPORT void putImageData(AlphaPremultiplication inputFormat, const ImageData&, const IntRect& srcRect, const IntPoint& destPoint, AlphaPremultiplication destFormat);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to