Bug 55609 – option to use skia's font backend when drawing text on windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mike Reed]

Today when the skia platform draws text on windows, it draws from paths extracted from GDI. This patch optionally uses Skia's native text drawing APIs which are faster as they draw from cached bitmaps (built from GDI).

[Mike Reed]

Created attachment 84458 [details]
optionally use skia's font backend

[WebKit Review Bot]

Attachment 84458 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:291:  LOCAL_GLYPHS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mike Reed]

Created attachment 84462 [details]
fix style complaint

[James Robinson]

Comment on attachment 84462 [details]
fix style complaint

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

Why we would add another macro for this?  If this API only changes behavior "significantly" (what does that mean?) in the SKIA_GPU path, why not tie this behavior to the SKIA_GPU path directly and not have the extra macro?  Maintaining a large set of combinations of macros is a code maintenance headache.

Also, why is this change only for windows?  We use skia on linux as well.

> WebCore/ChangeLog:1
> [phone]  Mike Reed  <[e-mail]>

This patch is rooted inside Source/ instead of the root of the repository.  This still works, but it breaks several tools (for example expanding context in the review tool).  Please generate patches from the root of the repository.

> WebCore/ChangeLog:10
> +        No new tests. text output should be essitially identical. 

typo "essentially"

I'm curious what "essentially identical" means given that we do pixel-exact testing.  If there are subtle differences, that means we'll have to maintain another set of baselines (which is probably unavoidable for text in GPU rendered cases, but worth keeping track of).

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:47
> +extern SkTypeface* SkCreateTypefaceFromLOGFONT(const LOGFONT&);

this is odd - why don't you include the correct header for this function?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:50
> +#if ENABLE(SKIA_GPU)
> +    #define USE_SKIA_TEXT_API

Do we want to turn this on for all content whenever SKIA_GPU is set, or just content rendered by a SKIA_GPU accelerated context?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:51
> +//  #define FORCE_SKIA_TEXT_FOR_ALL

why check in commented out code?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:296
> +    static const size_t kLocalGlyphMax = 64;
> +    SkAutoSTArray<kLocalGlyphMax, SkPoint> posStorage(numGlyphs);
> +    SkPoint* pos = posStorage.get();
> +    SkScalar x = point.fX;
> +    SkScalar y = point.fY;
> +    for (int i = 0; i < numGlyphs; i++) {

What happens when numGlyphs >= 64?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:333
> +    /*
> +        Much of this logic could also happen in
> +        FontCustomPlatformData::fontPlatformData and be cached,
> +        allowing us to avoid talking to GDI at this point.
> +    */
> +    LOGFONT info;

In WebKit this sort of comment is normally a FIXME and written with // in front of each line.

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:337
> +    if (size < 0)
> +        size = -size;

huh?  this deserves a comment at least

[Mike Reed]

Created attachment 84781 [details]
use SKIA_TEXT to opt-in to new code path

[James Robinson]

Comment on attachment 84781 [details]
use SKIA_TEXT to opt-in to new code path

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

Looks good.  If you'd like I can fix the comment nitpicks and land myself, or you can upload a new patch for commit-queue.

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:287
> +    // reallocate space on the stack. If numGlyphs is larger, the array
> +    // will dynamically alocate it.

nit: "reallocate" -> "Initially allocate space on the stack for 64 entries" or something like that.
"alocate" -> "allocate".

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:335
> +        size = -size; // we don't let GDI dpi-scale us (see SkFontHost_win.cpp

Thanks for the comment.  nits: "we"->"We", needs a trailing ")."

[Mike Reed]

Since its near the end of the day for the East, it would be great if you fixed the comments and did the land. Thanks.

[James Robinson]

Committed r80386: <http://trac.webkit.org/changeset/80386>