| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
The last commit subsets and embeds Type 42 PS/EPS, here's the size difference in outputs: nosub-dejavu.ps ----> 1.2 MB sub-dejavu.ps ----> 8.5 kB (To test it out on your machine, apply this patch for nosub-dejavu.ps:) diff --git a/lib/matplotlib/backends/backend_ps.py b/lib/matplotlib/backends/backend_ps.py
index 2a3ab64a1c..ed0af4ea85 100644
--- a/lib/matplotlib/backends/backend_ps.py
+++ b/lib/matplotlib/backends/backend_ps.py
@@ -997,12 +997,8 @@ class FigureCanvasPS(FigureCanvasBase):
) as tmp:
tmp.write(fontdata)
tmp.seek(0, 0)
- font = FT2Font(tmp.name)
- glyph_ids = [
- font.get_char_index(c) for c in chars
- ]
convert_ttf_to_ps(
- os.fsencode(tmp.name),
+ os.fsencode(font_path),
fh,
fonttype,
glyph_ids,The script: import matplotlib.pyplot as plt
plt.rcParams["ps.fonttype"] = 42
plt.figtext(0.5, 0.5, "hello")
# plt.savefig('sub-dejavu.ps') # before applying the patch
# plt.savefig('nosub-dejavu.ps') # after applying the patch |
Sorry, something went wrong.
I think it's difficult to test get_glyphs_subset, since it really depends on font files, and if we chose certain characters to test with a fixed font (lets say), there's no guarantee that the subset will be the same for different versions of the library. Is there any other way to test this? edit: @jklymak this isn't a draft anymore, just needed to address review comments 😄 |
Sorry, something went wrong.
|
You can mark as ready-to-review at any point. I just kick things to Draft so they don't show up in the queue if the PR requires action on the part of the author. (this one is still not passing the tests). |
Sorry, something went wrong.
Yeah, CI isn't installing fonttools, which is why NoModuleFound is all over the place (even after adding it to minvers.txt: #20391 (comment)) |
Sorry, something went wrong.
|
Install is done with --no-deps, so hard dependencies also need to be listed here: https://github.com/matplotlib/matplotlib/blob/master/.github/workflows/tests.yml#L148 minver.txt is only a constraint on versions. |
Sorry, something went wrong.
We ship DejaVu ourselves, so that should always be available for testing.
A file with just 'A' embedded should be smaller than one with the full alphabet, say? |
Sorry, something went wrong.
Oh, if we just test the subset being 'smaller' yeah we can do it, but not with the exact number glyphs, since that will definitely vary. (we also set recommeded_glyphs=True) But even so, isn't that the 'full-time job' of the fonttools library itself? or in other words wouldn't testing the get_glyphs_subset function be the same as testing the library itself (that it does reduce the number of glyphs)? |
Sorry, something went wrong.
|
Oh, and one more thing (sorry for not remembering this earlier): the PDF specification has some extra requirements for font subsets in section 9.6.4. The PostScript name of subsetted fonts needs to have a prepended tag of six random uppercase letters and a plus sign, e.g. EOODIA+Poetica if the original font name is Poetica. This is relevant in the case that multiple Matplotlib plots are combined in the same document, e.g. in a LaTeX paper with several figures. The random tags prevent collisions between the different versions of the same font. |
Sorry, something went wrong.
I think we don't do this even for Type 3 subsetting? |
Sorry, something went wrong.
|
This breaks the test_determinism_check: matplotlib/lib/matplotlib/tests/test_determinism.py Lines 85 to 88 in 593de35 This is for obvious reasons, since we have random string inside font postscript name. (due to the specification) |
Sorry, something went wrong.
|
Then the string shouldn't be random, but perhaps something derived from the subset of glyphs? Take hash(frozenset(glyphs)), convert it to base 26 and take the first six letters, or something like that. The PDF specification only requires the tag for Type 1 and TrueType fonts. It probably doesn't hurt with Type 3 fonts either. |
Sorry, something went wrong.
That totally makes sense! Let me try this out 👍🏼 |
Sorry, something went wrong.
|
@sauerburger can you suggest a test for that, or is it already in #20633? |
Sorry, something went wrong.
The test is already in, and that is exactly what is breaking the CI for this PR. |
Sorry, something went wrong.
|
It seems to me that the requested changes have been made. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
This PR is a fresh rebase from #18143.
(we already have an external ttconv dependency, which does not handle subsetting)
Possibly fixes #11303 (large file sizes)
Fixes #18191.
PR Checklist