Bug 219608 - print/freetype2 produces incorrect line spacing for some fonts
Summary: print/freetype2 produces incorrect line spacing for some fonts
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Kurt Jaeger
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2017-05-28 05:17 UTC by rkoberman
Modified: 2017-10-26 06:21 UTC (History)
4 users (show)

See Also:
pi: maintainer-feedback-
koobs: merge-quarterly?


Attachments
mate-terminal 24x80 with Bitstream Vera Sans Mono font w/ Freetype2-2.7.1 (13.11 KB, image/png)
2017-05-28 05:17 UTC, rkoberman
no flags Details
mate-terminal 24x80 with Bitstream Vera Sans Mono font w/ Freetype2-2.8 (16.32 KB, image/png)
2017-05-28 05:19 UTC, rkoberman
no flags Details
Some files/patch-src_truetype_ttdriver.c patch for test (for v2.8) (585 bytes, patch)
2017-06-04 23:23 UTC, lightside
no flags Details | Diff
Patch for print/freetype2 (since 441132) (2.70 KB, patch)
2017-06-05 00:41 UTC, lightside
no flags Details | Diff
Some files/patch-src_truetype_ttdriver.c patch for test (for v2.8) (608 bytes, patch)
2017-06-05 11:07 UTC, lightside
no flags Details | Diff
Patch for print/freetype2 (since 441132) (2.72 KB, patch)
2017-06-05 11:09 UTC, lightside
no flags Details | Diff
Screenshots for mate-terminal (Bitstream Vera Sans Mono Roman, 13) (235.45 KB, application/x-bzip2)
2017-06-06 01:33 UTC, lightside
no flags Details
Patch for print/freetype2 (since 441132) (2.91 KB, patch)
2017-06-06 02:03 UTC, lightside
no flags Details | Diff
Some archived SciTE screenshots (703.55 KB, application/x-bzip2)
2017-06-06 04:51 UTC, lightside
no flags Details
Screenshots for mate-terminal (DejaVu Sans Mono Book, 14) (261.41 KB, application/x-bzip2)
2017-06-08 11:31 UTC, lightside
no flags Details
Some files/patch-fix_size_metrics.diff patch for test (for v2.8) (2.00 KB, patch)
2017-07-01 07:43 UTC, lightside
no flags Details | Diff
Some files/patch-fix_size_metrics.diff patch for test (for v2.8) (2.01 KB, patch)
2017-07-01 07:47 UTC, lightside
no flags Details | Diff
Proposed patch for print/freetype2 (since 441132) (4.35 KB, patch)
2017-07-01 07:49 UTC, lightside
no flags Details | Diff
Testing environment for freetype2, based on git bisect (3.99 KB, application/x-bzip2)
2017-07-01 08:05 UTC, lightside
no flags Details
Some test results (need statically built font_info to reproduce) (9.88 KB, application/x-bzip2)
2017-07-01 08:58 UTC, lightside
no flags Details
Some files/patch-fix_size_metrics.diff patch for test (for v2.8) (2.07 KB, patch)
2017-07-01 23:18 UTC, lightside
no flags Details | Diff
Proposed patch for print/freetype2 (since 441132) (4.41 KB, patch)
2017-07-01 23:19 UTC, lightside
no flags Details | Diff
Some files/patch-fix_size_metrics.diff patch for test (for v2.8) (2.01 KB, patch)
2017-07-02 01:05 UTC, lightside
no flags Details | Diff
Proposed patch for print/freetype2 (since 441132) (4.35 KB, patch)
2017-07-02 01:05 UTC, lightside
no flags Details | Diff
Proposed patch for print/freetype2 (4.34 KB, patch)
2017-07-03 07:45 UTC, lightside
lightside: maintainer-approval? (gnome)
koobs: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rkoberman 2017-05-28 05:17:17 UTC
Created attachment 182981 [details]
mate-terminal 24x80 with Bitstream Vera Sans Mono font w/ Freetype2-2.7.1

After updating to print/freetypw2-2.8.0 using Bitstream Vera Sans Mono the vertical pitch increases and significantly add of the size of the window. See attached images.

Downgraded to 2.7.1 and the correct vertical pitch was restored. Switching to TT metrics or other options did not fix the problem.
Comment 1 rkoberman 2017-05-28 05:19:08 UTC
Created attachment 182982 [details]
mate-terminal 24x80 with Bitstream Vera Sans Mono font w/ Freetype2-2.8
Comment 2 lightside 2017-06-04 23:18:34 UTC
(In reply to comment #0)
I can confirm that line spacing is different for x11/mate-terminal (v1.12.1) after print/freetype2 (v2.8) update.

The git bisect showed following commit:
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=bcc74f4dafee25ea89f1d3144646cba7e30f9908
-8<--
# bad: [a12a34451a99cbbcad55d466940fd445171927fd] * Version 2.8 released. =======================
# good: [069083cccd73d1d68da68116c8d050bb62cdfe0e] * Version 2.7.1 released. =========================
git bisect start 'VER-2-8' 'VER-2-7-1' '--'
# good: [c1b000da00c747cb46fd4c95ca83bee802d35487] [sfnt] Remove redundant code.
git bisect good c1b000da00c747cb46fd4c95ca83bee802d35487
# bad: [837f1125668805f47d810aec5a2c02e88453b557] [autofit] Add support for Tifinagh script.
git bisect bad 837f1125668805f47d810aec5a2c02e88453b557
# good: [13fa85a246d5fddc037ef3f11e61f9cf0ff3b2be] [truetype] Another limitation for bytecode loop count maximum.
git bisect good 13fa85a246d5fddc037ef3f11e61f9cf0ff3b2be
# good: [4c1b5dc152d2bd8e63431c9e708e3b851835d8d6] s/index/idx/ where appropriate.
git bisect good 4c1b5dc152d2bd8e63431c9e708e3b851835d8d6
# bad: [54b58097ee7dd8bf8db5dc0c925ad220e5770f8c] [autofit] Disable stem adjustment for `FT_LOAD_TARGET_LCD'.
git bisect bad 54b58097ee7dd8bf8db5dc0c925ad220e5770f8c
# bad: [da38be831d2c8ea5443c73d01ecfbc750bba7045] [truetype] Fix HVAR and VVAR handling (#50678).
git bisect bad da38be831d2c8ea5443c73d01ecfbc750bba7045
# bad: [bcc74f4dafee25ea89f1d3144646cba7e30f9908] [truetype] Allow linear scaling for unhinted rendering (#50470).
git bisect bad bcc74f4dafee25ea89f1d3144646cba7e30f9908
# good: [1ede3674cbb61888cccaf30a83536654b1c9d4e8] [truetype] Fix thinko related to PS name of default named instance.
git bisect good 1ede3674cbb61888cccaf30a83536654b1c9d4e8
# first bad commit: [bcc74f4dafee25ea89f1d3144646cba7e30f9908] [truetype] Allow linear scaling for unhinted rendering (#50470).
-->8-

which leads to related (currently closed) PR - "#50470: No means to create unhinted size.":
https://savannah.nongnu.org/bugs/?50470

Some other links:
"[ft-devel] truetype metrics resize request question":
http://lists.nongnu.org/archive/html/freetype-devel/2011-07/msg00022.html
"[truetype] Fix metrics on size request for scalable fonts."
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=b0962ac34e66052ccfee7996e5468f30d4bd5a72
"[ft-devel] New `slight' auto-hinting mode":
http://lists.nongnu.org/archive/html/freetype-devel/2017-04/msg00042.html

"FreeType bug causes Chromium misrendering of PDF":
https://bugzilla.redhat.com/show_bug.cgi?id=1437999
where some people reported about issue for gnome-terminal (and other applications):
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c24
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c29
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c30
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c34

"#738781 - [Accessibility] Can one modify line spacing in gnome-terminal":
https://bugzilla.gnome.org/show_bug.cgi?id=738781
https://bugzilla.gnome.org/show_bug.cgi?id=738781#c3

"#781479 - vte linespacing changes with freetype 2.7.2":
https://bugzilla.gnome.org/show_bug.cgi?id=781479
https://bugzilla.gnome.org/show_bug.cgi?id=781479#c3
https://bugzilla.gnome.org/show_bug.cgi?id=781479#c15
Comment 3 lightside 2017-06-04 23:23:00 UTC
Created attachment 183227 [details]
Some files/patch-src_truetype_ttdriver.c patch for test (for v2.8)

I attached some patch for test, which uses previous approach as in following commit:
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=b0962ac34e66052ccfee7996e5468f30d4bd5a72
This reverts changes for src/truetype/ttdriver.c file in following commit:
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=bcc74f4dafee25ea89f1d3144646cba7e30f9908

with some changes for 2.8 version to fix following error:
-8<--
In file included from <..>/freetype2/work/freetype-2.8/src/truetype/truetype.c:22:
<..>/freetype2/work/freetype-2.8/src/truetype/ttdriver.c:360:28: error: 
      assigning to 'FT_Size_Metrics' (aka 'struct FT_Size_Metrics_') from incompatible type 'FT_Size_Metrics *'
      (aka 'struct FT_Size_Metrics_ *'); dereference with *
      ttsize->root.metrics = ttsize->metrics;
                           ^ ~~~~~~~~~~~~~~~
                             *
1 error generated.
-->8-
e.g. "ttsize->root.metrics = *ttsize->metrics;".
For example, copy patch to print/freetype2/files directory and rebuild port for test.

Not sure if this is right approach, but possible to make it optional (for EXTRA_PATCHES) as an alternative to AF_CONFIG_OPTION_TT_SIZE_METRICS define (TT_SIZE_METRICS port's option):
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/autofit/afloader.c?id=VER-2-8#n255
before possible fix available in new version (and/or some changes for affected applications).
Comment 4 lightside 2017-06-05 00:41:47 UTC
Created attachment 183228 [details]
Patch for print/freetype2 (since 441132)

(In reply to comment #3)
Added possible patch for print/freetype2 port with following (additional) changes:
- Bump PORTREVISION
- Fix description for TT_SIZE_METRICS option
- Add TT_SIZE_METRICS_ALT experimental option
- Add LONG_PCF_NAMES option to enable long PCF family names (PCF_CONFIG_OPTION_LONG_FAMILY_NAMES define was introduced in 2.8 version) [1]

Reference links:
1. http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/docs/CHANGES?h=VER-2-8#n15

Possible to rename new options or clarify descriptions, if needed.
Comment 5 rkoberman 2017-06-05 01:00:10 UTC
This one-line patch does the trick! Normal spacing is back with 2.8.

I lack any familiarity with FreeType, so I really don't understand the discussion in the bug, but I'm curious whether this moves FreeType back to where some fonts will produce the uneven results cited in the bug report.
Comment 6 lightside 2017-06-05 01:18:50 UTC
Looks like, something wrong with patch in comment #3 (and comment #4, which includes it).
The mpv (multimedia/mpv) crashes with segmentation fault (core dumped), in my case.

(In reply to comment #5)
> This one-line patch does the trick! Normal spacing is back with 2.8.
Thanks for testing. Yes, the x11/mate-terminal worked with it, as well as editors/scite, x11-fm/caja, www/firefox, etc. But not multimedia/mpv somehow. So, probably I need to obsolete patch in comment #4.
Comment 7 rkoberman 2017-06-05 03:07:13 UTC
(In reply to lightside from comment #6)
In what way did it show up in mpv? While I use mpv a lot, about the only text I see is in controller at the bottom of the screen. Is that whet you refer to?

I can't test as today's mpv update broke it. (I'll look at that shortly.)
Comment 8 rkoberman 2017-06-05 03:52:23 UTC
Feeling really dumb,now. With the patch to freetype2, mpv segfaults. With patch removed, it runs fine. Back to 2.7.1.
Comment 9 lightside 2017-06-05 11:07:52 UTC
Created attachment 183237 [details]
Some files/patch-src_truetype_ttdriver.c patch for test (for v2.8)

Attached new patch for test.

Added checking for error value before access to ttsize->metrics. This fixes segmentation fault for mpv.
Comment 10 lightside 2017-06-05 11:09:48 UTC
Created attachment 183238 [details]
Patch for print/freetype2 (since 441132)
Comment 11 rkoberman 2017-06-05 17:41:53 UTC
I tries the two line patch, but the excess spacing problem returned. Looks like !error test fails and the latest patch is an effective noop.
Comment 12 lightside 2017-06-05 18:25:05 UTC
(In reply to comment #11)
> I tries the two line patch, but the excess spacing problem returned.
> Looks like !error test fails and the latest patch is an effective noop.
Strange, because it works in my case.

I may recommend to close all mate-terminal instances and open it again for test. Possible, that previous libfreetype2.so library used from memory. Compare with some screenshot from 2.7.1 version for the same mate-terminal profile, for example.

As about the "if ( !error )" check, it used before access to ttsize->metrics->x_ppem in 2.8 version, for example:
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/truetype/ttdriver.c?h=VER-2-8#n357
-8<--
    if ( FT_IS_SCALABLE( size->face ) )
    {
      error = tt_size_reset( ttsize, 0 );

#ifdef TT_USE_BYTECODE_INTERPRETER
      /* for the `MPS' bytecode instruction we need the point size */
      if ( !error )
      {
        FT_UInt  resolution =
                   ttsize->metrics->x_ppem > ttsize->metrics->y_ppem
                     ? req->horiResolution
                     : req->vertResolution;
-->8-

The 2.7.1 doesn't have such check:
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/truetype/ttdriver.c?h=VER-2-7-1#n355
-8<--
    if ( FT_IS_SCALABLE( size->face ) )
    {
      error = tt_size_reset( ttsize );
      ttsize->root.metrics = ttsize->metrics;

#ifdef TT_USE_BYTECODE_INTERPRETER
      /* for the `MPS' bytecode instruction we need the point size */
      {
        FT_UInt  resolution = ttsize->metrics.x_ppem > ttsize->metrics.y_ppem
                                ? req->horiResolution
                                : req->vertResolution;
-->8-

The alternative method is to check for "if ( ttsize->metrics )", which also worked.
Comment 13 rkoberman 2017-06-05 19:34:08 UTC
(In reply to lightside from comment #12)
I had rebooted, so no old .so to worry about, but I did not change my configuration to enable TT_SIZE_METRICS_ALL in the configuration. Your reply comment made me realize that I should do this. Now it is working again.

Thanks for working on this. If you have no objections I'll wait a few days to confirm that thee are no other unexpected issues a then ask for maintainer approval and commit.

I would like something to clarify the meaning of the TT_SIZE_METRICS_ALL option, though. I don't think much of anyone will realize what it means (not that I have a better idea. I'd really like "Restore traditional TrueType line spacing", but that is not quite correct.
Comment 14 Koop Mast freebsd_committer freebsd_triage 2017-06-05 21:26:28 UTC
Thanks both of you for working on this issue. I will give it a go soon as well. 

lightside can you report this problem upstream? We are probably not the only onces having this issue and you clearly know how to explain the issue the best.
Comment 15 rkoberman 2017-06-05 23:25:37 UTC
(In reply to rkoberman from comment #13)
Sigh! My screen still looked odd, so I got actual pixel measurements:
Ver.    Height (inc. frame and title bar)
2.7.1   290
2.8     343
2.8.1   313

So, while the current setup is closer to what it once was, it's still noticeably more widely spaced than 2.7.1.
Comment 16 rkoberman 2017-06-05 23:37:24 UTC
(In reply to rkoberman from comment #15)
N.B. The frames were measured via the cursor and may be off by a pixel or two. All windows are 24 lines long.
Comment 17 lightside 2017-06-06 01:33:24 UTC
Created attachment 183250 [details]
Screenshots for mate-terminal (Bitstream Vera Sans Mono Roman, 13)

(In reply to comment #13)
> So, while the current setup is closer to what it once was, it's still
> noticeably more widely spaced than 2.7.1.
I don't know, my results are different. It looks the same in my case, compared to 2.7.1 version.
I enabled following options (others are off) for attachment #183238 [details]:
DOCS, LCD_FILTERING, LONG_PCF_NAMES, PNG, TT_SIZE_METRICS_ALT.
and for 2.7.1 version:
DOCS, LCD_FILTERING, PNG.
Some configuration for other settings was explained in bug 219271 comment #9.

I also attached some screenshots (archived; in separate directories), which created for freetype2 2.7.1 and 2.8_1 versions, with following output (in sh shell):
man_date.png:
$ man date

xrdb_pkg_info.png:
$ xrdb -query
$ pkg info freetype2 | grep -e Name -e Version -e ": on" -e ": off"

Profile settings for mate-terminal:
Font: Bitstream Vera Sans Mono Roman, 13.
Size: 80x24 (default).
Comment 18 lightside 2017-06-06 02:03:39 UTC
Created attachment 183251 [details]
Patch for print/freetype2 (since 441132)

> I would like something to clarify the meaning of the TT_SIZE_METRICS_ALL
> option, though. I don't think much of anyone will realize what it means
> (not that I have a better idea. I'd really like "Restore traditional TrueType
> line spacing", but that is not quite correct.
It was "TT_SIZE_METRICS_ALT", but showed in terminal as "TT_SIZE_METRICS_AL". The _ALT postfix used as for "alternative" meaning.

I decided to rename TT_SIZE_METRICS_ALT option to FIX_SIZE_METRICS with following description: "Fix metrics on size request for scalable fonts (alternative method)". The main part of description is used from following commit:
"[truetype] Fix metrics on size request for scalable fonts."
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=b0962ac34e66052ccfee7996e5468f30d4bd5a72

Also created OPTIONS_RADIO (0 or 1 among N) for FIX_SIZE_METRICS and TT_SIZE_METRICS options, because they are alternative methods, in my opinion, but implemented in different places (TrueType driver (src/truetype/ttdriver.c) or auto-hinter/auto-fitter code (src/autofit/afloader.c)).
Comment 19 lightside 2017-06-06 02:50:10 UTC
(In reply to comment #13)
> Thanks for working on this. If you have no objections I'll wait a few days to
> confirm that thee are no other unexpected issues a then ask for maintainer
> approval and commit.
No problem, but I don't know about possible regressions between 2.7.1 and 2.8 versions, which possibly caused some differences in your case. I just proposed what worked in my case (and settings).

(In reply to comment #14)
> Thanks both of you for working on this issue. I will give it a go soon as well.
Thanks for your attention.

(In reply to comment #14)
> can you report this problem upstream? We are probably not the only onces
> having this issue and you clearly know how to explain the issue the best.
I shared some links in comment #2, which shows that this issue is known to upstream developers:
https://savannah.nongnu.org/bugs/?50470
https://savannah.nongnu.org/bugs/?50470#comment17
-8<--
We are back to square one. The rationale (http://lists.nongnu.org/archive/html/freetype-devel/2011-07/msg00022.html) for the original fix in 2011 was to fix ascend, descend, and line spacing values and it worked for any font.
-->8-

https://savannah.nongnu.org/bugs/?50470#comment26
-8<--
Here you can find an extensive explanation of the problem.

http://lists.nongnu.org/archive/html/freetype-devel/2017-04/msg00042.html
-->8-
But that PR was closed.

I don't object if someone shares a link to this PR and/or patch in attachment #183237 [details]. But I guess, that this method is already known (converted for 2.8 version), but not used for some reason. Otherwise, it was possible to create some define to enable this in the same place (src/truetype/ttdriver.c). Possible, that AF_CONFIG_OPTION_TT_SIZE_METRICS method wasn't complete for 2.8 version (related to line spacing).

In turn, this possibly caused other actions:
Gnome's vte developers may consider to fix this (probably gnome-terminal related; not sure about mate-terminal):
https://bugzilla.gnome.org/show_bug.cgi?id=781479#c15
-8<--
Egmont Koblinger 2017-04-19 18:01:08 UTC

In the mean time, we could consider adding an API and a hidden g-t setting to add/subtract a constant pixel value from the cell size computed by pango. That'd address bug 738781 as well.
-->8- 

Chromium related:
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c66
-8<--
Marek Kasík 2017-05-22 05:48:44 EDT
<..>
Regarding the Chromium case, it seems that people around Chromium are discussing about bundling of their own freetype which could solve the issue even in F24, F25 and F26 depending on whether and when they will do that (https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-packagers/xhmgu-1Vcok).
-->8- 

So, what already proposed in this PR could be just a intermediate solution.
Comment 20 rkoberman 2017-06-06 03:30:54 UTC
(In reply to lightside from comment #17)
Just rebuilt with the exact options you listed and used xwininfo to get exact height and it is 289 pixels for a 24 line display. I am don't know whether the options change fixed something (I only changed LONG_PCF_NAMES and PNG and I don't see any way either could be responsible.

In any case, it is working fine, so far.

Re: Comment #18:
Yes, they really should be radio buttons. I should have noticed that.

Again, thanks!
Comment 21 lightside 2017-06-06 04:51:06 UTC
Created attachment 183254 [details]
Some archived SciTE screenshots

(In reply to comment #13)
> So, while the current setup is closer to what it once was, it's still
> noticeably more widely spaced than 2.7.1.
(In reply to comment #17)
> I don't know, my results are different. It looks the same in my case, compared to 2.7.1 version.
I tested patch from attachment #183251 [details] for SciTE application (editors/scite). Attached some archived screenshots.
Looks like, there are differences between 2.7.1 (2.7.1_dpi_96.png) and 2.8_1 (2.8_1_dpi_96.png) versions, if Xft.dpi (96.0 for test) used in ~/.Xresources file, but other (94) for monitor, e.g. in ~/.xinitrc file: `xrandr --dpi 94`. I also tried to test this in bug 219271 comment #9.

So, the proposed patch, probably, is not a correct/complete fix for other applications with different fonts and settings (especially, if comparing to 2.7.1 version results).

(In reply to comment #13)
> Again, thanks!
Thank you too. Sorry, if something wrong still. I just tried to adapt previous methods, while other parts of code already changed. Probably, for correct fix there is a need to compare whole properties for fonts somehow (e.g. debugging, use print/ft2demos programs, etc.).
Comment 22 lightside 2017-06-08 11:31:45 UTC
Created attachment 183327 [details]
Screenshots for mate-terminal (DejaVu Sans Mono Book, 14)

I found what font was used in "About SciTE" dialog (editors/scite), which showed some issues (in comment #21). It was "DejaVu Serif Book, 14".
This was found in scite/src/Credits.cxx file for following function:
void SciTEBase::SetAboutMessage(GUI::ScintillaWindow &wsci, const char *appTitle)
and related code:
wsci.CallString(SCI_STYLESETFONT, STYLE_DEFAULT, "Serif");
int fontSize = 14;

where
-8<--
% fc-match Serif
DejaVuSerif.ttf: "DejaVu Serif" "Book"
-->8-

The DejaVuSerif.ttf was installed by x11-fonts/dejavu port and x11-fonts/xorg-fonts-truetype port depends from it.
-8<--
% pkg which -o /usr/local/share/fonts/dejavu/DejaVuSerif.ttf
/usr/local/share/fonts/dejavu/DejaVuSerif.ttf was installed by package x11-fonts/dejavu
-->8-

Possible to test "DejaVu Sans Mono Book, 14" for mate-terminal, installed by the same port:
-8<--
% pkg which -o /usr/local/share/fonts/dejavu/DejaVuSansMono.ttf
/usr/local/share/fonts/dejavu/DejaVuSansMono.ttf was installed by package x11-fonts/dejavu
-->8-

I attached archive with new screenshots, which shows the difference: decreased line spacing (compared to 2.7.1 version), in my case. This may mean, that there are other differences between 2.7.1 and 2.8 version, based on used fonts (i.e., the patch in attachment #183237 [details] doesn't work for all cases, if the same result needed, as in 2.7.1 version).

As about other testing methods, I created something like follows, based on pango-view usage (x11-toolkits/pango), but not sure, if this is correct method:
pango-font.sh:
-8<--
#!/bin/sh

font="Monospace Sans Serif"
dpi="72 96"
size="11 12 13 14"
backend="cairo ft2"
text="The quick brown fox jumps over the lazy dog"
texts=`printf "${text}\n${text}\n${text}"`

if [ -n "$1" ]; then
	if ! [ -d "$1" ]; then
		mkdir -p "$1"
	fi
	echo Output directory: $1
	cd "$1"
fi

for f in ${font}; do
	for d in ${dpi}; do
		for s in ${size}; do
			for b in ${backend}; do
	pango-view -q --backend="${b}" --dpi="${d}" --font="${f} ${s}" -t "${texts}" -o "${f}-${b}-${s}-${d}dpi.png"
			done
		done
	done
done

sha256 *.png > checksums.txt
-->8-

pango-waterfall.sh
-8<--
#!/bin/sh

font="Monospace Sans Serif"
dpi="72 96"
backend="cairo ft2"
text="The quick brown fox jumps over the lazy dog"

if [ -n "$1" ]; then
	if ! [ -d "$1" ]; then
		mkdir -p "$1"
	fi
	echo Output directory: $1
	cd "$1"
fi

for f in ${font}; do
	for d in ${dpi}; do
		for b in ${backend}; do
	pango-view -q --backend="${b}" --waterfall --dpi="${d}" --font="${f}" -t "${text}" -o "${f}-${b}-${d}dpi.png"
		done
	done
done

sha256 *.png > checksums.txt
-->8-

Then possible to use such scripts to check visual differences and/or checksums (but they maybe different between runs, while visually almost the same, somehow).
Comment 23 lightside 2017-06-08 11:34:27 UTC
I also found some information about freetype related mailing lists:
https://savannah.nongnu.org/mail/?group=freetype

There exists some related topics:
"[ft] cell size changed for terminal font":
http://lists.nongnu.org/archive/html/freetype/2017-06/msg00006.html
"[ft] With Freetype 2.8 my desktop looks bad"
http://lists.nongnu.org/archive/html/freetype/2017-05/msg00010.html

Looks like, if I'm not wrong, the suggested solution is to use fractional value for font size. There was some example to use 9.75 pt instead of 10 pt here: http://lists.nongnu.org/archive/html/freetype/2017-05/msg00011.html

Not sure, if such information is useful for this PR, but it maybe related.
Comment 24 lightside 2017-07-01 07:43:25 UTC
Created attachment 183976 [details]
Some files/patch-fix_size_metrics.diff patch for test (for v2.8)

Sorry for the delay. Some time was needed to create automated testing environment for using with "git bisect run".

The new git bisect showed following commit (based on some output from DejaVu Sans Mono Book 14 font):
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=8ab08cff63eeb23b6c9f2c4470ae9809f2acf244
-8<--
# bad: [a12a34451a99cbbcad55d466940fd445171927fd] * Version 2.8 released. =======================
# good: [069083cccd73d1d68da68116c8d050bb62cdfe0e] * Version 2.7.1 released. =========================
git bisect start 'VER-2-8' 'VER-2-7-1' '--'
# bad: [c1b000da00c747cb46fd4c95ca83bee802d35487] [sfnt] Remove redundant code.
git bisect bad c1b000da00c747cb46fd4c95ca83bee802d35487
# bad: [f4e569664332b30eca48643ed5194d0da91b0560] [sfnt] s/TT_NameEntry/TT_Name/.
git bisect bad f4e569664332b30eca48643ed5194d0da91b0560
# good: [7ccca6aec167c2c30c569765ece808f0eee023a6] [pcf] Disable long family names by default.
git bisect good 7ccca6aec167c2c30c569765ece808f0eee023a6
# bad: [8013d89f7fe7da123e3cd5708e501caf88405226] ftsnames.h, ttnameid.h, tttables.h: Revise documentation.
git bisect bad 8013d89f7fe7da123e3cd5708e501caf88405226
# bad: [56645f8a8ba0a08ed2bd383b5a1558e5c790bf74] Fix documentation of `FT_Get_Sfnt_Name'.
git bisect bad 56645f8a8ba0a08ed2bd383b5a1558e5c790bf74
# bad: [d718ac4ead0d711bd73d8103ba67cca10a55b3d9] [truetype] Provide metrics variation service.
git bisect bad d718ac4ead0d711bd73d8103ba67cca10a55b3d9
# bad: [07ee1d250c5ae008e3467dea39dfc0b7f99af0b3] [truetype] Parse `MVAR' table.
git bisect bad 07ee1d250c5ae008e3467dea39dfc0b7f99af0b3
# bad: [8ab08cff63eeb23b6c9f2c4470ae9809f2acf244] [truetype] More preparations for MVAR support.
git bisect bad 8ab08cff63eeb23b6c9f2c4470ae9809f2acf244
# first bad commit: [8ab08cff63eeb23b6c9f2c4470ae9809f2acf244] [truetype] More preparations for MVAR support.
-->8-

I attached new patch, which fixes this.

Also I created following application to get some related information for specified font (similar to what in http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/base/ftobjs.c?h=VER-2-8#n3076 and some from ftdump, but with ability to specify dpi):
-8<-- font_info
#include <ft2build.h>
#include FT_FREETYPE_H

FT_Error font_info(FT_Library ft, const char *font, const long dpi, const long size)
{
	FT_Error error;
	FT_Face face;

	error = FT_New_Face(ft, font, 0, &face);
	if (error) {
		fprintf(stderr, "Unable to load font: %s\n", font);
		return error;
	}
	// Set size in pixels
	//error = FT_Set_Pixel_Sizes(face, 0, size);
	error = FT_Set_Char_Size(face, size * 64, size * 64, dpi, dpi);
	if (error) {
		fprintf(stderr, "Unable to set font size for: %s\n", font);
	}
	else {
		printf("font: %s\nstyle: %s\ndpi: %ld\nsize: %ld\n", \
			face->family_name, face->style_name, dpi, size);
		//if (FT_IS_SCALABLE(face))
		printf("Face:\n"
			"  ascender: %f\n" \
			"  descender: %f\n" \
			"  height: %f\n",
			face->ascender / 64.0,
			face->descender / 64.0,
			face->height / 64.0
		);

		FT_Size_Metrics *metrics = &face->size->metrics;

		printf("Size metrics:\n" \
			"  ascender: %f\n" \
			"  descender: %f\n" \
			"  height: %f\n" \
			"  max advance: %f\n" \
			"  x scale: %ld (%f)\n" \
			"  y scale: %ld (%f)\n" \
			"  x ppem: %d\n" \
			"  y ppem: %d\n",
			metrics->ascender / 64.0,
			metrics->descender / 64.0,
			metrics->height / 64.0,
			metrics->max_advance / 64.0,
			metrics->x_scale, metrics->x_scale / 65536.0,
			metrics->y_scale, metrics->y_scale / 65536.0,
			metrics->x_ppem,
			metrics->y_ppem
		);
	}

	FT_Done_Face(face);

	return error;
}

int main(int argc, char **argv)
{
	if (argc < 2) {
		fprintf(stderr, "Usage: %s [-v] [-d dpi] [-s size] path_to_font\n", argv[0]);
		return 1;
	}

	const char *font = NULL;
	long dpi = 72, size = 10;
	int verbose = 0;

	for (int i = 1; i < argc; ++i) {
		if (argv[i][0] == '-') switch (argv[i][1]) {
			case 'd': {
				if (i + 1 < argc) {
					//dpi = atoi(argv[++i]);
					dpi = strtol(argv[++i], NULL, 10);
				}
				else {
					fprintf(stderr, "Specify dpi for the font\n");
					return 1;
				}
				break;
			}
			case 's': {
				if (i + 1 < argc) {
					//size = atoi(argv[++i]);
					size = strtol(argv[++i], NULL, 10);
				}
				else {
					fprintf(stderr, "Specify size (in pixels) for the font\n");
					return 1;
				}
				break;
			}
			case 'v': {
				verbose = 1;
				break;
			}
		}
		else {
			font = argv[i];
		}
	}

	if (font == NULL) {
		fprintf(stderr, "Specify path to the font\n");
		return 1;
	}
	
	if (verbose)
		printf("path: %s\n", font);

	FT_Error error;
	FT_Library ft;

	error = FT_Init_FreeType(&ft);
	if (error) {
		fprintf(stderr, "Unable to initialize FreeType\n");
		return 1;
	}

	error = font_info(ft, font, dpi, size);
	if (error)
		return 1;

	FT_Done_FreeType(ft);

	return 0;
}
-->8-
Comment 25 lightside 2017-07-01 07:47:41 UTC
Created attachment 183977 [details]
Some files/patch-fix_size_metrics.diff patch for test (for v2.8)

Cosmetic fixes.
Comment 26 lightside 2017-07-01 07:49:57 UTC
Created attachment 183978 [details]
Proposed patch for print/freetype2 (since 441132)

Attached new proposed patch.

By the way, I also found related topic on some Arch Linux forum:
"freetype2 2.8 line spacing issue":
https://bbs.archlinux.org/viewtopic.php?pid=1712478
where some author (Chazza) proposed freetype2-ttmetrics AUR package:
https://bbs.archlinux.org/viewtopic.php?pid=1717280#p1717280
https://aur.archlinux.org/packages/freetype2-ttmetrics/
https://aur.archlinux.org/cgit/aur.git/log/?h=freetype2-ttmetrics

But its current version (on 2017-06-11 date) contains some unnecessary changes, in my opinion, like defining AF_CONFIG_OPTION_TT_SIZE_METRICS:
https://aur.archlinux.org/cgit/aur.git/tree/enable_truetype_like_size_metrics.patch?h=freetype2-ttmetrics&id=a59d3ddd0388a9331b2e6cf0780ab273c38e61f6
or reverting too much (in my opinion, after some tests) and didn't check for error value before ttsize->root.metrics assign:
https://aur.archlinux.org/cgit/aur.git/tree/revert_allow_linear_scaling.patch?h=freetype2-ttmetrics&id=05178ddbcac5c8f9d5bf1812009de6b5d30c18f5
Comment 27 lightside 2017-07-01 08:05:44 UTC
Created attachment 183980 [details]
Testing environment for freetype2, based on git bisect

Attached some archive with testing environment, which I created.

How to use:
1. Extract (downloaded) attached archive to new directory:
% mkdir bisect
% tar -xf freetype2_bisect.tar.bz2 -C bisect
2. Change destination to new directory and clone freetype2 git repository in it:
% cd bisect
% git clone https://git.savannah.gnu.org/git/freetype/freetype2.git
or
% git clone https://github.com/cpp-mirrors/freetype2.git
3. Configure needed options in test.sh file, e.g. OPTION_SUBPIXEL_HINTING, OPTION_LONG_PCF_NAMES, OPTION_SUBPIXEL_RENDERING, which will be used, when available.
4. Configure "result" and "expected_checksum" variables in test.sh file. For example:
result=`./${TEST_APP} -d 96 -s 14 /usr/local/share/fonts/dejavu/DejaVuSansMono.ttf`
4.1 Then checkout VER-2-7-1:
% cd freetype2
% git checkout VER-2-7-1
4.2 And run test.sh in related directory:
% ../test.sh
If there were no errors, this gives some sha256 checksum (in case of using the same version of tested font), which need to specify for expected_checksum variable in test.sh file:
expected_checksum="0d4844bc2209e6038bf6e2090083bc6d3418c4576577fd28cb661f70a2a3411b"
4.3. (Optional) Checkout previous commit, e.g. master:
% git checkout master
5. Configure git bisect and run it:
% git bisect start VER-2-8 VER-2-7-1 --
% git bisect run ../test.sh
6. At the end this may show found commit, which possible to check with `git bisect visualize` and `git bisect log` commands.
7. Finish bisection search and go back to (previous) commit:
% git bisect reset
Comment 28 lightside 2017-07-01 08:58:51 UTC
Created attachment 183981 [details]
Some test results (need statically built font_info to reproduce)

(In reply to comment #22)
I found what font sizes were used in pango-view application for waterfall: https://github.com/GNOME/pango/blob/1.40.6/pango-view/viewer-render.c#L189-L194.
Looks like, font sizes between 8 and 48, per 4 increment.

Based on this, I created testing script for font_info console application (see comment #24 or attachment #183980 [details] from comment #27 for source code) to test between 1 and 100 sizes for "DejaVu Sans Mono Book" and "Bitstream Vera Sans Mono Roman" fonts:
-8<-- check_fonts.sh
#!/bin/sh

testapp=./font_info
results=results.txt
font="/usr/local/share/fonts/dejavu/DejaVuSansMono.ttf /usr/local/share/fonts/bitstream-vera/VeraMono.ttf"
dpi="72 96"
size="$(seq 1 100)"

if ! [ -f "${testapp}" ]; then
	echo "The ${testapp} file doesn't exist in the ${PWD} directory"
	exit 1
fi

if [ -f "${results}" ]; then
	mv -f "${results}" "${results}.bak"
	touch "${results}"
fi

for f in ${font}; do
	if ! [ -f ${f} ]; then
		echo "The ${f} font doesn't exist"
		continue
	fi
	for d in ${dpi}; do
		for s in ${size}; do
			${testapp} -d "${d}" -s "${s}" ${f} >> "${results}"
		done
	done
done

sha256 "${results}" > checksum.txt
-->8-

It showed the same results between 2.7.1 and 2.8_1 (with FIX_SIZE_METRICS option; results/2.7.1/results.txt, results/2.8_1_fix_size_metrics/results.txt), with using patch in attachment #183977 [details]. But different results for previous patch in attachment #183237 [details] (results/2.8_1_fix_size_metrics_prev/results.txt), 2.8 (with TT_SIZE_METRICS option and related patch for src/truetype/ttobjs.c; results/2.8_ttobjs/results.txt, just in case).

Results were attached to archive.

Some generated pango-view images, mentioned in comment #22, was placed to another location (because of file size restrictions; for about 2 months): https://files.fm/f/u6zx8wnj
SHA256 (pango-view_images.tar.bz2) = 8395b15cd5c5900a9f6e4337008e77b55d17dad924c6a2a9cfae86fad4b0c68c

The pango-view images (e.g. for Monospace-cairo-96dpi.png image) shows, that line spacing is the same between 2.7 and 2.8_1_fix_size_metrics, but some glyphs (may) look different. In the same time, the glyphs between 2.8_tt_size_metrics and 2.8_1_fix_size_metrics (may) look the same, but different line spacing. Probably, this is feature of 2.8 version (or pango-view related), where font rendering was changed for some fonts (and sizes). Again, checksums for ft2 backend may be different (compared to cairo backend), but visually the same.

(In reply to comment #24)
> -8<-- font_info
I meant font_info.c file, which possible to build with using following commands:
For static freetype2 library (e.g. after building of it, near include, objs, src, etc. directories):
% cc -DFT2_BUILD_LIBRARY -Iinclude -L/usr/local/lib -o font_info font_info.c objs/.libs/libfreetype.a -lz -lbz2 -lpng
For dynamic freetype2 library:
% cc -I/usr/local/include/freetype2 -L/usr/local/lib -o font_info font_info.c -lfreetype -lz -lbz2 -lpng
Comment 29 lightside 2017-07-01 23:18:20 UTC
Created attachment 183994 [details]
Some files/patch-fix_size_metrics.diff patch for test (for v2.8)

Added some optimized version of the patch.
Comment 30 lightside 2017-07-01 23:19:38 UTC
Created attachment 183995 [details]
Proposed patch for print/freetype2 (since 441132)
Comment 31 rkoberman 2017-07-02 01:01:55 UTC
I have tested with all apps I use that depend on freetype2 and it looks good. Also prefer the wording of the options is much clearer than the previous one (from a few days ago).

Thanks so much for your work on this.
Comment 32 lightside 2017-07-02 01:05:04 UTC
Created attachment 183997 [details]
Some files/patch-fix_size_metrics.diff patch for test (for v2.8)

Minimized some changes in the patch.
Comment 33 lightside 2017-07-02 01:05:55 UTC
Created attachment 183998 [details]
Proposed patch for print/freetype2 (since 441132)
Comment 34 lightside 2017-07-02 01:11:17 UTC
(In reply to rkoberman from comment #31)
> I have tested with all apps I use that depend on freetype2 and it looks good.
Thank you very much for testing (on your configuration).

(In reply to rkoberman from comment #31)
> Also prefer the wording of the options is much clearer than the previous one
> (from a few days ago).
I guess, the maintainer may decide what to change in descriptions, if needed.
Comment 35 lightside 2017-07-02 01:51:31 UTC
(In reply to rkoberman from comment #31)
> Also prefer the wording of the options is much clearer than the previous one
> (from a few days ago).
I guess, you meant about changed file name in FIX_SIZE_METRICS_EXTRA_PATCHES (from "extra-patch-src_truetype_ttdriver.c" to "extra-patch-fix_size_metrics.diff" (because of more files affected in the patch)), because FIX_SIZE_METRICS_DESC was the same in attachment #183251 [details]. Probably, you meant about proposed changes for TT_SIZE_METRICS_DESC.
So, it's fine, if I read your words correctly this time. I also changed some descriptions in the patch itself.
Comment 36 lightside 2017-07-03 07:45:38 UTC
Created attachment 184030 [details]
Proposed patch for print/freetype2

Attached patch, which created with using "svn diff" command. Suggested by PR's submitter.
Comment 37 Kubilay Kocak freebsd_committer freebsd_triage 2017-10-26 01:17:04 UTC
Please obsolete all patches that are not relevant/QA's/proposed for commit to head
Comment 38 Kubilay Kocak freebsd_committer freebsd_triage 2017-10-26 01:17:50 UTC
Comment on attachment 184030 [details]
Proposed patch for print/freetype2

Approved by: portmgr (maintainer timeout, 5+ months)
Comment 39 Kurt Jaeger freebsd_committer freebsd_triage 2017-10-26 04:33:03 UTC
testbuilds on 12a, 11.1a, 10.3i are fine
Comment 40 rkoberman 2017-10-26 04:56:20 UTC
(In reply to Kubilay Kocak from comment #37)
I'm confused. I see only one patch that has not been obsoleted. So do I need to do anything else or is this good to go?
Comment 41 commit-hook freebsd_committer freebsd_triage 2017-10-26 06:21:06 UTC
A commit references this bug:

Author: pi
Date: Thu Oct 26 06:20:54 UTC 2017
New revision: 452905
URL: https://svnweb.freebsd.org/changeset/ports/452905

Log:
  print/freetype2: fix incorrect line spacing for some fonts

  Problem description: After updating to print/freetypw2-2.8.0 using
  Bitstream Vera Sans Mono the vertical pitch increases and significantly
  add of the size of the window. See attached images.

  See PR for lots of links to related reports upstream
  and on other platforms.

  PR:		219608
  Submitted by:	lightside@gmx.com
  Reported by:	Kevin Oberman <rkoberman@gmail.com>

Changes:
  head/print/freetype2/Makefile
  head/print/freetype2/files/extra-patch-fix_size_metrics.diff
Comment 42 Kurt Jaeger freebsd_committer freebsd_triage 2017-10-26 06:21:33 UTC
Committed, thanks!