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

Variable Font Instantiation Limited #1981

Closed
pmjdebruijn opened this issue Oct 8, 2023 · 16 comments
Closed

Variable Font Instantiation Limited #1981

pmjdebruijn opened this issue Oct 8, 2023 · 16 comments
Labels
bug Existing features not working as expected

Comments

@pmjdebruijn
Copy link
Contributor

pmjdebruijn commented Oct 8, 2023

variable-font-bug.zip

It seems that WeasyPrint instantiates each font-weight when it first encounters it, even if later on, it encounters a different font-variation-settings: 'wght' applied to that font-weight. Seeming font-weight is used for "naming" the embedded font, so Roboto-Flex-Bold and font-variation-settings is used to actually determine the real visual weight (wght). However once real visual wght X is assigned/applied to "Bold" it remains fixed after that.

While the current variable font implementation is already quite useful in it's current form, as it allows users to finetune font weights, more creative uses of variable fonts quickly leads to very confusing situations.

If this is not so trivial to fix, maybe consider adding a CLI warning about this limiation?

@pmjdebruijn
Copy link
Contributor Author

pmjdebruijn commented Oct 8, 2023

Note, it seems that only "Regular" and "Bold" font-weights are used for naming instances, which means users are limited to two arbitrary real visual weights. Expanding this to include at least Light, Medium, SemiBold would already significantly improve things, although this won't be a "proper" fix.

That being said, given that fonts can have multiple axis of variation, you'll quickly end up having to do something like font-family + '-' + trunc(hmac-sha256(font-family, axis1 + axis2 + axis3 + axis4 + axis5 + etc), 10) I guess?

@pmjdebruijn
Copy link
Contributor Author

@pmjdebruijn
Copy link
Contributor Author

Presumably this is where the problem may reside:
https://github.com/Kozea/WeasyPrint/blob/main/weasyprint/pdf/stream.py#L326

@pmjdebruijn
Copy link
Contributor Author

@pmjdebruijn
Copy link
Contributor Author

Only a minor metadata cosmetic issue remains, all variations are either encoded as "Roboto-Flex" (implicitly regular), or "Roboto-Flex-Bold", presumably this is Pango misbehaving? As I'd expect wght 450-550 to be encoded as "Roboto-Flex-Medium"...

@pmjdebruijn
Copy link
Contributor Author

The following:

<p style="font-weight: 100; font-variation-settings: 'wght' 100; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 100)</p>
<p style="font-weight: 200; font-variation-settings: 'wght' 200; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 200)</p>
<p style="font-weight: 300; font-variation-settings: 'wght' 300; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 300)</p>
<p style="font-weight: 400; font-variation-settings: 'wght' 400; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 400)</p>
<p style="font-weight: 500; font-variation-settings: 'wght' 500; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 500)</p>
<p style="font-weight: 600; font-variation-settings: 'wght' 600; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 600)</p>
<p style="font-weight: 700; font-variation-settings: 'wght' 700; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 700)</p>
<p style="font-weight: 800; font-variation-settings: 'wght' 800; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 800)</p>
<p style="font-weight: 900; font-variation-settings: 'wght' 900; ">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi a tellus vel. (wght 900)</p>

Results in:

pdffonts variable-font-bug-2.pdf
name                                 type              encoding         emb sub uni object ID
------------------------------------ ----------------- ---------------- --- --- --- ---------
JSUIGM+Roboto-Flex                   CID TrueType      Identity-H       yes yes yes     19  0
RSCFBI+Roboto-Flex                   CID TrueType      Identity-H       yes yes yes     23  0
EIHVZQ+Roboto-Flex                   CID TrueType      Identity-H       yes yes yes     27  0
YFFEAX+Roboto-Flex                   CID TrueType      Identity-H       yes yes yes     31  0
NWDZUM+Roboto-Flex                   CID TrueType      Identity-H       yes yes yes     35  0
COIJIX+Roboto-Flex                   CID TrueType      Identity-H       yes yes yes     39  0
UPCFVR+Roboto-Flex-Bold              CID TrueType      Identity-H       yes yes yes     43  0
LRIFNX+Roboto-Flex-Bold              CID TrueType      Identity-H       yes yes yes     47  0
DSDORO+Roboto-Flex-Bold              CID TrueType      Identity-H       yes yes yes     51  0

So 700 and above is automatically getting the Bold Suffix.

While this is minor, it results in cases where the naming is very misleading where a -Bold could have a real wght of 100 :)

pmjdebruijn added a commit to pmjdebruijn/WeasyPrint that referenced this issue Oct 15, 2023
liZe added a commit that referenced this issue Oct 17, 2023
@liZe
Copy link
Member

liZe commented Oct 17, 2023

Fixed by #1988.

@liZe liZe closed this as completed Oct 17, 2023
@liZe liZe added the bug Existing features not working as expected label Oct 17, 2023
@pmjdebruijn
Copy link
Contributor Author

My pull request didn't fix the bug in it's entirety... There's still the interaction with font-weight: as illustrated in my last post, but I haven't been successful in figuring out where this issue might lie... Do you have any thoughts/hints on this matter?

@liZe
Copy link
Member

liZe commented Oct 17, 2023

There's still the interaction with font-weight: as illustrated in my last post

I’m not sure to understand your problem. Do you mean that the name of the font in the metadata is wrong?

@pmjdebruijn
Copy link
Contributor Author

pmjdebruijn commented Oct 18, 2023

Yeah sortof...

<p style="font-weight: normal; font-variation-settings: 'wght' 300; ">Lorem ipsum</p>
<p style="font-weight: bold; font-variation-settings: 'wght' 300; ">Lorem ipsum</p>

The above will result in two fonts (Roboto-Flex + Roboto-Flex-Bold) being embedded, even though they're the same font variation. And having something called 'Bold', while being 'Light' in reality is a bit misleading of course.

Practically speaking, this won't happen too often in real world use-cases, and when it does it just results in a larger PDF than optimal, so it's not the end of the world.

But if you can give me some pointers, I may look into this a little further...

@pmjdebruijn
Copy link
Contributor Author

This would have made some sense:

diff --git a/weasyprint/pdf/stream.py b/weasyprint/pdf/stream.py
index ce309ebf..9b6603d8 100644
--- a/weasyprint/pdf/stream.py
+++ b/weasyprint/pdf/stream.py
@@ -130,6 +130,18 @@ class Font:
                 variations = {
                     part.split('=')[0]: float(part.split('=')[1])
                     for part in ffi.string(variations).decode().split(',')}
+            if 'wght' in variations:
+                if variations['wght'] < 150: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_THIN)
+                if variations['wght'] >= 150 and variations['wght'] < 250: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_ULTRALIGHT)
+                if variations['wght'] >= 250 and variations['wght'] < 350: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_LIGHT)
+                if variations['wght'] >= 350 and variations['wght'] < 450: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_NORMAL)
+                if variations['wght'] >= 450 and variations['wght'] < 550: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_MEDIUM)
+                if variations['wght'] >= 550 and variations['wght'] < 650: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_SEMIBOLD)
+                if variations['wght'] >= 650 and variations['wght'] < 750: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_BOLD)
+                if variations['wght'] >= 750 and variations['wght'] < 850: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_ULTRABOLD)
+                if variations['wght'] >= 850 and variations['wght'] < 950: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_HEAVY)
+                if variations['wght'] >= 950: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_ULTRAHEAVY)
+                print(ffi.string(pango.pango_font_description_to_string(self.description)))
             if 'wght' not in variations:
                 variations['wght'] = pango.pango_font_description_get_weight(
                     self.description)

However, it seems to have no effects on the generated PDF, neither does:

diff --git a/weasyprint/pdf/stream.py b/weasyprint/pdf/stream.py
index ce309ebf..aaa1c92a 100644
--- a/weasyprint/pdf/stream.py
+++ b/weasyprint/pdf/stream.py
@@ -130,6 +130,10 @@ class Font:
                 variations = {
                     part.split('=')[0]: float(part.split('=')[1])
                     for part in ffi.string(variations).decode().split(',')}
+            if 'wght' in variations:
+                # leaving weight as-is can cause doubly embedded fonts
+                pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_NORMAL)
+                print(ffi.string(pango.pango_font_description_to_string(self.description)))
             if 'wght' not in variations:
                 variations['wght'] = pango.pango_font_description_get_weight(
                     self.description)

@pmjdebruijn
Copy link
Contributor Author

So this fixes the issue:

diff --git a/weasyprint/pdf/stream.py b/weasyprint/pdf/stream.py
index ce309ebf..69296b22 100644
--- a/weasyprint/pdf/stream.py
+++ b/weasyprint/pdf/stream.py
@@ -34,6 +34,29 @@ class Font:
         self.style = pango.pango_font_description_get_style(description)
         self.family = ffi.string(
             pango.pango_font_description_get_family(description))
+
+        #if 'fvar' in self.ttfont:
+        variations = pango.pango_font_description_get_variations(
+            self.description)
+        if variations == ffi.NULL:
+            variations = {}
+        else:
+            variations = {
+                part.split('=')[0]: float(part.split('=')[1])
+                for part in ffi.string(variations).decode().split(',')}
+        if 'wght' in variations:
+            if variations['wght'] < 150: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_THIN)
+            if variations['wght'] >= 150 and variations['wght'] < 250: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_ULTRALIGHT)
+            if variations['wght'] >= 250 and variations['wght'] < 350: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_LIGHT)
+            if variations['wght'] >= 350 and variations['wght'] < 450: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_NORMAL)
+            if variations['wght'] >= 450 and variations['wght'] < 550: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_MEDIUM)
+            if variations['wght'] >= 550 and variations['wght'] < 650: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_SEMIBOLD)
+            if variations['wght'] >= 650 and variations['wght'] < 750: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_BOLD)
+            if variations['wght'] >= 750 and variations['wght'] < 850: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_ULTRABOLD)
+            if variations['wght'] >= 850 and variations['wght'] < 950: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_HEAVY)
+            if variations['wght'] >= 950: pango.pango_font_description_set_weight(self.description, pango.PANGO_WEIGHT_ULTRAHEAVY)
+        print(ffi.string(pango.pango_font_description_to_string(self.description)))
+
         description_string = ffi.string(
             pango.pango_font_description_to_string(description))
         # Never use the built-in hash function here: it’s not stable

Though this patch is probably not entirely proper, as some code is now duplicated, so some refactoring might be worth while.

I'd love to hear your thoughts.

@liZe
Copy link
Member

liZe commented Oct 18, 2023

I'd love to hear your thoughts.

To be honest, I don’t want to add extra code for this, as we could introduce other bugs that will be hard to debug later. If I can find some time, I’ll create a variable testing font and add some tests, we need to avoid regressions more that to have better font names in metadata 😀.

pmjdebruijn added a commit to pmjdebruijn/WeasyPrint that referenced this issue Oct 18, 2023
(1) Prevents identical wght variations from being doubly embedded
    under different named weights (typically normal and bold)

(2) Assign good approximate named weights, which prevents
    metadata from being actively misleading.
@pmjdebruijn
Copy link
Contributor Author

As I mentioned earlier it also causes identical fonts to be doubly embedded, thus easily avoidably enlarging some PDFs.

Given I was almost done when you responded, I just posted a pull request, which has my earlier suggestion refactored to the best of my (somewhat limited) ability.

I really hope you would reconsider this, I understand the need to prevent regressions, but Weasy's current behavior is actively misleading, and as far as I can tell risk of regression should not be excessive here.

Also you've done beta releases for weasy in the past, so why not considering doing that in this case as well?

@pmjdebruijn
Copy link
Contributor Author

pmjdebruijn commented Oct 18, 2023

Oh and I forgot to mention, Weasy's variable font support is now still young, thus it's use in the real world is probably quite limited at this point in time. This will only increase over time. Therefore if this has to be fixed in the deeper future, (for whatever reason) the pain of any potential regression will only be that much bigger.

pmjdebruijn added a commit to pmjdebruijn/WeasyPrint that referenced this issue Oct 19, 2023
(1) Prevents identical wght variations from being doubly embedded
    under different named weights (typically normal and bold)

(2) Assign good approximate named weights, which prevents
    metadata from being actively misleading.
@pmjdebruijn
Copy link
Contributor Author

I just revised for PEP8 compliance as far as it's possible, as the pango API makes complying with the 79 char line length impossible without violating other rules.

However PEP8, allows for 99 char lines in some cases, and my commit does comply with that:
https://hg.python.org/peps/rev/fb24c80e9afb#l1.99

pmjdebruijn added a commit to pmjdebruijn/WeasyPrint that referenced this issue Oct 20, 2023
Adds italic naming to font metadata where appropriate
pmjdebruijn added a commit to pmjdebruijn/WeasyPrint that referenced this issue Oct 20, 2023
Adds stretch naming to font metadata where appropriate
liZe pushed a commit to pmjdebruijn/WeasyPrint that referenced this issue Dec 5, 2023
(1) Prevents identical wght variations from being doubly embedded
    under different named weights (typically normal and bold)

(2) Assign good approximate named weights, which prevents
    metadata from being actively misleading.
liZe pushed a commit to pmjdebruijn/WeasyPrint that referenced this issue Dec 5, 2023
Adds italic naming to font metadata where appropriate
liZe pushed a commit to pmjdebruijn/WeasyPrint that referenced this issue Dec 5, 2023
Adds stretch naming to font metadata where appropriate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing features not working as expected
Projects
None yet
Development

No branches or pull requests

2 participants