Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M122 public #232

Merged
merged 15 commits into from
Apr 30, 2024
Merged

M122 public #232

merged 15 commits into from
Apr 30, 2024

Conversation

HinTak
Copy link
Collaborator

@HinTak HinTak commented Feb 1, 2024

This should passed CI. But there is no reason of merging/upgrading at the moment, since all it differs is in default fontmgr being emulated.

@kyamagu I am not sure what is the problem, but the "FontConfigInterface" is the one being used up to 121 and being moved to chrome, but it segfaults , and somehow the "fontconfig" one, which was not and is not used anywhere, works. If you have time I'd appreciate your thought, but probably don't spend too much time on it, since we have something that "works", and this shouldn't be merged yet.

SkTypeface::MakeFromName is gone
SkTypeface::MakeFromFile also gone
SkTypeface::MakeFromData is gone

New lastResortMgr argument to SkTypeface::MakeDeserialize

Milestone 122
-------------
  * `SkFontMgr::RefDefault()` has been deleted. Clients should instantiate and manage their own
    `SkFontMgr`s and use them to explicitly create `SkTypeface`s

Fixes kyamagu#231

Adapted from
chromium/chromium@a6166e8
The FontConfigInterface version somehow segfaults.
@kyamagu
Copy link
Owner

kyamagu commented Feb 2, 2024

@HinTak As far as I look, the SkFontConfigInterface::RefGlobal() calls a static instance, but this static instance must be instantiated somewhere. It seems Chromium initializes a font loader in multiple places.

  sk_sp<SkFontConfigInterface> fci(SkFontConfigInterface::RefGlobal());
  return fci ? SkFontMgr_New_FCI(std::move(fci)) : nullptr;

@HinTak
Copy link
Collaborator Author

HinTak commented Feb 2, 2024

I believe we were using this particular SkFontMgr::Factory() in m121 - https://github.com/google/skia/blob/chrome/m121/src/ports/SkFontMgr_FontConfigInterface_factory.cpp - there are multiple implementations, only one is linked in per platform.

@kyamagu yes, chrome seems to be also use fontconfig directly in multiple places too, independently of skia...

@kyamagu
Copy link
Owner

kyamagu commented Feb 2, 2024

The singleton object is static and instantiated here.
https://github.com/google/skia/blob/main/src/ports/SkFontConfigInterface_direct_factory.cpp#L11

The nullptr to FcConfig* is probably okay, as most fontconfig APIs seem to use the default FcConfig*. But maybe fontconfig needs FcInit() called somewhere.
https://www.freedesktop.org/software/fontconfig/fontconfig-devel/x103.html

@kyamagu
Copy link
Owner

kyamagu commented Feb 2, 2024

Anyway, it is probably fine as long as SkFontMgr_New_FontConfig(nullptr) is working...

@HinTak
Copy link
Collaborator Author

HinTak commented Feb 2, 2024

The call chain is doing multiple std::move in the middle, so I am wondering if some of some of fontconfig internals gets out of scope at some point.

There is a curious breakage between 121 and 122 where for Mac os x, font(typeface("")) stops returning a "practically usable" font, and the glyph outlines come to none. Not surprising given the other recent changes, but that's a other reason not to merge this until 123 etc.

@HinTak HinTak mentioned this pull request Mar 10, 2024
@HinTak
Copy link
Collaborator Author

HinTak commented Apr 26, 2024

I'll put "don't use empty font(typeface("")) on mac os x" in the readme.m122 - it isn't something upstream wants you to do (an "any font") so it is not surprising it stops working at some point. Possibly upstream removed a "LEGACY..." define somewhere.

@HinTak
Copy link
Collaborator Author

HinTak commented Apr 27, 2024

I have added an explict test for font(typeface("Times")) returns non-empty glyphs for 'abcde' since somehow font(typeface("")) returns 5 empty ones m122 . It is not a surprise change given the other general no-default/valid-but-empty-default elsewhere, but it is a not-useful one for general users, so having a choice that would put some non-empty glyphs out is nice.

pavpanchekha and others added 2 commits April 30, 2024 04:32
The macOS detector macro is called `__APPLE__`, not `__apple__`
Revert "m122 Breakage on Mac OS X: NoneType not subscribable"

This reverts commit 43a5e63.

Revert "Typo - missing one bracket"

This reverts commit a526e34.

Revert "m122 Breakage: On Mac OS X, these two tests no longer give valid paths"

This reverts commit ecba963.
@HinTak HinTak merged commit c5b64b1 into kyamagu:main Apr 30, 2024
14 checks passed
@HinTak HinTak deleted the m122-public branch April 30, 2024 15:00
@HinTak
Copy link
Collaborator Author

HinTak commented Apr 30, 2024

Last:
2084 passed, 98 skipped, 11 xfailed, 27 warnings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants