Update FreeType to 2.13.3 #29816
Update FreeType to 2.13.3 #29816
Conversation
|
Another size test: So once everything is repacked and compressed by git, the repo only gains 16M! |
|
If we're going to regen the baselines, we should also take that opportunity to remove some of the backcompat knobs -- at least text.kerning_factor (which clearly only existed to avoid regen'ing the baselines), and possibly text.hinting_factor as well (at least I would switch the default to 1 and generate the images with that amount of hinting, even if we decide to keep this functionality). |
|
Rather than regenerating the images at this point, I would start by pulling the tolerances used by Fedora upstream (so we can backport that to 3.10.x) and then in a second step regenerate the images on main and push the tolerances back to 0. |
Yes, good point; I will see what effect that has on things.
Fedora doesn't have any other tolerances other than the 6 other files already changed (though in this PR I changed the results instead of the tolerance, we could do like in Fedora instead). For image tests, we overlay the correct images from my |
|
Removing
I'll commit these all separately to make it easier to review, but we can squash these all together if it's all working fine. The majority of these are things like |
|
On a related note, and I'm not sure which ones are relevant here, but I wonder if we should drop several PDF and SVG test images when they would be changed but don't effectively test anything different from the PNG. |
|
For the repo size problem we could provide some guidance for the depth flag so most contributors don't need to download the whole thing. Eg --depth=500 gets you the last two years of commits and is x MB. Most folks don't need the full commit history (or even anything more than depth=1) |
|
The first image comparison that shows up Is it beneficial/necessary to grab the images from that other repo you've been keeping, or would it make sense to blow away the images directory on CI and move all the resultant images into the directory to start fresh in a sense? |
Surprisingly, this doesn't appear to make any difference to test images. It only affects 9 tests, and only with regards to bboxes and tight layout that are explicitly compared. This doesn't matter whether I try with FreeType 2.6.1 or 2.13.3.
Ah, you're right. I had not done a deep dive into the changes, but likely this was an image that had text that was removed, so it was regenerated from the FreeType changes, but effectively looked the same. I will go through these again and make sure we don't have any extraneous changes like this one.
The other repo was useful for tracking purposes and a quick changeover, but it might be best to start fresh and make sure we don't change too many extra images. |
|
I haven't updated this PR for it yet, but going over the diffs, one surprising change is that while the mathtext tests with DejaVu Sans look better (more consistent baselines, tweaks to kerning), the same ones with Computer Modern look worse (mostly due to a wobbly baseline.) But on the other hand, a lot of those had the wobbly baseline before... I've also opened #29827 to remove most of the redundant PDFs that would be changed here (except the ones from |
|
See also #14177 (comment) and #22459 re: wobbly baseline. I have some hopes that switching agg+usetex to rasterizing glyphs ourselves (see also #29807) would be one way to fix that. |
|
Slightly unrelated, but if we decide to update this many PNGs, it should be a good time to add an oxipng pre-commit hook. https://github.com/shssoichiro/oxipng I would expect that the sizes are halved or so on average. (It would be useful independent of this, but I have not gotten around to propose it and it would be unfortunate to change all those images without compression.) |
But these are not Unfortunately, updating FreeType still causes changes in those test images due to different kerning and metrics (the whole line may move up/down a pixel, but individual glyphs no longer do.) So I'm going to roll that change in here for the changed images to be squashed together. |
|
I've now rebased this on top of #29827, and gone through and removed the extra image changes. Now it's about 60% the number it was before. As I expect this will be squashed, there are a few things here:
|
|
Are the new tests also using hinting_factor=1? |
|
No, as I mentioned earlier, that barely changed anything. I'm trying to see if that's true or whether I didn't set it up right. |
|
For the minimum version builds, it looks like we're hitting mesonbuild/meson-python#424, which was fixed in meson-python 0.13.2, so I guess we'll have to bump that requirement (but it's already 0.13.1, so that's not a big jump.) |
|
Hmm, something seems a bit off with the kerning. For example, https://github.com/matplotlib/matplotlib/pull/29816/files#diff-a98ee9405471b4a49ef6fa8d8083e4a485936d4d8f8568782b8e1d70bc311f3e spreads the |
Namely, `text.hinting` is now `default` instead of `force_autohint` (or `none` for classic tests) and `text.hinting_factor` is now 1, not 8.
If we've updated an image in the past couple commits, then we can remove the backwards-compatibility styling so that they're generated as intended.
The `FontInfo.num` value returned by `TruetypeFonts._get_info` is a character code, but `FT2Font.get_kerning` takes *glyph indices*, meaning that kerning was likely off in most cases.
|
After discussing it a bit, I think the kerning is fine (or as good as it'll be.) I was misled by only testing "Text" and not "2D Text" as in the test. With the additional "2D ", the With hinting-factor=1: In these images, on the left is "2D Text" and just "Text" to the right, at 12pt and 24pt. With "2D " to shift the The reason for the change is that FreeType kerning is rounded to integer pixels; with hinting-factor=8, the "pixels" are 8 times smaller and so can be a little closer than with a 1 factor. Unfortunately, the libraqm update will shrink this kerning back to similar results as before. We did discuss perhaps setting hinting-factor=8 now and only changing to 1 with libraqm (which does require it or else everything is broken) since libraqm will shrink the kerning again. However, reverting that change will cause 1100 image changes whereas libraqm is only about 90, so I don't think we should do that. I've rebased this PR, and squashed the image changes into the last change. I've also run oxipng on them to shrink them down. |
|
Now that I remember it, this may also be the opportunity to remove a few hacks in mathtext. In particular the following should be quite clear: diff --git i/lib/matplotlib/_mathtext.py w/lib/matplotlib/_mathtext.py
index 415033b9ac..059dd6d5dc 100644
--- i/lib/matplotlib/_mathtext.py
+++ w/lib/matplotlib/_mathtext.py
@@ -149,20 +149,20 @@ class Output:
d = ymax - ymin - self.box.height
image = FT2Image(int(np.ceil(w)), int(np.ceil(h + max(d, 0))))
- # Ideally, we could just use self.glyphs and self.rects here, shifting
- # their coordinates by (-xmin, -ymin), but this yields slightly
- # different results due to floating point slop; shipping twice is the
- # old approach and keeps baseline images backcompat.
- shifted = ship(self.box, (-xmin, -ymin))
-
- for ox, oy, info in shifted.glyphs:
+ for ox, oy, info in self.glyphs:
+ ox -= xmin
+ oy -= ymin
info.font.draw_glyph_to_bitmap(
image,
int(ox),
int(oy - np.ceil(info.metrics.iceberg)),
info.glyph,
antialiased=antialiased)
- for x1, y1, x2, y2 in shifted.rects:
+ for x1, y1, x2, y2 in self.rects:
+ x1 -= xmin
+ x2 -= xmin
+ y1 -= ymin
+ y2 -= ymin
height = max(int(y2 - y1) - 1, 0)
if height == 0:
center = (y2 + y1) / 2(a few baseline images change, which can be squashed with the changes here) The idea is that the mathtext parser first places the glyphs relative to the "pen position at the start of the string" (during the There's a few other changes in mathtext (essentially related to fixing #15313/#15339, still broken with this PR) that would also result in tiny baseline image changes that I'd like to figure out and also squash with the changes here, but I would need more time to fully flesh them out so I would not hold this PR up for that. |
|
I also realized that the changes here (likely the "wobbly-baseline fixing" one, although I didn't check) make some mathtext rendering clearly worse, e.g. |
|
I pushed to https://github.com/anntzer/matplotlib/tree/mt1 an updated version of The idea of the change is basically the following: mathtext currently rasterizes glyphs by passing the coordinate of their top left corner to draw_glyph_to_bitmap (which makes pasting the rasterized glyph bitmap into the whole image more convenient, but leads to wobbly baselines if one isn't careful with rounding and hinting); instead, modify draw_glyph_to_bitmap (which should likely be renamed into another function) to accept glyph origins, which can be directly passed from the mathtext glyph layouting algorithm. While at it, also restore the subpixel glyph localization code which was commented out (e.g. sub_offset in draw_glyph_to_bitmap). A few changes which I think are for the better: new mathfont_dejavusans_60: note the more homogeneous spacing between the "bfit" letters This also fixes #15313. (Note that I have another good reason to have the mathtext rasterizer work using glyph origins instead of glyph topleft corners: I would like to reuse it to rasterize usetex dvi's, which will allow getting rid of the dvipng dependency (this is in particular needed for luatex/xetex support, as dvipng doesn't support their dvi dialects, but is a worthwhile improvement in itself too, I think).) |
I was thinking this had something to do with #18389, which was a long-standing issue so I didn't worry about it too much yet.
That was merged and seems to be a docs change; did you mean a different PR?
Do you think this would also help with #29551? That seemed to be a problem with subpixel positioning.
This is definitely something that annoyed me. and happy to see it fixed better. |
Possible, didn't really investigate.
Typo, should have been #15339. Edited in the message above as well.
Testing #29551 (comment) but switching to mathtext ( The correct fix for that problem is likely to completely get rid of the intermediate mathtext raster and directly rasterize to the Agg raster at the correct position. (This is essentially mplcairo's approach, and would also get rid of the need to figure out the exact size of the FT2Image to allocate, which is surprisingly complicated (see comments in Output.to_raster() in the mt1 branch).)
|
|
Further note for self: check whether #18181's backcompat hack can be removed and folded with the changes here. Edit: The change would be the patch below, which indeed again changes a bunch of baseline images; I haven't checked how much are files already touched by this PR. diff --git i/lib/matplotlib/backends/backend_pdf.py w/lib/matplotlib/backends/backend_pdf.py
index 4c6bb266e0..702110b1f0 100644
--- i/lib/matplotlib/backends/backend_pdf.py
+++ w/lib/matplotlib/backends/backend_pdf.py
@@ -617,40 +617,18 @@ def _get_pdf_charprocs(font_path, glyph_ids):
procs = {}
for glyph_id in glyph_ids:
g = font.load_glyph(glyph_id, LoadFlags.NO_SCALE)
- # NOTE: We should be using round(), but instead use
- # "(x+.5).astype(int)" to keep backcompat with the old ttconv code
- # (this is different for negative x's).
- d1 = (np.array([g.horiAdvance, 0, *g.bbox]) * conv + .5).astype(int)
+ d1 = [
+ round(g.horiAdvance * conv), 0,
+ # Round bbox corners *outwards*, so that they indeed bound th glyph.
+ math.floor(g.bbox[0] * conv), math.floor(g.bbox[1] * conv),
+ math.ceil(g.bbox[2] * conv), math.ceil(g.bbox[3] * conv),
+ ]
v, c = font.get_path()
- v = (v * 64).astype(int) # Back to TrueType's internal units (1/64's).
- # Backcompat with old ttconv code: control points between two quads are
- # omitted if they are exactly at the midpoint between the control of
- # the quad before and the quad after, but ttconv used to interpolate
- # *after* conversion to PS units, causing floating point errors. Here
- # we reproduce ttconv's logic, detecting these "implicit" points and
- # re-interpolating them. Note that occasionally (e.g. with DejaVu Sans
- # glyph "0") a point detected as "implicit" is actually explicit, and
- # will thus be shifted by 1.
- quads, = np.nonzero(c == 3)
- quads_on = quads[1::2]
- quads_mid_on = np.array(
- sorted({*quads_on} & {*(quads - 1)} & {*(quads + 1)}), int)
- implicit = quads_mid_on[
- (v[quads_mid_on] # As above, use astype(int), not // division
- == ((v[quads_mid_on - 1] + v[quads_mid_on + 1]) / 2).astype(int))
- .all(axis=1)]
- if (font.postscript_name, glyph_id) in [
- ("DejaVuSerif-Italic", 77), # j
- ("DejaVuSerif-Italic", 135), # \AA
- ]:
- v[:, 0] -= 1 # Hard-coded backcompat (FreeType shifts glyph by 1).
- v = (v * conv + .5).astype(int) # As above re: truncation vs rounding.
- v[implicit] = (( # Fix implicit points; again, truncate.
- (v[implicit - 1] + v[implicit + 1]) / 2).astype(int))
+ v = (v * 64 * conv).round() # Back to TrueType's internal units (1/64's).
procs[font.get_glyph_name(glyph_id)] = (
" ".join(map(str, d1)).encode("ascii") + b" d1\n"
+ _path.convert_to_string(
- Path(v, c), None, None, False, None, -1,
+ Path(v, c), None, None, False, None, 0,
# no code for quad Beziers triggers auto-conversion to cubics.
[b"m", b"l", b"", b"c", b"h"], True)
+ b"f")
diff --git i/src/_path.h w/src/_path.h
index c037037767..1b54426c7e 100644
--- i/src/_path.h
+++ w/src/_path.h
@@ -1066,38 +1066,25 @@ void quad2cubic(double x0, double y0,
void __add_number(double val, char format_code, int precision,
std::string& buffer)
{
- if (precision == -1) {
- // Special-case for compat with old ttconv code, which *truncated*
- // values with a cast to int instead of rounding them as printf
- // would do. The only point where non-integer values arise is from
- // quad2cubic conversion (as we already perform a first truncation
- // on Python's side), which can introduce additional floating point
- // error (by adding 2/3 delta-x and then 1/3 delta-x), so compensate by
- // first rounding to the closest 1/3 and then truncating.
- char str[255];
- PyOS_snprintf(str, 255, "%d", (int)(round(val * 3)) / 3);
- buffer += str;
- } else {
- char *str = PyOS_double_to_string(
- val, format_code, precision, Py_DTSF_ADD_DOT_0, nullptr);
- // Delete trailing zeros and decimal point
- char *c = str + strlen(str) - 1; // Start at last character.
- // Rewind through all the zeros and, if present, the trailing decimal
- // point. Py_DTSF_ADD_DOT_0 ensures we won't go past the start of str.
- while (*c == '0') {
- --c;
- }
- if (*c == '.') {
- --c;
- }
- try {
- buffer.append(str, c + 1);
- } catch (std::bad_alloc& e) {
- PyMem_Free(str);
- throw e;
- }
+ char *str = PyOS_double_to_string(
+ val, format_code, precision, Py_DTSF_ADD_DOT_0, nullptr);
+ // Delete trailing zeros and decimal point
+ char *c = str + strlen(str) - 1; // Start at last character.
+ // Rewind through all the zeros and, if present, the trailing decimal
+ // point. Py_DTSF_ADD_DOT_0 ensures we won't go past the start of str.
+ while (*c == '0') {
+ --c;
+ }
+ if (*c == '.') {
+ --c;
+ }
+ try {
+ buffer.append(str, c + 1);
+ } catch (std::bad_alloc& e) {
PyMem_Free(str);
+ throw e;
}
+ PyMem_Free(str);
}
|
PR summary
On the call last week, we considered whether it made sense to do this upgrade. This PR shows the effect of doing so. Note for simplicity, I simply copied the new images from https://github.com/QuLogic/mpl-images, and then updated any new images directly. It's possible that there were some extraneously changed images as a result. I will check this shortly.
The latest FreeType version supports many newer font features, and fixes several CVEs, which we were mostly hoping were not relevant to us, but this may or may not be true. Additionally, I've found that libraqm runs into issues with hinting in older versions. While I'm not entirely sure which of libraqm or FreeType holds that bug, updating libraqm to new versions would also require newer FreeType anyway.
The main concern here was the repo size. Locally, I started with a completely fresh clone before creating this commit. The
.gitdirectory increased from 461M to 482M, or an increase of 21M / 4.6%.PR checklist