Bug 217506 – [Cocoa] [GPU Process] Perform image decoding of color fonts in the Web Process

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

[Cocoa] [GPU Process] Perform image decoding of color fonts in the Web Process

[Myles C. Maxfield]

Created attachment 410919 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/69768186>

[Myles C. Maxfield]

I need to:

1. Split this patch up into 3 pieces
2. Work out the details (see comments in the WIP patch)
3. Add tests

[Simon Fraser (smfr)]

Comment on attachment 410919 [details]
WIP

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

Are we going to record all glyph drawing? Currently we don't have a way to measure perf impact (no motionmark test draws text into canvas); should we wait to land this until we can measure its perf impact?

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h:50
> +    DrawGlyphsRecorder(Recorder&);

explicit

[Myles C. Maxfield]

Created attachment 410998 [details]
WIP

[Myles C. Maxfield]

Created attachment 410999 [details]
WIP

[Myles C. Maxfield]

Created attachment 411002 [details]
WIP

[Myles C. Maxfield]

After playing around with this for a little while, the only canvas tests I could write that were broken by this patch are:

1. The placement of images and their shadows
2. Shadows of COLR glyphs

[Myles C. Maxfield]

Created attachment 411200 [details]
WIP

[Myles C. Maxfield]

Created attachment 411291 [details]
Almost ready

[Myles C. Maxfield]

Created attachment 411305 [details]
WIP

[Myles C. Maxfield]

Created attachment 411517 [details]
Needs tests

[Myles C. Maxfield]

Created attachment 411520 [details]
Needs tests

[Myles C. Maxfield]

Created attachment 411531 [details]
Ahem with sbix table

[Myles C. Maxfield]

Created attachment 411534 [details]
Needs COLR test

[Myles C. Maxfield]

Created attachment 411542 [details]
Patch

[Myles C. Maxfield]

This is almost ready.

Two things are remaining:
1. Performance testing
2. sbix images seem to be horizontally flipped

[Myles C. Maxfield]

3. There are a few FIXMEs

[Myles C. Maxfield]

Comment on attachment 411542 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:133
> +    updateCTM(m_originalCTM);

There might be more to do here.

[Myles C. Maxfield]

We probably should never set the CTM. We probably should always append a modification instead. Just in case, to be defensive.

[Myles C. Maxfield]

Created attachment 412368 [details]
WIP

Remaining: 1. Outline shadows are not drawn in the right place 2. Make sure non-emulated shadows work 3. Perf testing

[Myles C. Maxfield]

Created attachment 412395 [details]
WIP

Perf testing is the last remaining step

[Myles C. Maxfield]

Created attachment 412480 [details]
Patch

[Myles C. Maxfield]

Created attachment 412589 [details]
Patch

[Myles C. Maxfield]

Created attachment 412598 [details]
Patch

[Myles C. Maxfield]

Without patch:
With 1 canvas element: 125.58 | -1.87% / +1.35%
Only 1 canvas element: 169.66 | -1.97% / +1.48%

With patch:
With 1 canvas element: 126.87 | -1.68% / +1.08%
Only 1 canvas element: 171.21 | -1.54% / +1.67%

Not a regression!

[Myles C. Maxfield]

Created attachment 412603 [details]
Patch

[Myles C. Maxfield]

Created attachment 412617 [details]
Patch

[Myles C. Maxfield]

Tests in https://bugs.webkit.org/show_bug.cgi?id=218346

[Simon Fraser (smfr)]

Comment on attachment 412617 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/text/canvas-sbix.html

More words here please.

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h:103
> +    float m_originalShadowBlur;

initialize

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h:108
> +    float m_currentShadowBlur;

initialize

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorder.h:110
> +    bool m_currentShadowsIgnoreTransforms;

initialize

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:111
> +    m_originalFillColor = state.fillColor;
> +    m_originalFillGradient = state.fillGradient;
> +    m_originalFillPattern = state.fillPattern;
> +
> +    m_originalStrokeColor = state.strokeColor;
> +    m_originalStrokeGradient = state.strokeGradient;
> +    m_originalStrokePattern = state.strokePattern;
> +
> +    m_currentFillColor = m_originalFillColor;
> +    m_currentStrokeColor = m_originalStrokeColor;

Could you group things into structs to make this one assignment?

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:157
> +    m_originalShadowOffset = state.shadowOffset;
> +    m_originalShadowBlur = state.shadowBlur;
> +    m_originalShadowColor = state.shadowColor;
> +    m_originalShadowsIgnoreTransforms = state.shadowsIgnoreTransforms;
> +
> +    m_currentShadowOffset = m_originalShadowOffset;
> +    m_currentShadowBlur = m_originalShadowBlur;
> +    m_currentShadowColor = m_originalShadowColor;
> +    m_currentShadowsIgnoreTransforms = m_originalShadowsIgnoreTransforms;

Could also clean this up.

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:171
> +void DrawGlyphsRecorder::updateCTM(AffineTransform ctm)

const AffineTransform&

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:233
> +        newState.shadowOffset = shadowOffset;
> +        newState.shadowBlur = shadowBlur;
> +        newState.shadowColor = shadowColor;

Here too

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:246
> +    m_currentShadowOffset = shadowOffset;
> +    m_currentShadowBlur = shadowBlur;
> +    m_currentShadowColor = shadowColor;

Here too

[Myles C. Maxfield]

Created attachment 412706 [details]
Rebased

[Myles C. Maxfield]

Comment on attachment 412617 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:233
>> +        newState.shadowColor = shadowColor;
> 
> Here too

I can't do it here, because newState is a GraphicsContextState, and modifying that is outside the scope of this patch. I can do it in the above places, though.

>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:246
>> +    m_currentShadowColor = shadowColor;
> 
> Here too

I don't think it's a good fit in this particular place, because that 4th argument is a different type and has some logic on the line below. I've moved the shadow stuff into a struct for encapsulation, but I don't think this particular place can be a single line assignment.

[Myles C. Maxfield]

Created attachment 412710 [details]
Patch

[Myles C. Maxfield]

Created attachment 412714 [details]
Patch

[Myles C. Maxfield]

Created attachment 412757 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 412757 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        character, Core Text will end up telling Core Graphics to draw all the outlines, and then telling Core
> +        Graphics to draw the emoji image (using Core Graphics's normal image drawing routines).

This makes it sound like the emoji are z-ordered on top but I don't think they are.

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:106
> +    m_originalState.ctm = m_owner.currentState().ctm; // FIXME: Deal with device scale.

How important is this FIXME?

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:309
> +    // We want the GPU process's CTM and text matrix to match the current CTM and text matrix.

This code shouldn't reference the GPU process.

[Myles C. Maxfield]

Comment on attachment 412757 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:106
>> +    m_originalState.ctm = m_owner.currentState().ctm; // FIXME: Deal with device scale.
> 
> How important is this FIXME?

I think it's important. Right now our display list infrastructure doesn't handle retina displays. All display lists assume a 1x base CTM.

[Myles C. Maxfield]

Comment on attachment 412757 [details]
Patch

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

>>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:106
>>> +    m_originalState.ctm = m_owner.currentState().ctm; // FIXME: Deal with device scale.
>> 
>> How important is this FIXME?
> 
> I think it's important. Right now our display list infrastructure doesn't handle retina displays. All display lists assume a 1x base CTM.

I've rewritten this to be "// FIXME: Deal with base CTM."

[Myles C. Maxfield]

Created attachment 412778 [details]
Patch for committing

[Myles C. Maxfield]

Committed r269211: <https://trac.webkit.org/changeset/269211>

[Myles C. Maxfield]

Committed r269220: <https://trac.webkit.org/changeset/269220>