Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 #41592

timmaffett · 2023-04-28T19:37:56Z

This PR fixes font-subset to check to see if a font is a variable font with variable font axes before additionally dropping the GSUB/GPOS/GDEF tables. These tables were being forced dropped in all cases (even though harfbuzz had been modified to always keep them). I made the change only drop the tables for variable fonts to preserve the previous behavior in the most possible cases.

This PR fixes #125704.

To see the bug (or verify it is fixed) in the live web examples below you must select the font variations Fill->1 and Weight->100.
(See issue #125704 for more details).

The issue #125704 includes examples of the font-subset being used (and breaking) the variable fonts, as well as example of the --no-tree-shake-icons being used where the fonts do not break.

Additionally, I have created an additional live example where this PR has been applied to font-subset and icon tree shaking is still taking place.

(Example w/ icon tree-shaking using the broken font-subset for icon tree shaking is found here ).

In the example build output below note that the non-variable fonts "CupertinoIcons.ttf" and "MaterialIcons-Regular.otf" have the same size savings as before the change, but the variable fonts "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf", "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf", and "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" now have a much more reasonable saving of ~2% because every icon in the font is included in the example. The previous extra ~30% savings was from having the GSUB table removed. The 30% size savings for a tree-shaking for a case where every icon is used in the example probably should have been suspect.. lol.

Output of build using fixed font-subset.exe live fix example :

flutter build web --release --web-renderer canvaskit --base-href "/material_symbols_icons_showing_tree_shake_fixed/"

Font asset "CupertinoIcons.ttf" was tree-shaken, reducing it from 283452 to 1236 bytes (99.6% reduction). Tree-shaking can be disabled by providing the
--no-tree-shake-icons flag when building your app.
Font asset "MaterialIcons-Regular.otf" was tree-shaken, reducing it from 1645184 to 10808 bytes (99.3% reduction). Tree-shaking can be disabled by providing the
--no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 5848492 to 5683212 bytes (2.8% reduction). Tree-shaking can be disabled by
providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 6944756 to 6779476 bytes (2.4% reduction). Tree-shaking can be disabled
by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 9361824 to 9196544 bytes (1.8% reduction). Tree-shaking can be disabled
by providing the --no-tree-shake-icons flag when building your app.
Compiling lib\main.dart for the Web...                             79.5s

BEFORE font-subset fix live bug example here:

flutter build web --release --web-renderer canvaskit --base-href "/material_symbols_icons_showing_tree_shake_bug/"

Font asset "CupertinoIcons.ttf" was tree-shaken, reducing it from 283452 to 1236 bytes (99.6% reduction). Tree-shaking
can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 5848492 to 4079548 bytes
(30.2% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 6944756 to 4781576 bytes(31.1% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialIcons-Regular.otf" was tree-shaken, reducing it from 1645184 to 10808 bytes (99.3% reduction).
Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 9361824 to 6397020 bytes
(31.7% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Compiling lib\main.dart for the Web...                             63.8s

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-04-28T19:37:59Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

timmaffett · 2023-04-28T23:40:28Z

I have now added tests to the PR. All tests that are performed on the existing (non variable) test font are now repeated on a variable font. This variable font is a smaller subset of the Material Symbols icon outlined font.

The supplied golden fonts (1variable.ttf, 2variable.ttf and 3variable.ttf) were verified to be correct using fonttools ttx to create hand inspectable .ttx files and these were compared to the corresponding versions created with the pyftsubset tool. The output of font-subset and pyftsubset where identical for the 1, 2 and 3 codepoint tests (other than the ttx checkSumAdjustment entry).

All tests were verified to pass.

jonahwilliams · 2023-04-29T16:40:30Z

tools/font-subset/main.cc

@@ -83,7 +87,7 @@ int main(int argc, char** argv) {

  HarfbuzzWrappers::HbBlobPtr font_blob(
      hb_blob_create_from_file(input_file_path.c_str()));
-  if (!hb_blob_get_length(font_blob.get())) {
+  if (!hb_blob_get_length(font_blob.get())) {cd 


Suggested change

if (!hb_blob_get_length(font_blob.get())) {cd

if (!hb_blob_get_length(font_blob.get())) {

timmaffett · 2023-04-29T18:05:05Z

Fixed.

Embarrassing! I apologize for that careless typo - I remember that unexpected request to save that file now.. I am usually more careful than that I promise ;)
Too many vscode and console windows open - that's my excuse.

edit: post mortem - That accidently snuck in when I was adding the test.py commit with the tests.

dnfield · 2023-05-01T16:51:02Z

CI is complaining about formatting issues.

When I build and run this locally on an arm64 mac, some of the tests are failing. They also fail with your code changes commented out.

timmaffett · 2023-05-01T17:23:53Z

I saw the complaint about formatting - I checked for any hidden tab chars or trailing spaces and did not find any. I tried to format things as the existing code is formatted.. Is there a formatting tool that can be used on the .cc files ?

I am running the test locally on my windows machine. The mac CI tests seem to be passing?
Which tests are you seeing fail ?

The added if statement should only change the previous behavior when there is both variable font tables found within the font, and these tables contain variable axes definitions. This should prevent it from doing anything in any fonts that may have 'empty' variable font tables included. In all other cases the output should be identical (ie any GSUB/GPOS/GDEF tables will be dropped in the output font).

dnfield · 2023-05-01T18:09:23Z

Our development workflows on Windows aren't great. I ran the format script for you locally. I'll try to figure out why the tests are complaining so much for me.

Also it seems like I may have made things unhappy in the git history, which I'll try to fix.

…t with variable font axes before additionally dropping the GSUB/GPOS/GDEF tables. These tables were being forced dropped in all cases (even though harfbuzz had been modified to always keep them). I made the change only drop the tables for variable fonts to preserve the previous behavior in the most possible cases. This PR fixes #125704. To see the bug (or verify it is fixed) in the live web examples below you must select the font variations Fill->1 and Weight->100. (See issue #125704 for more details). The issue #125704 includes examples of the font-subset being used (and breaking) the variable fonts, as well as example of the --no-tree-shake-icons being used where the fonts do not break. Additionally, I have created an additional live example where this PR has been applied to font-subset and icon tree shaking is still taking place. (Example w/ icon tree-shaking using the broken font-subset for icon tree shaking is found here ). In the example build output below note that the non-variable fonts "CupertinoIcons.ttf" and "MaterialIcons-Regular.otf" have the same size savings as before the change, but the variable fonts "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf", "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf", and "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" now have a much more reasonable saving of ~2% because every icon in the font is included in the example. The previous extra ~30% savings was from having the GSUB table removed. The 30% size savings for a tree-shaking for a case where every icon is used in the example probably should have been suspect.

timmaffett · 2023-05-01T19:38:46Z

Thanks for formatting - I just rebased to what you ended up with.
I have been procrastinating setting up my engine build environment on my mac, I need to. I definitely notice the windows shortfalls - sometimes I can use WSL to get around it, sometimes not.

dnfield · 2023-05-01T20:08:16Z

Tests build and run as expected on Linux (and apparently Windows), but not mac.

dnfield · 2023-05-01T20:34:55Z

Ok, it looks like they're passing on Intel Mac but maybe not ARM. Looking to see if CI is running arm somewhere...

dnfield · 2023-05-01T20:40:32Z

The 3variable.ttf seems to not have a GSUB table...

timmaffett · 2023-05-01T20:46:55Z

investigating...
it looks corrupted somehow from my original here.
I will re-upload

timmaffett · 2023-05-01T20:47:46Z

no - sorry mistake on my part - not corrupted.

dnfield · 2023-05-01T20:47:50Z

Argh. I think this is an error on my end.

dnfield · 2023-05-01T20:48:08Z

I was building the host_debug_unopt_arm64 variant and testing host_debug_unopt. Sorry about that.

dnfield

LGTM

timmaffett · 2023-05-01T20:52:28Z

ok great!
Thanks for all your work on this!

…iable fonts where they are needed Fixes flutter#125704 (flutter/engine#41592)

…125839) flutter/engine@58cc541...3fa6084 2023-05-01 timmaffett@gmail.com Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 (flutter/engine#41592) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard bot added the needs tests label Apr 28, 2023

This was referenced Apr 28, 2023

Support Material Symbols flutter/flutter#102560

Open

Icons don't fill right when weight is 100 timmaffett/material_symbols_icons#1

Closed

jonahwilliams reviewed Apr 29, 2023

View reviewed changes

jonahwilliams requested a review from dnfield April 29, 2023 16:40

dnfield force-pushed the font-subset_fix branch from 1a67963 to bac516a Compare May 1, 2023 18:16

dnfield approved these changes May 1, 2023

View reviewed changes

dnfield merged commit 3fa6084 into flutter:main May 1, 2023
28 checks passed

engine-flutter-autoroll mentioned this pull request May 1, 2023

Roll Flutter Engine from 58cc54197a54 to 3fa6084dfb36 (1 revision) flutter/flutter#125839

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2023

3fa6084df Fixes font-subset to not drop GSUB/GPOS/GDEF tables for var…

a0dc880

…iable fonts where they are needed Fixes flutter#125704 (flutter/engine#41592)

r-ilagan mentioned this pull request Jul 5, 2023

Symbols.text_selection_start icon disappearing in release build timmaffett/material_symbols_icons#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 #41592

Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 #41592

timmaffett commented Apr 28, 2023 •

edited

flutter-dashboard bot commented Apr 28, 2023

timmaffett commented Apr 28, 2023

jonahwilliams Apr 29, 2023

timmaffett commented Apr 29, 2023 •

edited

dnfield commented May 1, 2023

timmaffett commented May 1, 2023 •

edited

dnfield commented May 1, 2023

timmaffett commented May 1, 2023

dnfield commented May 1, 2023

dnfield commented May 1, 2023

dnfield commented May 1, 2023

timmaffett commented May 1, 2023

timmaffett commented May 1, 2023

dnfield commented May 1, 2023

dnfield commented May 1, 2023

dnfield left a comment

timmaffett commented May 1, 2023

	if (!hb_blob_get_length(font_blob.get())) {cd
	if (!hb_blob_get_length(font_blob.get())) {

Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 #41592

Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 #41592

Conversation

timmaffett commented Apr 28, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented Apr 28, 2023

timmaffett commented Apr 28, 2023

jonahwilliams Apr 29, 2023

Choose a reason for hiding this comment

timmaffett commented Apr 29, 2023 • edited

dnfield commented May 1, 2023

timmaffett commented May 1, 2023 • edited

dnfield commented May 1, 2023

timmaffett commented May 1, 2023

dnfield commented May 1, 2023

dnfield commented May 1, 2023

dnfield commented May 1, 2023

timmaffett commented May 1, 2023

timmaffett commented May 1, 2023

dnfield commented May 1, 2023

dnfield commented May 1, 2023

dnfield left a comment

Choose a reason for hiding this comment

timmaffett commented May 1, 2023

timmaffett commented Apr 28, 2023 •

edited

timmaffett commented Apr 29, 2023 •

edited

timmaffett commented May 1, 2023 •

edited