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

Added TrueType default font to allow for different sizes #7354

Merged
merged 5 commits into from
Oct 14, 2023

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Aug 26, 2023

Resolves #6622

At the moment, Pillow's builtin font, ImageFont.load_default(), has a static size.

This PR checks if FreeType support is available, and if so, uses Aileron Regular instead, a CC0 font from https://dotcolon.net/font/aileron

out

Because it is CC0, we are free to change it, so I removed various characters using FontForge to reduce the file size, and converted it to TTF using https://convertio.co. This increases the size of ImageFont.py from 42kb in main to 60kb in this PR.

You might ask why I didn't convert it to WOFF or WOFF2. WOFF2 requires at least FreeType 2.10.2 which not all of our Docker jobs have, and WOFF doesn't load on some of our Docker jobs.

With a TrueType font, the size is able to be changed. In terms of our API, the following arguments are added.

ImageFont.load_default(size=24)
draw.text((0, 0), "test", font_size=24)
draw.textlength((0, 0), "test", font_size=24)
draw.textbbox((0, 0), "test", font_size=24)
draw.multiline_text((0, 0), "test", font_size=24)
draw.multiline_textbbox((0, 0), "test", font_size=24)

This PR designates all of the new ImageDraw arguments as keyword arguments.

@hugovk
Copy link
Member

hugovk commented Oct 13, 2023

Let's add this to release notes.

@hugovk hugovk merged commit 7bf1a87 into python-pillow:main Oct 14, 2023
53 of 54 checks passed
@radarhere radarhere deleted the load_default branch October 14, 2023 04:31
@pmeier
Copy link

pmeier commented Oct 17, 2023

Unfortunately, this breaks BC. We (torchvision) had some reference images with the old font and check some functionality that we have on top of Pillow against the reference. Since Pillow==10.1.0, these tests fail, since the font and ultimately the pixel values are different.

I really like this change, because the old default font was ugly AF (not complaining; atleast there was a default font). Would it be possible to expose the old font so? IIUC, it is currently only possible to load it with ImageFont.load_default() and only if the _imageft module is not available. Meaning, we would need to monkey patch the check to get the font out.

@radarhere
Copy link
Member Author

Hi. Given that you like the change, would it not be simpler to update the torchvision tests? Even if we provide a way to switch back to the old font, wouldn't you want to add torchvision tests for the new font as well?

I personally think of backwards compatibility in terms of errors and functionality, not individual pixels. As another example, in Pillow 9.0.0, we switched to libjpeg-turbo in our wheels, which lead to users reporting that JPEG pixel values were different - but I don't think that means the Pillow wheels were backwards incompatible.

Not trying to shift the problem onto you, just looking to understand the motivation here.

@pmeier
Copy link

pmeier commented Oct 17, 2023

Given that you like the change, would it not be simpler to update the torchvision tests?

The keyword in my statement is I. But I don't know if there is consensus for this amongst the other maintainers. I'll get back to you on that.

Even if we provide a way to switch back to the old font, wouldn't you want to add torchvision tests for the new font as well?

For the stuff that we are testing, the actual font is irrelevant. We just want to make sure that some bounding boxes drawn onto an image are labelled. Meaning, regardless with which font we ultimately end up with, we'll never have multiple reference images for different fonts.

I personally think of backwards compatibility in terms of errors and functionality, not individual pixels.

That is fair, although I would still classify a change in default value BC breaking. However, the case here is not just a change of default value, but the old value is no longer accessible. Meaning, it is impossible for the user to get the old behavior back.

I'm not familiar with Pillows BC policy, but since the versioning scheme is semver, I would only expect BC breaks on major versions. And even there I would expect a deprecation cycle where possible. Doesn't work for switching from libjpeg to libjpeg-turbo, but for the use case here that would have been possible.

@radarhere
Copy link
Member Author

I wouldn't expect serious reliance on our default font that only produces tiny pixellated text - but I suppose I should consider that I wouldn't have expected #6622 to receive the attention that it has either. I've created PR #7476 to add ImageFont.load_pilfont() to restore access to the old font.

@nulano
Copy link
Contributor

nulano commented Oct 19, 2023

FWIW different FreeType versions sometimes render fonts with minor differences, so you might still prefer a bitmap font. However, IIRC it was more of an issue with older versions and not that common anymore.

@radarhere
Copy link
Member Author

The torchvision concern was later discarded

we internally agreed that we can update our images and don't need this change. If no one else complains, there is no need to make this change for us.

@nulano
Copy link
Contributor

nulano commented Dec 27, 2023

@radarhere This change broke some of the tests for #7244.
I think #7476 should be reconsidered at least for those tests.

@radarhere
Copy link
Member Author

Wouldn't a simple fix be to just create an instance of the ImageFont class directly in the test? I've created #7647

@radarhere
Copy link
Member Author

#7647 has now been merged.

@thijstriemstra
Copy link
Contributor

The keyword in my statement is I. But I don't know if there is consensus for this amongst the other maintainers. I'll get back to you on that.

It also caused our tests to fail and decided to include and specifically load the old builtin font to restore failing tests, instead of having to redo each test for the new builtin font (rm-hull/luma.core#274)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why not ship with a default font able to change font size in ImageDraw.Draw.text?
5 participants