Bug 221744 – [GPU Process] Temporarily disable drawing OT-SVG glyphs in the GPU process until it can be implemented properly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[GPU Process] Temporarily disable drawing OT-SVG glyphs into canvas until it can be implemented properly

[Myles C. Maxfield]

Created attachment 419948 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/74222334>

[Myles C. Maxfield]

To-do:

1. Figure out how to make it build on non-internal OSes
2. Add ASSERT_WITH_SECURITY_IMPLICATION()s to make sure the GPUP doesn't parse OT-SVG content
3. Split this patch into two: one for the refactoring, and one for the actual change

We probably don't want to add any tests, because we don't want to guarantee that OT-SVG doesn't work... Instead, we would just temporarily disable any existing tests we have that make sure that drawing OT-SVG into canvas *does* work.

[Myles C. Maxfield]

Created attachment 420036 [details]
WIP

[Myles C. Maxfield]

isInGPUProcess()

[Myles C. Maxfield]

and RELEASE_ASSERT() is better to use here than ASSERT_WITH_SECURITY_IMPLICATION().

[Myles C. Maxfield]

Created attachment 420126 [details]
Patch

[Myles C. Maxfield]

Created attachment 420128 [details]
Patch

[Myles C. Maxfield]

Created attachment 420137 [details]
Patch

[Myles C. Maxfield]

Created attachment 420193 [details]
Patch

[Myles C. Maxfield]

Created attachment 420211 [details]
Patch

[Myles C. Maxfield]

Created attachment 420213 [details]
Patch

[Myles C. Maxfield]

Created attachment 420234 [details]
Patch

[Myles C. Maxfield]

Created attachment 420251 [details]
Patch

[Myles C. Maxfield]

Created attachment 420254 [details]
Patch

[Myles C. Maxfield]

Created attachment 420262 [details]
Patch

[Myles C. Maxfield]

Created attachment 420263 [details]
Patch

[Myles C. Maxfield]

Created attachment 420264 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 420264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420264&action=review

Presumably this will cause some new test failures in the GPUProcess bot?

> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:197
> +        for (unsigned i = 0; i < numGlyphs; ++i)
> +            RELEASE_ASSERT(!font.glyphIsRenderedWithOTSVG(glyphs[i]));

Wouldn't it be much more efficient to pass an array of glyphs to glyphIsRenderedWithOTSV()? This is also adding an extra traversal of the glyph array even in release builds.

[Myles C. Maxfield]

Created attachment 420348 [details]
Patch

[Myles C. Maxfield]

Created attachment 420359 [details]
Patch

[Myles C. Maxfield]

Created attachment 420399 [details]
Patch

[Myles C. Maxfield]

Created attachment 420401 [details]
Patch

[Myles C. Maxfield]

Created attachment 420421 [details]
Patch

[Myles C. Maxfield]

Created attachment 420426 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 420426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420426&action=review

> Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:34
> +OTSVGTable::OTSVGTable()
> +{
> +}

= default

> Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:44
> +    other.table = nullptr;

isn't this what std::exchange is for?

> Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:58
> +    table = other.table;
> +    other.table = nullptr;

std::exchange (or std::swap I forget)

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:785
> +            m_otSVGTable = PAL::OTSVGTable();

= { }?

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:793
> +        if (PAL::softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph(m_otSVGTable.value().table, glyphs[i]) != kCFNotFound) {

Is there any perf hit for calling softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph every time inside the loop?

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:376
> +struct GlyphsAndAdvancesStorage {
> +    Vector<GlyphBufferGlyph> glyphs;
> +    Vector<GlyphBufferAdvance> advances;
> +};
> +struct GlyphsAndAdvances {
> +    const GlyphBufferGlyph* glyphs;
> +    const GlyphBufferAdvance* advances;
> +    unsigned numGlyphs;
> +    GlyphBufferAdvance initialAdvance;
> +    Optional<GlyphsAndAdvancesStorage> storage;
> +};
> +static GlyphsAndAdvances filterOutOTSVGGlyphs(const Font& font, const GlyphBufferGlyph* glyphs, const GlyphBufferAdvance* advances, unsigned numGlyphs)

I'd like a few more blank lines here.

[Myles C. Maxfield]

Created attachment 420588 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 420800 [details]
Patch for committing

[Myles C. Maxfield]

The Windows errors look unrelated. I'll land tonight and watch the bots to make sure nothing bad happens.

[Myles C. Maxfield]

Committed r273121 (234318@main): <https://commits.webkit.org/234318@main>

[Myles C. Maxfield]

Comment on attachment 420426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420426&action=review

>> Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:44
>> +    other.table = nullptr;
> 
> isn't this what std::exchange is for?

std::swap()

>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:793
>> +        if (PAL::softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph(m_otSVGTable.value().table, glyphs[i]) != kCFNotFound) {
> 
> Is there any perf hit for calling softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph every time inside the loop?

Do you mean a perf hit because the function is soft linked, or a perf hit because we have to call it at all?

If you mean because it's soft linked, the macro is actually smart enough to only do the soft linking stuff the first time it's called. The function pointer initially gets set to a stub which will look up the real function and then assign it to the function pointer. So the runtime cost for soft-linking is minimal.

If you mean because we have to call it at all, we only call it for SVG fonts (see line 788 above), and almost no fonts are SVG fonts. So we're almost never going to run this code.