- Revision
- 293951
- Author
- hey...@apple.com
- Date
- 2022-05-07 16:34:10 -0700 (Sat, 07 May 2022)
Log Message
Don't propagate GraphicsContextState change bits into TextPainter's glyph display list recorder
https://bugs.webkit.org/show_bug.cgi?id=239952
<rdar://problem/92635604>
Source/WebCore:
Reviewed by Said Abou-Hallawa and Antti Koivisto.
In FontCascade::displayListForTextRun, we create a
DisplayList::Recorder, then call drawGlyphBuffer. We initialize the
DisplayList::Recorder with the GraphicsContextState of the
GraphicsContext we're drawing to. Just before this, we will have set the
current fill color on that GraphicsContext.
When GPUP DOM rendering is disabled, GraphicsContextCG responds to
setFillColor etc. by updating GraphicsContextState, including setting
the Change flag, then immediately updating the CGContext, and clearing
the Change flag.
But when GPUP DOM rendering is enabled, the GraphicsContext is a
DisplayList::Recorder for the layer we're painting in to. Because
DisplayList::Recorder applies its state changes lazily, it can be in the
situation where its GraphicsContextState has had the fill brush changed,
and the Change flag is still set. So DisplayList::Recorder starts off
with a GraphicsContextState with unapplied changes in it. We end up in
DisplayList::Recorder::drawGlyphsAndCacheFont, which calls
appendStateChangeItemIfNecessary, which sees that the Change bit is set,
and generates a SetInlineFillColor display list item, which is
recorded and then replayed the next time the same text is painted.
This recorded fill color then may be wrong for the next TextPainter
that wants to reuse the cached glyph display list.
Display list recorders should never be initialized with a
GraphicsContextState that has change flags set on it. We can assert
this, then make FontCascade explicitly clear those flags on the state
object it passes in to the DisplayList::Recorder.
Test: fast/text/glyph-display-list-color.html
* platform/graphics/FontCascade.cpp:
(WebCore::FontCascade::displayListForTextRun const):
* platform/graphics/GraphicsContextState.cpp:
(WebCore::GraphicsContextState::cloneForRecording const):
* platform/graphics/GraphicsContextState.h:
* platform/graphics/displaylists/DisplayListRecorder.cpp:
(WebCore::DisplayList::Recorder::Recorder):
Add setForceUseGlyphDisplayListForTesting and
cachedGlyphDisplayListsForTextNode functions on Internal for the
test to use:
* rendering/GlyphDisplayListCache.h:
(WebCore::GlyphDisplayListCache::getIfExists):
* rendering/TextPainter.cpp:
(WebCore::TextPainter::shouldUseGlyphDisplayList):
(WebCore::TextPainter::setForceUseGlyphDisplayListForTesting):
(WebCore::TextPainter::cachedGlyphDisplayListsForTextNodeAsText):
* rendering/TextPainter.h:
(WebCore::TextPainter::glyphDisplayListIfExists):
* testing/Internals.cpp:
(WebCore::Internals::setForceUseGlyphDisplayListForTesting):
(WebCore::Internals::cachedGlyphDisplayListsForTextNode):
* testing/Internals.h:
* testing/Internals.idl:
LayoutTests:
Reviewed by Antti Koivisto.
* fast/text/glyph-display-list-color-expected.txt: Added.
* fast/text/glyph-display-list-color.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (293950 => 293951)
--- trunk/LayoutTests/ChangeLog 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/LayoutTests/ChangeLog 2022-05-07 23:34:10 UTC (rev 293951)
@@ -1,3 +1,14 @@
+2022-05-07 Cameron McCormack <hey...@apple.com>
+
+ Don't propagate GraphicsContextState change bits into TextPainter's glyph display list recorder
+ https://bugs.webkit.org/show_bug.cgi?id=239952
+ <rdar://problem/92635604>
+
+ Reviewed by Antti Koivisto.
+
+ * fast/text/glyph-display-list-color-expected.txt: Added.
+ * fast/text/glyph-display-list-color.html: Added.
+
2022-05-06 Megan Gardner <megan_gard...@apple.com>
Fix flakey test by using the old API on old systems.
Modified: trunk/LayoutTests/TestExpectations (293950 => 293951)
--- trunk/LayoutTests/TestExpectations 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/LayoutTests/TestExpectations 2022-05-07 23:34:10 UTC (rev 293951)
@@ -5116,6 +5116,9 @@
# due to how MessagePort::dispatchMessages() is implemented.
imported/w3c/web-platform-tests/workers/shared-worker-name-via-options.html [ DumpJSConsoleLogInStdErr Failure Pass ]
+# Display list logging is only available in debug
+[ Release ] fast/text/glyph-display-list-color.html [ Skip ]
+
# Plugins
# FIXME: Remove these tests.
plugins/ [ Skip ]
Added: trunk/LayoutTests/fast/text/glyph-display-list-color-expected.txt (0 => 293951)
--- trunk/LayoutTests/fast/text/glyph-display-list-color-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/text/glyph-display-list-color-expected.txt 2022-05-07 23:34:10 UTC (rev 293951)
@@ -0,0 +1,6 @@
+
+(draw-glyphs
+ (local-anchor (0,0))
+ (anchor-point (0,0))
+ (length 9) extent at (0,0) size 0x0)
+
Added: trunk/LayoutTests/fast/text/glyph-display-list-color.html (0 => 293951)
--- trunk/LayoutTests/fast/text/glyph-display-list-color.html (rev 0)
+++ trunk/LayoutTests/fast/text/glyph-display-list-color.html 2022-05-07 23:34:10 UTC (rev 293951)
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<script>
+if (window.testRunner) {
+ testRunner.waitUntilDone();
+ testRunner.dumpAsText();
+}
+if (window.internals)
+ internals.setForceUseGlyphDisplayListForTesting(true);
+</script>
+<body _onload_="run()">
+<div id=text style="color: red;">Some text</div>
+<pre id=output></pre>
+<script>
+function run() {
+ requestAnimationFrame(function() {
+ requestAnimationFrame(function() {
+ if (window.internals) {
+ internals.setForceUseGlyphDisplayListForTesting(false);
+ output.textContent = internals.cachedGlyphDisplayListsForTextNode(text.firstChild);
+ text.remove();
+ }
+ if (window.testRunner)
+ testRunner.notifyDone();
+ });
+ });
+}
+</script>
Modified: trunk/Source/WebCore/ChangeLog (293950 => 293951)
--- trunk/Source/WebCore/ChangeLog 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/ChangeLog 2022-05-07 23:34:10 UTC (rev 293951)
@@ -1,3 +1,68 @@
+2022-05-07 Cameron McCormack <hey...@apple.com>
+
+ Don't propagate GraphicsContextState change bits into TextPainter's glyph display list recorder
+ https://bugs.webkit.org/show_bug.cgi?id=239952
+ <rdar://problem/92635604>
+
+ Reviewed by Said Abou-Hallawa and Antti Koivisto.
+
+ In FontCascade::displayListForTextRun, we create a
+ DisplayList::Recorder, then call drawGlyphBuffer. We initialize the
+ DisplayList::Recorder with the GraphicsContextState of the
+ GraphicsContext we're drawing to. Just before this, we will have set the
+ current fill color on that GraphicsContext.
+
+ When GPUP DOM rendering is disabled, GraphicsContextCG responds to
+ setFillColor etc. by updating GraphicsContextState, including setting
+ the Change flag, then immediately updating the CGContext, and clearing
+ the Change flag.
+
+ But when GPUP DOM rendering is enabled, the GraphicsContext is a
+ DisplayList::Recorder for the layer we're painting in to. Because
+ DisplayList::Recorder applies its state changes lazily, it can be in the
+ situation where its GraphicsContextState has had the fill brush changed,
+ and the Change flag is still set. So DisplayList::Recorder starts off
+ with a GraphicsContextState with unapplied changes in it. We end up in
+ DisplayList::Recorder::drawGlyphsAndCacheFont, which calls
+ appendStateChangeItemIfNecessary, which sees that the Change bit is set,
+ and generates a SetInlineFillColor display list item, which is
+ recorded and then replayed the next time the same text is painted.
+ This recorded fill color then may be wrong for the next TextPainter
+ that wants to reuse the cached glyph display list.
+
+ Display list recorders should never be initialized with a
+ GraphicsContextState that has change flags set on it. We can assert
+ this, then make FontCascade explicitly clear those flags on the state
+ object it passes in to the DisplayList::Recorder.
+
+ Test: fast/text/glyph-display-list-color.html
+
+ * platform/graphics/FontCascade.cpp:
+ (WebCore::FontCascade::displayListForTextRun const):
+ * platform/graphics/GraphicsContextState.cpp:
+ (WebCore::GraphicsContextState::cloneForRecording const):
+ * platform/graphics/GraphicsContextState.h:
+ * platform/graphics/displaylists/DisplayListRecorder.cpp:
+ (WebCore::DisplayList::Recorder::Recorder):
+
+ Add setForceUseGlyphDisplayListForTesting and
+ cachedGlyphDisplayListsForTextNode functions on Internal for the
+ test to use:
+
+ * rendering/GlyphDisplayListCache.h:
+ (WebCore::GlyphDisplayListCache::getIfExists):
+ * rendering/TextPainter.cpp:
+ (WebCore::TextPainter::shouldUseGlyphDisplayList):
+ (WebCore::TextPainter::setForceUseGlyphDisplayListForTesting):
+ (WebCore::TextPainter::cachedGlyphDisplayListsForTextNodeAsText):
+ * rendering/TextPainter.h:
+ (WebCore::TextPainter::glyphDisplayListIfExists):
+ * testing/Internals.cpp:
+ (WebCore::Internals::setForceUseGlyphDisplayListForTesting):
+ (WebCore::Internals::cachedGlyphDisplayListsForTextNode):
+ * testing/Internals.h:
+ * testing/Internals.idl:
+
2022-05-06 Wenson Hsieh <wenson_hs...@apple.com>
Block-style image overlay containers should have lighter shadows
Modified: trunk/Source/WebCore/platform/graphics/FontCascade.cpp (293950 => 293951)
--- trunk/Source/WebCore/platform/graphics/FontCascade.cpp 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/platform/graphics/FontCascade.cpp 2022-05-07 23:34:10 UTC (rev 293951)
@@ -213,7 +213,7 @@
return nullptr;
std::unique_ptr<DisplayList::InMemoryDisplayList> displayList = makeUnique<DisplayList::InMemoryDisplayList>();
- DisplayList::RecorderImpl recordingContext(*displayList, context.state(), FloatRect(), AffineTransform(), DrawGlyphsRecorder::DeconstructDrawGlyphs::No);
+ DisplayList::RecorderImpl recordingContext(*displayList, context.state().cloneForRecording(), FloatRect(), AffineTransform(), DrawGlyphsRecorder::DeconstructDrawGlyphs::No);
FloatPoint startPoint = toFloatPoint(WebCore::size(glyphBuffer.initialAdvance()));
drawGlyphBuffer(recordingContext, glyphBuffer, startPoint, customFontNotReadyAction);
Modified: trunk/Source/WebCore/platform/graphics/GraphicsContextState.cpp (293950 => 293951)
--- trunk/Source/WebCore/platform/graphics/GraphicsContextState.cpp 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/platform/graphics/GraphicsContextState.cpp 2022-05-07 23:34:10 UTC (rev 293951)
@@ -36,6 +36,13 @@
{
}
+GraphicsContextState GraphicsContextState::cloneForRecording() const
+{
+ auto clone = *this;
+ clone.m_changeFlags = { };
+ return clone;
+}
+
bool GraphicsContextState::containsOnlyInlineChanges() const
{
if (m_changeFlags != (m_changeFlags & basicChangeFlags))
Modified: trunk/Source/WebCore/platform/graphics/GraphicsContextState.h (293950 => 293951)
--- trunk/Source/WebCore/platform/graphics/GraphicsContextState.h 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/platform/graphics/GraphicsContextState.h 2022-05-07 23:34:10 UTC (rev 293951)
@@ -68,6 +68,8 @@
ChangeFlags changes() const { return m_changeFlags; }
void didApplyChanges() { m_changeFlags = { }; }
+ GraphicsContextState cloneForRecording() const;
+
const SourceBrush& fillBrush() const { return m_fillBrush; }
void setFillBrush(const SourceBrush& brush) { setProperty(Change::FillBrush, &GraphicsContextState::m_fillBrush, brush); }
void setFillColor(const Color& color) { setProperty(Change::FillBrush, &GraphicsContextState::m_fillBrush, { color }); }
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp (293950 => 293951)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp 2022-05-07 23:34:10 UTC (rev 293951)
@@ -51,6 +51,7 @@
, m_initialScale(initialCTM.xScale())
, m_deconstructDrawGlyphs(deconstructDrawGlyphs)
{
+ ASSERT(!state.changes());
m_stateStack.append({ state, initialCTM, initialCTM.mapRect(initialClip) });
}
Modified: trunk/Source/WebCore/rendering/GlyphDisplayListCache.h (293950 => 293951)
--- trunk/Source/WebCore/rendering/GlyphDisplayListCache.h 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/rendering/GlyphDisplayListCache.h 2022-05-07 23:34:10 UTC (rev 293951)
@@ -65,6 +65,11 @@
return nullptr;
}
+ DisplayList::DisplayList* getIfExists(const LayoutRun& run)
+ {
+ return m_glyphRunMap.get(&run);
+ }
+
void remove(const LayoutRun& run)
{
m_glyphRunMap.remove(&run);
Modified: trunk/Source/WebCore/rendering/TextPainter.cpp (293950 => 293951)
--- trunk/Source/WebCore/rendering/TextPainter.cpp 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/rendering/TextPainter.cpp 2022-05-07 23:34:10 UTC (rev 293951)
@@ -27,6 +27,7 @@
#include "FilterOperations.h"
#include "GraphicsContext.h"
#include "HTMLParserIdioms.h"
+#include "InlineIteratorTextBox.h"
#include "LayoutIntegrationInlineContent.h"
#include "LegacyInlineTextBox.h"
#include "RenderCombineText.h"
@@ -226,9 +227,40 @@
#endif
}
+static bool forceUseGlyphDisplayListForTesting = false;
+
bool TextPainter::shouldUseGlyphDisplayList(const PaintInfo& paintInfo)
{
- return !paintInfo.context().paintingDisabled() && paintInfo.enclosingSelfPaintingLayer() && paintInfo.enclosingSelfPaintingLayer()->paintingFrequently();
+ return !paintInfo.context().paintingDisabled() && paintInfo.enclosingSelfPaintingLayer() && (paintInfo.enclosingSelfPaintingLayer()->paintingFrequently() || forceUseGlyphDisplayListForTesting);
}
+void TextPainter::setForceUseGlyphDisplayListForTesting(bool enabled)
+{
+ forceUseGlyphDisplayListForTesting = enabled;
+}
+
+String TextPainter::cachedGlyphDisplayListsForTextNodeAsText(Text& textNode, DisplayList::AsTextFlags flags)
+{
+ if (!textNode.renderer())
+ return String();
+
+ StringBuilder builder;
+
+ for (auto textBox : InlineIterator::textBoxesFor(*textNode.renderer())) {
+ DisplayList::DisplayList* displayList = nullptr;
+ if (auto* legacyInlineBox = textBox.legacyInlineBox())
+ displayList = TextPainter::glyphDisplayListIfExists(*legacyInlineBox);
+#if ENABLE(LAYOUT_FORMATTING_CONTEXT)
+ else
+ displayList = TextPainter::glyphDisplayListIfExists(*textBox.inlineBox());
+#endif
+ if (displayList) {
+ builder.append(displayList->asText(flags));
+ builder.append('\n');
+ }
+ }
+
+ return builder.toString();
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/rendering/TextPainter.h (293950 => 293951)
--- trunk/Source/WebCore/rendering/TextPainter.h 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/rendering/TextPainter.h 2022-05-07 23:34:10 UTC (rev 293951)
@@ -73,8 +73,16 @@
static void clearGlyphDisplayLists();
static bool shouldUseGlyphDisplayList(const PaintInfo&);
+ WEBCORE_EXPORT static void setForceUseGlyphDisplayListForTesting(bool);
+ WEBCORE_EXPORT static String cachedGlyphDisplayListsForTextNodeAsText(Text&, DisplayList::AsTextFlags);
private:
+ template<typename LayoutRun>
+ static DisplayList::DisplayList* glyphDisplayListIfExists(const LayoutRun& run)
+ {
+ return GlyphDisplayListCache<LayoutRun>::singleton().getIfExists(run);
+ }
+
void paintTextOrEmphasisMarks(const FontCascade&, const TextRun&, const AtomString& emphasisMark, float emphasisMarkOffset,
const FloatPoint& textOrigin, unsigned startOffset, unsigned endOffset);
void paintTextWithShadows(const ShadowData*, const FilterOperations*, const FontCascade&, const TextRun&, const FloatRect& boxRect, const FloatPoint& textOrigin,
Modified: trunk/Source/WebCore/testing/Internals.cpp (293950 => 293951)
--- trunk/Source/WebCore/testing/Internals.cpp 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/testing/Internals.cpp 2022-05-07 23:34:10 UTC (rev 293951)
@@ -215,6 +215,7 @@
#include "StyleSheetContents.h"
#include "SystemSoundManager.h"
#include "TextIterator.h"
+#include "TextPainter.h"
#include "TextPlaceholderElement.h"
#include "TextRecognitionOptions.h"
#include "ThreadableBlobRegistry.h"
@@ -642,6 +643,8 @@
auto& sessionManager = reinterpret_cast<MediaSessionManagerGLib&>(PlatformMediaSessionManager::sharedManager());
sessionManager.setDBusNotificationsEnabled(false);
#endif
+
+ TextPainter::setForceUseGlyphDisplayListForTesting(false);
}
Internals::Internals(Document& document)
@@ -3280,6 +3283,32 @@
return layer->backing()->replayDisplayListAsText(displayListFlags);
}
+void Internals::setForceUseGlyphDisplayListForTesting(bool enabled)
+{
+ TextPainter::setForceUseGlyphDisplayListForTesting(enabled);
+}
+
+ExceptionOr<String> Internals::cachedGlyphDisplayListsForTextNode(Node& node, unsigned short flags)
+{
+ Document* document = contextDocument();
+ if (!document || !document->renderView())
+ return Exception { InvalidAccessError };
+
+ if (!is<Text>(node))
+ return Exception { InvalidAccessError };
+
+ node.document().updateLayoutIgnorePendingStylesheets();
+
+ if (!node.renderer())
+ return Exception { InvalidAccessError };
+
+ DisplayList::AsTextFlags displayListFlags = 0;
+ if (flags & DISPLAY_LIST_INCLUDES_PLATFORM_OPERATIONS)
+ displayListFlags |= DisplayList::AsTextFlag::IncludesPlatformOperations;
+
+ return TextPainter::cachedGlyphDisplayListsForTextNodeAsText(downcast<Text>(node), displayListFlags);
+}
+
ExceptionOr<void> Internals::garbageCollectDocumentResources() const
{
Document* document = contextDocument();
Modified: trunk/Source/WebCore/testing/Internals.h (293950 => 293951)
--- trunk/Source/WebCore/testing/Internals.h 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/testing/Internals.h 2022-05-07 23:34:10 UTC (rev 293951)
@@ -482,6 +482,9 @@
ExceptionOr<String> displayListForElement(Element&, unsigned short flags);
ExceptionOr<String> replayDisplayListForElement(Element&, unsigned short flags);
+ void setForceUseGlyphDisplayListForTesting(bool enabled);
+ ExceptionOr<String> cachedGlyphDisplayListsForTextNode(Node&, unsigned short flags);
+
ExceptionOr<void> garbageCollectDocumentResources() const;
void beginSimulatedMemoryPressure();
Modified: trunk/Source/WebCore/testing/Internals.idl (293950 => 293951)
--- trunk/Source/WebCore/testing/Internals.idl 2022-05-07 18:15:14 UTC (rev 293950)
+++ trunk/Source/WebCore/testing/Internals.idl 2022-05-07 23:34:10 UTC (rev 293951)
@@ -610,6 +610,9 @@
// Returns the display list that was actually painted.
DOMString replayDisplayListForElement(Element element, optional unsigned short flags = 0);
+ undefined setForceUseGlyphDisplayListForTesting(boolean enabled);
+ DOMString cachedGlyphDisplayListsForTextNode(Node node, optional unsigned short flags = 0);
+
undefined garbageCollectDocumentResources();
undefined insertAuthorCSS(DOMString css);
