Skip to content
Permalink
Browse files
Don't propagate GraphicsContextState change bits into TextPainter's g…
…lyph 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.

Canonical link: https://commits.webkit.org/250397@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@293951 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
heycam committed May 7, 2022
1 parent ba1d40b commit 2804ea0e1fc54839995e6ae7c78346d9db39bcc3
Showing 15 changed files with 204 additions and 2 deletions.
@@ -1,3 +1,14 @@
2022-05-07 Cameron McCormack <heycam@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_gardner@apple.com>

Fix flakey test by using the old API on old systems.
@@ -5116,6 +5116,9 @@ imported/w3c/web-platform-tests/workers/modules/dedicated-worker-import-csp.html
# 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 ]
@@ -0,0 +1,6 @@

(draw-glyphs
(local-anchor (0,0))
(anchor-point (0,0))
(length 9) extent at (0,0) size 0x0)

@@ -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>
@@ -1,3 +1,68 @@
2022-05-07 Cameron McCormack <heycam@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_hsieh@apple.com>

Block-style image overlay containers should have lighter shadows
@@ -213,7 +213,7 @@ std::unique_ptr<DisplayList::InMemoryDisplayList> FontCascade::displayListForTex
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);
@@ -36,6 +36,13 @@ GraphicsContextState::GraphicsContextState(const ChangeFlags& changeFlags, Inter
{
}

GraphicsContextState GraphicsContextState::cloneForRecording() const
{
auto clone = *this;
clone.m_changeFlags = { };
return clone;
}

bool GraphicsContextState::containsOnlyInlineChanges() const
{
if (m_changeFlags != (m_changeFlags & basicChangeFlags))
@@ -68,6 +68,8 @@ class GraphicsContextState {
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 }); }
@@ -51,6 +51,7 @@ Recorder::Recorder(const GraphicsContextState& state, const FloatRect& initialCl
, m_initialScale(initialCTM.xScale())
, m_deconstructDrawGlyphs(deconstructDrawGlyphs)
{
ASSERT(!state.changes());
m_stateStack.append({ state, initialCTM, initialCTM.mapRect(initialClip) });
}

@@ -65,6 +65,11 @@ class GlyphDisplayListCache {
return nullptr;
}

DisplayList::DisplayList* getIfExists(const LayoutRun& run)
{
return m_glyphRunMap.get(&run);
}

void remove(const LayoutRun& run)
{
m_glyphRunMap.remove(&run);
@@ -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 @@ void TextPainter::clearGlyphDisplayLists()
#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
@@ -73,8 +73,16 @@ class TextPainter {

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,
@@ -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 @@ void Internals::resetToConsistentState(Page& page)
auto& sessionManager = reinterpret_cast<MediaSessionManagerGLib&>(PlatformMediaSessionManager::sharedManager());
sessionManager.setDBusNotificationsEnabled(false);
#endif

TextPainter::setForceUseGlyphDisplayListForTesting(false);
}

Internals::Internals(Document& document)
@@ -3280,6 +3283,32 @@ ExceptionOr<String> Internals::replayDisplayListForElement(Element& element, uns
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();
@@ -482,6 +482,9 @@ class Internals final : public RefCounted<Internals>, private ContextDestruction
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();
@@ -610,6 +610,9 @@ typedef (FetchRequest or FetchResponse) FetchObject;
// 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);

0 comments on commit 2804ea0

Please sign in to comment.