Summary: | [Cocoa] [GPU Process] Perform image decoding of color fonts in the Web Process | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, ews-watchlist, gyuyoung.kim, mmaxfield, ryuan.choi, sergio, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 217539, 217541, 217749, 218346 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2020-10-09 00:10:17 PDT
Created attachment 410919 [details]
WIP
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 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 Created attachment 410998 [details]
WIP
Created attachment 410999 [details]
WIP
Created attachment 411002 [details]
WIP
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 Created attachment 411200 [details]
WIP
Created attachment 411291 [details]
Almost ready
Created attachment 411305 [details]
WIP
Created attachment 411517 [details]
Needs tests
Created attachment 411520 [details]
Needs tests
Created attachment 411531 [details]
Ahem with sbix table
Created attachment 411534 [details]
Needs COLR test
Created attachment 411542 [details]
Patch
This is almost ready. Two things are remaining: 1. Performance testing 2. sbix images seem to be horizontally flipped 3. There are a few FIXMEs 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. We probably should never set the CTM. We probably should always append a modification instead. Just in case, to be defensive. 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
Created attachment 412395 [details]
WIP
Perf testing is the last remaining step
Created attachment 412480 [details]
Patch
Created attachment 412589 [details]
Patch
Created attachment 412598 [details]
Patch
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! Created attachment 412603 [details]
Patch
Created attachment 412617 [details]
Patch
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 Created attachment 412706 [details]
Rebased
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. Created attachment 412710 [details]
Patch
Created attachment 412714 [details]
Patch
Created attachment 412757 [details]
Patch
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. 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. 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." Created attachment 412778 [details]
Patch for committing
Committed r269211: <https://trac.webkit.org/changeset/269211> Committed r269220: <https://trac.webkit.org/changeset/269220> |