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

Implement Font-Fallback in Matplotlib #20740

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

aitikgupta
Copy link
Contributor

@aitikgupta aitikgupta commented Jul 25, 2021

PR Summary

This PR modifies the internal structure of FT2Font (the interface between fonts and Matplotlib) in favor of implementing Font Fallback for Matplotlib, and allow Agg backend to use the new codepath.
It builds on the previous PR: #20549, which was the 'first-step', i.e., "parsing multiple families".. this PR implements using those families for font fallback.

This would help us in multi-language support, for example (Previous / After):

^the fonts are chosen such that the difference is visually noticeable.

A flowchart explaining the text rendering algorithm with font fallback:
FontFallback

Here's the script:

import matplotlib.pyplot as plt
# "Authentic" is a fancy font, whereas "SimHei" is a CJK font
plt.rcParams['font.family'] = ['Authentic', 'SimHei']
plt.rcParams['font.size'] = 30

plt.figtext(0.18, 0.45, "There are 多个汉字 in between!")
plt.show()

Fixes #18883, #15260

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented Jul 25, 2021

To actually trigger this workflow, I'll push similar commits similar to #20549..
Note: this is only tested against Agg backend (since agg is pretty straightforward in terms of font usage)

Also, there's a lot of print/debug statements throughout, I'll remove them once this moves ahead..

@aitikgupta aitikgupta marked this pull request as draft Jul 26, 2021
@aitikgupta aitikgupta force-pushed the fallback-revamp branch 2 times, most recently from 9aee34b to 9fabbc7 Compare Aug 6, 2021
@aitikgupta aitikgupta changed the title Implement Font-Fallback in Matplotlib Implement Font-Fallback for Agg backend Aug 6, 2021
@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented Aug 7, 2021

Note: There's some structural changes to how FT2Font is handling stuff in #20804, which builds on this PR..

EDIT: I've merged the FT2Font changes together for both PRs

@aitikgupta aitikgupta force-pushed the fallback-revamp branch 2 times, most recently from 0fa0625 to e868fbc Compare Aug 7, 2021
@aitikgupta aitikgupta marked this pull request as ready for review Aug 7, 2021
@aitikgupta aitikgupta changed the title Implement Font-Fallback for Agg backend Implement Font-Fallback in Matplotlib Aug 13, 2021
@tacaswell tacaswell added this to Medium Priority in v3.6 Milestone Aug 13, 2021
jkseppan added a commit to jkseppan/matplotlib that referenced this issue Aug 20, 2021
On Linux and Mac. Add a font_manager test that loads the font and
uses it. This could be useful in testing the multi-font support
(matplotlib#20740 etc).

I couldn't get the font installed on Windows. There is a Chocolatey
installer that installs all of the Noto fonts, which takes a really
long time to run.
@jklymak
Copy link
Member

@jklymak jklymak commented Oct 4, 2021

@aitikgupta Do these have a review team looking at them?

@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented Oct 4, 2021

@jklymak I don't think anybody is actively reviewing these font handling PRs at the moment.. but they're ready for review and not drafts (I'll resolve the conflicts)

@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented Oct 22, 2021

Going ahead and removing the test, since font distributions for different Linux versions across CI are different.. and thus their kerning information is different -- different visual outputs.

We can test it with a self-shipped CJK font, which is guaranteed to be present across all platforms.. but that'd be another problem because their file sizes are huge.

@anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 27, 2021

(Unfortunately, I will not have the bandwidth to review this in the coming days.)

lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved
lib/matplotlib/font_manager.py Show resolved Hide resolved
lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved
lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved
lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved
src/ft2font_wrapper.cpp Outdated Show resolved Hide resolved
src/ft2font_wrapper.cpp Show resolved Hide resolved
src/ft2font_wrapper.cpp Show resolved Hide resolved
src/ft2font_wrapper.cpp Outdated Show resolved Hide resolved
src/ft2font_wrapper.cpp Show resolved Hide resolved
@jklymak
Copy link
Member

@jklymak jklymak commented Jan 14, 2022

Hi @aitikgupta we were all discussing this PR yesterday - did you have an opportunity to rebase and address the review comments?

@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented Jan 16, 2022

Hey @jklymak, I'm currently out sick. Will get back to this as soon as I can, probably a week or two. Apologies!

@jklymak
Copy link
Member

@jklymak jklymak commented Jan 16, 2022

No rush, get better!

@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented Jan 23, 2022

Hey @QuLogic, addressed all the comments (added comments for further discussions), and brought the PR up to speed!

@tacaswell
Copy link
Member

@tacaswell tacaswell commented May 25, 2022

Rebased to get CI to re-run and to make sure nothing has changed too badly under us.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented May 25, 2022

0477b9e and 02a140c are the commits we need to revert now after the rebase.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented May 25, 2022

I also had a local commit with

diff --git a/src/ft2font_wrapper.cpp b/src/ft2font_wrapper.cpp
index 0017617b75..8ced8e1e92 100644
--- a/src/ft2font_wrapper.cpp
+++ b/src/ft2font_wrapper.cpp
@@ -263,7 +263,7 @@ struct PyFT2Font
     Py_ssize_t shape[2];
     Py_ssize_t strides[2];
     Py_ssize_t suboffsets[2];
-    std::vector<PyObject *> fallbacks;
+    PyObject* fallback_tuple;
 };
 
 static unsigned long read_from_file_callback(FT_Stream stream,
@@ -386,22 +386,32 @@ static int PyFT2Font_init(PyFT2Font *self, PyObject *args, PyObject *kwds)
     open_args.flags = FT_OPEN_STREAM;
     open_args.stream = &self->stream;
 
-    if (fallback_list && PyList_Check(fallback_list)) {
-        Py_ssize_t size = PyList_Size(fallback_list);
-
-        for (Py_ssize_t i = 0; i < size; ++i) {
-            // this returns a borrowed reference
-            PyObject* item = PyList_GetItem(fallback_list, i);
-            // Increase the ref count, we will undo this in dealloc
-            // this makes sure things do not get gc'd under us!
-            Py_INCREF(item);
-            self->fallbacks.push_back(item);
-            // TODO: check whether item is actually an FT2Font
-            FT2Font *fback = reinterpret_cast<PyFT2Font *>(item)->x;
-            // Also (locally) cache the underlying FT2Font objects
-            fallback_fonts.push_back(fback);
-        }
+    if (PyList_Check(fallback_list)) {
+        // we got a list
+        self->fallback_tuple = PyList_AsTuple(fallback_list);
+    } else if (PyTuple_Check(fallback_list)) {
+        // we got a tuple
+        PyErr_SetString(PyExc_TypeError, "Tuple not supported yet");
+       goto exit;
+    } else if (fallback_list == Py_None) {
+        // we got None
+        self->fallback_tuple = PyTuple_New(0)
+    } else {
+        // we got anything else!
+        PyErr_SetString(PyExc_TypeError, "Must be list, tuple, or None");
+       goto exit;
+    }
 
+    Py_ssize_t size = PyTuple_GET_SIZE(self->fallback_tuple);
+    fallback_fonts.resize(size);
+
+    for (Py_ssize_t i = 0; i < size; ++i) {
+        // this returns a borrowed reference
+        PyObject* item = PyTuple_GET_ITEM(self->fallback_tuple, i);
+        // TODO: check whether item is actually an FT2Font
+        FT2Font *fback = reinterpret_cast<PyFT2Font *>(item)->x;
+        // Also (locally) cache the underlying FT2Font objects
+        fallback_fonts.push_back(fback);
     }
 
     if (PyBytes_Check(filename) || PyUnicode_Check(filename)) {
@@ -437,9 +447,7 @@ exit:
 static void PyFT2Font_dealloc(PyFT2Font *self)
 {
     delete self->x;
-    for (size_t i = 0; i < self->fallbacks.size(); i++) {
-      Py_DECREF(self->fallbacks[i]);
-    }
+    Py_DECREF(self->fallback_tuple);
 
     Py_XDECREF(self->py_file);
     Py_TYPE(self)->tp_free((PyObject *)self);

in it, but I do not remember it's state....

@tacaswell
Copy link
Member

@tacaswell tacaswell commented May 25, 2022

I think we should ignore the git cleanliness issue until we get this all the way sorted and then do a final re-write of history, the context of exactly what @aitikgupta was thinking is the work was going in is still way to valuable to risk me squashing it away ;)

@QuLogic
Copy link
Member

@QuLogic QuLogic commented May 25, 2022

  1. I think there still might be something weird with the referencing and such, but I need to go over it all again.
  2. a. Definitely. b. No, that can be a separate PR.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented May 25, 2022

and even after fixing a few compiler errors, that diff definetily causes segfaults, so not obviously a good idea yet....

Co-authored-by: Aitik Gupta <aitikgupta@gmail.com>
@tacaswell
Copy link
Member

@tacaswell tacaswell commented May 25, 2022

0477b9e and 02a140c are the commits we need to revert now after the rebase.

To do this naively we also need to back out 3aeba2b and d0b3e0b so we should try to come up with a different way to expose this information to the Python layer....

@aitikgupta
Copy link
Contributor Author

@aitikgupta aitikgupta commented May 25, 2022

0477b9e and 02a140c are the commits we need to revert now after the rebase.

To do this naively we also need to back out 3aeba2b and d0b3e0b so we should try to come up with a different way to expose this information to the Python layer....

fill_glyphs family and the exposing of caches isn't needed for Agg fallback to work, so we can probably leave this discussion to #20804 and #20832?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend/agg Release critical status: waiting for other PR topic: text/fonts
Projects
v3.6 Milestone
High Priority
Development

Successfully merging this pull request may close these issues.

7 participants