Skip to content

Support (first font of) TTC files. #9787

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

Merged
merged 2 commits into from
Jan 4, 2019
Merged

Support (first font of) TTC files. #9787

merged 2 commits into from
Jan 4, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 15, 2017

TTC is a TrueType Collection file (a collection of TTFs). Currently,
ft2font only supports getting the first font from the collection, and
neither the pdf nor ps backends are able to use them (due to limitations
of ttconv). Still, it seems better than nothing to support them for Agg
and SVG (that comes for free via FreeType).

xref #3135, #3912.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer
Copy link
Contributor Author

anntzer commented Nov 15, 2017

  • Some tests fail on Travis because the font cache is not reset. I will manually delete the font cache for a CI re-run if there is general agreement over the PR.

  • Supporting multiple fonts from a single ttc file is going to be a bit complicated to do properly, not so much on the ft2font/_ft2 side (it's just passing an additional index to the ctor), but because many places right now assume that you can use a font's pathname as cache key.

@timhoffm
Copy link
Member

Could one use something like pathname[1] as caching key if there are multiple fonts in a file?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 21, 2018

I'd rather just do it properly and switch everything to (pathname, index) (which is the standard representation FreeType uses, see https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_New_Face (pathname, face_index)). But that's out of scope for this PR.

@clouds56
Copy link

What's blocking? May I help here?

@timhoffm
Copy link
Member

We need a second positive review from one of the matplotlib reviewers before the PR can be merged.

@anntzer Travis Python 3.7 now fails with:

________________________________ test_find_ttc _________________________________
[gw1] linux -- Python 3.7.0 /home/travis/virtualenv/python3.7.0/bin/python
    @pytest.mark.xfail(not os.environ.get("TRAVIS"), reason="Font may be missing.")
    def test_find_ttc():
        fp = FontProperties(family=["WenQuanYi Zen Hei"])
        font = findfont(fp)
>       assert os.path.basename(font) == "wqy-zenhei.ttc"
E       AssertionError: assert 'DejaVuSans.ttf' == 'wqy-zenhei.ttc'
E         - DejaVuSans.ttf
E         + wqy-zenhei.ttc
lib/matplotlib/tests/test_font_manager.py:106: AssertionError

This has to be investigated before merging. Is the font missing there?

@anntzer
Copy link
Contributor Author

anntzer commented Nov 25, 2018

This has to be investigated before merging. Is the font missing there?

That's because I need to temporarily force the use of new font caches. Added a second commit that does so; then we can later revert that commit.

@anntzer anntzer force-pushed the ttc branch 2 times, most recently from 72d25de to ccd00ac Compare November 25, 2018 14:42
@Stephen-Chilcote
Copy link
Contributor

Some of the metadata seems to end up wrong when using this. FT2Font.get_sfnt() gives back garbage on most (with a few odd exceptions) .TTC fonts. This makes certain fonts inaccessible - in my specific case, I'm asking for Microsoft YaHei and ending up with Microsoft YaHei light, and there's no way to specifically get the normal weight version.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 5, 2018

I guess that may be because YaHei has the light font at index 0 and the regular one at index 1? (I don't have access to it.)
In any case this PR never pretended to support fonts after the first one.

@Stephen-Chilcote
Copy link
Contributor

Stephen-Chilcote commented Dec 6, 2018

No, light and normal are separate TTCs (the whole point of .ttc is that it efficiently stores fonts that have many of the same glyphs; packing completely different versions of the same font doesn't do you any good.) The problem is that MatPlotLib can't load their metadata aside from the font family and sees them as identical.

YaHei should come preinstalled on every Windows machine with 8 or later. I see similar issues with most .ttcs on my system, though maybe it's something unique Microsoft is doing. I only saw it work correctly with non-CJK fonts though I imagine that's coincidence.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 6, 2018

Oh, I see. I think it's basically the same issue as #8550? (in which case once again it's orthogonal to ttc support)

@Stephen-Chilcote
Copy link
Contributor

Stephen-Chilcote commented Dec 6, 2018

No; it's the same end result, but in that case the issue was failure to correctly interpret the metadata, whereas here the metadata comes back as junk. The font is named "YaHei Light;" if it could read the name it would assign it the correct weight, it just can't read the name.

With a conditional breakpoint on font_manager.py:342 with the condition set to 'ttc' in font.fname, I see sfnt full of unreadable bytes, as opposed to when it loads a .ttf font where it's full of ASCII strings. Do you not see this same behavior?

@anntzer
Copy link
Contributor Author

anntzer commented Dec 6, 2018

No, the family name (as loaded by FreeType into the family_name field of FT_FaceRec) is 'Microsoft YaHei'.
Now there are additional names that are available via the sfnt table mechanism, but we don't load them because they are in a bunch of random encodings (specifically, here they are in some MS encodings which are UTF-16-BE) (https://github.com/anntzer/freetypybind/blob/master/src/_ft2.cpp#L317 for some common cases...). For fonts that have sfnt entries encoded as mac_roman (that's the 1, 0, 0, 2/4 at the top of the function) (I guess these are the ttfs you loaded) we load that metadata (and even then we don't use the family name entry from it), but otherwise we don't bother.

@Stephen-Chilcote
Copy link
Contributor

Stephen-Chilcote commented Dec 6, 2018

Ah, I get it now. Okay, this is a Microsoft being weird issue, not a .ttc issue. Weird how it worked before the .ttc switch, but it's not your problem. Thanks for the info.

TTC is a TrueType Collection file (a collection of TTFs).  Currently,
ft2font only supports getting the first font from the collection, and
neither the pdf nor ps backends are able to use them (due to limitations
of ttconv).  Still, it seems better than nothing to support them for Agg
and SVG (that comes for free via FreeType).
@jklymak
Copy link
Member

jklymak commented Jan 4, 2019

@anntzer Looks like you need to do a commit/revert dance on this PR because of the cache? If so, I think you can self-merge, and then do the revert commit soon after? (not sure how big a deal it is to leave master in an uncahched state)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 4, 2019

I'll open a PR to restore the cache (that's what tests that the cache actually works...) immediately after this is merged (e.g. by you? :)).

@jklymak
Copy link
Member

jklymak commented Jan 4, 2019

Sure, I just wanted to make sure you were around to do it. If now is good, here it goes...

@jklymak jklymak merged commit cc5c188 into matplotlib:master Jan 4, 2019
@anntzer anntzer deleted the ttc branch January 4, 2019 17:32
@anntzer anntzer mentioned this pull request Jan 4, 2019
6 tasks
@QuLogic QuLogic modified the milestones: v3.0.3, v3.1 Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants