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

Refactor font_getsize and font_render #4910

Merged
merged 10 commits into from Sep 19, 2020
Merged

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Sep 9, 2020

First part of #4724, refactoring the main font functions.

Changes proposed in this pull request:

  • organize and add comments to variable declarations
  • rename variables and remove duplicates (e.g. x_position and y_max had almost the same meaning for horizontal and vertical text respectively)
  • include all glyphs and both coordinates into offset calculations, not just first and last; fixes issue Vertical text alignment ignores yOffset #4553 and similar, see test with multiple test cases (to be part of followup PR)
  • adjust coordinate rounding
    • use FT_GLYPH_BBOX_PIXELS instead of rounding up the difference returned for FT_GLYPH_BBOX_SUBPIXELS, this fixes some text clipping (e.g. Absent pixels when drawing font #4569 (comment)), see documentation
    • simplify the code by using the bbox for both vertical and horizontal bounds, instead of calculating one of the dimensions from the metrics
    • change PIXEL macro to round to nearest instead of higher, as it is no longer used to compute size, only positions; should help with kerning in basic layout in future PR;
      visually comparing the output gives an almost imperceptible spacing difference
    • use bitmap_top instead of horiBearingY when rendering, fixes issue Incorrect vertical alignment of character when using fontmode = '1' #2293

These changes require modifying the test_imagefontctl test images, but in most cases there is only a minor spacing change, a vertical or horizontal adjustment due to #4553 and related issues, or the original image even had clipped text (e.g. test_direction_ttb)


Text clipping comparisons

I added some test code to compare alignment in getsize and render. In case of mismatch a warning is generated in test branches. I tested both the standard test suite, as well as the following extra test cases:

Test cases

For each test case I give the generated images with basic layout first, raqm second.

  1. "gjpqy()|{[[}],;/_" using Helvetica LT Std Light.otf at size 48 from Fix rendered characters have been chipped for some TrueType fonts #45 discussion.
  2. "0123456789" using vista-hei-all.ttf at size 35 from Absent pixels when drawing font #4569.
  3. "01234567890123456789" using verdanab.ttf at size 13 with mode 1 from Incorrect vertical alignment of character when using fontmode = '1' #2293, see also Truetype fonts not being rendered correctly with the TEXT method #4177 (comment)

master

(click to expand)

It was a bit tricky to figure out which variables are best to compare, but I think I got the best possible match between getsize and render, see branch master...nulano:anchor-part1-clip-test

15 extra warnings in test suite (7 due to clipping, 8 due to different alignment).

  1. looks ok, no warnings

test_clipping py master test_helvetica-basic-

test_clipping py master test_helvetica-raqm-

  1. clipping, with warning

test_clipping py master test_hei_vista-basic-

c:\git\pillow\src\PIL\ImageFont.py:490: RuntimeWarning: Text is clipped!
    Measured: (0, 25, [(1, 0), (21, 0), (37, 0), (55, 0), (72, 0), (90, 1), (109, 0), (127, 1), (144, 0), (162, 0)])
    Rendered: (0, 24, [(1, -1), (21, -1), (37, -1), (55, -1), (72, -1), (90, 0), (109, -1), (127, 0), (144, -1), (162, -1)])
    Clipped glyphs: [(0, 'top', 1), (1, 'top', 1), (2, 'top', 1), (3, 'top', 1), (4, 'top', 1), (6, 'top', 1), (8, 'top', 1), (9, 'top', 1)]
  warnings.warn(m, RuntimeWarning)

test_clipping py master test_hei_vista-raqm-

c:\git\pillow\src\PIL\ImageFont.py:490: RuntimeWarning: Text is clipped!
    Measured: (0, 25, [(1, 0), (21, 0), (36, 0), (54, 0), (70, 0), (88, 1), (106, 0), (124, 1), (140, 0), (158, 0)])
    Rendered: (0, 24, [(1, -1), (21, -1), (36, -1), (54, -1), (70, -1), (88, 0), (106, -1), (124, 0), (140, -1), (158, -1)])
    Clipped glyphs: [(0, 'top', 1), (1, 'top', 1), (2, 'top', 1), (3, 'top', 1), (4, 'top', 1), (6, 'top', 1), (8, 'top', 1), (9, 'top', 1)]
  warnings.warn(m, RuntimeWarning)
  1. poor alignment, no warnings

test_clipping py master test_verdanab-basic-

test_clipping py master test_verdanab-raqm-


PR

(click to expand)

There were no problems writing debug code here, as both functions now have the same variables with the same names and meaning, see branch nulano/Pillow@anchor-part1...nulano:anchor4

no warnings in test suite

  1. looks ok, no warnings

test_clipping py test_helvetica-basic-

test_clipping py test_helvetica-raqm-

  1. looks ok, no warnings

test_clipping py test_hei_vista-basic-

test_clipping py test_hei_vista-raqm-

  1. looks ok, alignment warnings

test_clipping py test_verdanab-basic-

c:\git\pillow\src\PIL\ImageFont.py:498: RuntimeWarning: Text is misaligned!
    Measured: (0, 10, [(1, 0), (11, 1), (19, 1), (28, 1), (37, 1), (46, 1), (55, 1), (64, 1), (73, 1), (82, 1), (91, 0), (101, 1), (109, 1), (118, 1), (127, 1), (136, 1), (145, 1), (154, 1), (163, 1), (172, 1)])
    Rendered: (0, 9, [(1, 0), (11, 0), (19, 0), (28, 0), (37, 0), (46, 0), (55, 0), (64, 0), (73, 0), (82, 0), (91, 0), (101, 0), (109, 0), (118, 0), (127, 0), (136, 0), (145, 0), (154, 0), (163, 0), (172, 0)])
  warnings.warn(m, RuntimeWarning)

test_clipping py test_verdanab-raqm-

c:\git\pillow\src\PIL\ImageFont.py:498: RuntimeWarning: Text is misaligned!
    Measured: (0, 10, [(1, 0), (11, 1), (20, 1), (29, 1), (38, 1), (47, 1), (57, 1), (66, 1), (75, 1), (84, 1), (94, 0), (104, 1), (112, 1), (121, 1), (131, 1), (140, 1), (149, 1), (158, 1), (168, 1), (177, 1)])
    Rendered: (0, 9, [(1, 0), (11, 0), (20, 0), (29, 0), (38, 0), (47, 0), (57, 0), (66, 0), (75, 0), (84, 0), (94, 0), (104, 0), (112, 0), (121, 0), (131, 0), (140, 0), (149, 0), (158, 0), (168, 0), (177, 0)])
  warnings.warn(m, RuntimeWarning)

These warnings are not surprising due to the following from the FreeType tutorial II section 4b (emphasis mine):

In general, the above function does not compute an exact bounding box of a string! As soon as hinting is involved, glyph dimensions must be derived from the resulting outlines. For anti-aliased pixmaps, FT_Outline_Get_BBox then yields proper results. In case you need 1-bit monochrome bitmaps, it is even necessary to actually render the glyphs because the rules for the conversion from outline to bitmap can also be controlled by hinting instructions (cf. dropout control).

Fixing this would essentially require reverting #4652, so it is up for discussion. However, unlike master, the PR at least renders this case properly.

Regarding changing FT_Outline_Get_CBox with FT_Outline_Get_BBox, it is my understanding that the former always returns a larger or same bbox, so is likely unnecessary to change. From the documentation for FT_Outline_Get_CBox (emphasis mine):

Return an outline's ‘control box’. The control box encloses all the outline's points, including Bezier control points. Though it coincides with the exact bounding box for most glyphs, it can be slightly larger in some situations (like when rotating an outline that contains Bezier outside arcs).

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any obvious problems, further reviews also welcome :)

Comment on lines 262 to 267
# vertical offset is added for historical reasons, see discussion in #4789
size, offset = self.font.getsize(text, False, direction, features, language)
return (
size[0] + stroke_width * 2 + offset[0],
size[0] + stroke_width * 2,
size[1] + stroke_width * 2 + offset[1],
)
Copy link
Contributor Author

@nulano nulano Sep 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes an issue introduced in #2576 (Pillow 4.2.0) where the x offset was sometimes added (subtracted) twice in the C function, see #4789 (comment). This change adjusts the Python function to match this behaviour after fixing the C function.

The offset was first added in #784 to match pre-#45/#185 behaviour (and make getsize return the bottom right text coordinate, which used to match the size), but #2576 effectively reverted this change for the horizontal axis when the first character has a negative X bearing in horizontal text (otherwise the x offset is 0).

I think it is better to stick with the current behaviour (which returns the actual width but might be confusing if the first character has a negative bearing) and document it (in part 3 of #4724) rather than reverting back to the previous behaviour, as it has been this way for a long time now.

I have updated the comment to link to this explanation for future reference.

@hugovk hugovk merged commit 93d011e into python-pillow:master Sep 19, 2020
@hugovk
Copy link
Member

hugovk commented Sep 19, 2020

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants