r-lib / vdiffr

Visual regression testing and graphical diffing with testthat
https://vdiffr.r-lib.org
Other
185 stars 31 forks source link

How to accept or debug almost exact matches (failures) #97

Closed krassowski closed 3 years ago

krassowski commented 3 years ago

All my tests were passing nicely up until the moment I upgraded (new computer, R 4.0 etc) and when I got back to development the new doppelgangers for ggplot graphs became ever so slightly different, with the majority of differences in sub-pixel text length differences, for example:

< my current Ubuntu (20.10)
> my previous results, older Ubuntu version, macOS-latest on GitHub and Travis CI

< <g clip-path='url(#cpMC4wMHw3MjAuMDB8NTc2LjAwfDAuMDA=)'><text transform='trans
: late(104.11,227.68) rotate(-90)' style='font-size: 11.00px; font-family: Liber
: ation Sans;' textLength='67.06px' lengthAdjust='spacingAndGlyphs'>MPAA Raiting
: </text></g>                                                                   
> <g clip-path='url(#cpMC4wMHw3MjAuMDB8NTc2LjAwfDAuMDA=)'><text transform='trans
: late(104.11,227.69) rotate(-90)' style='font-size: 11.00px; font-family: Liber
: ation Sans;' textLength='67.08px' lengthAdjust='spacingAndGlyphs'>MPAA Raiting
: </text></g>                                           

the difference here is in two places (104.11,227.68 vs 104.11,227.69, and 67.06px vs 67.08px). The difference is so small that when reviewing the changed doppelgangers I was not able to see any difference in the diff mode at all.

I cannot figure out what causes it (and I think that it is something wrong locally on my computer, because various OSes on Travis and GH Actions give old results, with both R 3.6 and R 4.0).

Three questions:

krassowski commented 3 years ago

I checked versions of vdiffr, fontquiver, ggplot2, vdiffr-svg-engine, etc.

I'm seeing a similar flicker here: https://github.com/r-lib/svglite/pull/104/files#diff-42122f00f87a1a9ef5e54c7b7b15f8e19fc96156dcaafdfa4de045d2ccc98733 but I do not know if this is related. It seems that vdiffr does not depend on svglite, yet it uses its code in some version - is this correct?

krassowski commented 3 years ago

I can reproduce the value of 67.06px locally with:

> str_width("MPAA Raiting", font_size = 11)
[1] 67.0625

Same with:

> file <- fontquiver::font("Liberation", "sans", "regular")$ttf
> str_width("MPAA Raiting", font_size = 11, font_file = file)
[1] 67.0625

Where:

> file
[1] "/usr/lib/R/site-library/fontLiberation/fonts/liberation-fonts/LiberationSans-Regular.ttf"
md5sum /usr/lib/R/site-library/fontLiberation/fonts/liberation-fonts/LiberationSans-Regular.ttf
2a9e080aae06fccde903e2d54f42b801  

with:

> sessionInfo()
R version 4.0.2 (2020-06-22)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.10

Matrix products: default
BLAS:   /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0
LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0

locale:
 [1] LC_CTYPE=en_GB.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_GB.UTF-8        LC_COLLATE=en_GB.UTF-8    
 [5] LC_MONETARY=en_GB.UTF-8    LC_MESSAGES=en_GB.UTF-8   
 [7] LC_PAPER=en_GB.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_GB.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] freetypeharfbuzz_0.2.5

loaded via a namespace (and not attached):
[1] compiler_4.0.2          fontLiberation_0.1.0    fontquiver_0.2.1       
[4] fontBitstreamVera_0.1.1

More info:

> str_info("MPAA Raiting", font_size = 11, font_file = file)
   width   height   ascent  descent 
67.06250 10.25000  7.96875  2.28125 
> font_info(font_size = 11, font_file = file)
  ascent  descent  linegap 
9.953125 2.328125 0.375000 
krassowski commented 3 years ago

Yet, when I run the same script on https://rdrr.io/snippets/, I get a different value:

file <- fontquiver::font("Liberation", "sans", "regular")$ttf
freetypeharfbuzz::str_width("MPAA Raiting", font_size = 11, font_file = file)
[1] 67.07812

while the path is the same:

file
[1] "/usr/lib/R/site-library/fontLiberation/fonts/liberation-fonts/LiberationSans-Regular.ttf"

the md5 checksum differs:

> html::md5sum(file)
bbbd0bdbfba093dec2ee4ca8b62e61f9

even though the fontLiberation package version is the same both locally and on rdrr.io (0.1.0)...

This leads me to discover that my fontLiberation installation is lazy. It does not actually ship ttf fonts, it just symlinked them:

$ ls /usr/lib/R/site-library/fontLiberation/fonts/liberation-fonts/ -l
total 2364
-rw-r--r-- 1 root root    242 May 30  2020 AUTHORS
-rw-r--r-- 1 root root    806 May 30  2020 ChangeLog
lrwxrwxrwx 1 root root     80 May 30  2020 LiberationMono-BoldItalic.ttf -> ../../../../../../share/fonts/truetype/liberation2/LiberationMono-BoldItalic.ttf
-rw-r--r-- 1 root root 162884 May 30  2020 LiberationMono-BoldItalic.woff
lrwxrwxrwx 1 root root     74 May 30  2020 LiberationMono-Bold.ttf -> ../../../../../../share/fonts/truetype/liberation2/LiberationMono-Bold.ttf
-rw-r--r-- 1 root root 171036 May 30  2020 LiberationMono-Bold.woff
...
lrwxrwxrwx 1 root root     78 May 30  2020 LiberationSerif-Regular.ttf -> ../../../../../../share/fonts/truetype/liberation2/LiberationSerif-Regular.ttf
-rw-r--r-- 1 root root 207624 May 30  2020 LiberationSerif-Regular.woff
-rw-r--r-- 1 root root   1977 May 30  2020 README
-rw-r--r-- 1 root root    350 May 30  2020 TODO

Now, I guess it might suggest that Ubuntu 20.10 shipped with updated fonts.

@lionel- would it be possible to enforce that fontLiberation uses a consistent version of the font files across all installations? It might also explain the earlier issues, e.g. #93.

krassowski commented 3 years ago

I tried down-grading fonts-liberation2 to 2.1.0-1 and to 2.00.1-5 but it did not help. It worked with Ubuntu 20.04 which had 2.1.0-1, so it is even more confusing now.

But I do believe it is a real problem (lack of reproducibility between installations was previously reported) and an important one. Any help would be appreciated.

krassowski commented 3 years ago

Oddly enough all letters are of identical width. I probably should note that this issue does only appear for some strings, not all of them. I verified that all letters, digits and uppercase letters (and space) are of the same width with:

library(freetypeharfbuzz)
file <- fontquiver::font("Liberation", "sans", "regular")$ttf
characters = data.frame(letter=c(letters, LETTERS, ' ', 0:9))
characters$width = lapply(characters$letter, str_width, font_size = 11, font_file = file)
characters[characters$letter == ' ', 'letter'] = '<space>'
print(characters)

I saved the result from rdrr.io as reference.tsv, my local result as local.tsv and compared:

reference = read.table('reference.tsv')
local = read.table('local.tsv')
local$width == reference$width
all(local$width == reference$width)

which returns TRUE.

krassowski commented 3 years ago

To investigate further I compared widths of all substrings:

library(freetypeharfbuzz)
file <- fontquiver::font("Liberation", "sans", "regular")$ttf
text = "MPAA Raiting"
result = list()
for (i in 1:nchar(text)) {
   for (j in i:nchar(text)) {
      str = substr("MPAA Raiting", i, j)
      result[[length(result) + 1]] = list(
         substring=str,
         width=str_width(str, font_size = 11, font_file = file)
      )
   }
}
write.csv(do.call(rbind, result), textConnection("output", "w"))
cat(paste(output, collapse='\n'))

I saved the result from rdrr.io as reference.csv, my local result as local.csv and compared:

reference = read.csv('reference.csv', row.names=1)
local = read.csv('local.csv', row.names=1)
combined = merge(local, reference, by = 'substring', suffixes = c('_local', '_reference'))
combined[combined$width_local != combined$width_reference, ]

which produces:

      substring width_local width_reference
49          MPA    23.01562        23.03125
50         MPAA    30.35938        30.37500
51        MPAA     32.81250        32.82812
52       MPAA R    40.75000        40.76562
53      MPAA Ra    46.87500        46.89062
54     MPAA Rai    49.31250        49.32812
55    MPAA Rait    52.37500        52.39062
56   MPAA Raiti    54.81250        54.82812
57  MPAA Raitin    60.93750        60.95312
58 MPAA Raiting    67.06250        67.07812
62           PA    13.85938        13.87500
63          PAA    21.20312        21.21875
64         PAA     23.65625        23.67188
65        PAA R    31.59375        31.60938
66       PAA Ra    37.71875        37.73438
67      PAA Rai    40.15625        40.17188
68     PAA Rait    43.21875        43.23438
69    PAA Raiti    45.65625        45.67188
70   PAA Raitin    51.78125        51.79688
71  PAA Raiting    57.90625        57.92188

highlighting that PA is a minimal reproducible example.

krassowski commented 3 years ago

I downloaded fontLiberation_0.1.0.zip build for Windows from CRAN because it contains the fonts in tff format. I extracted the fonts to a local fonts directory and run the str_width with the obtained font file:

path_to_font_from_windows_build = 'fonts/liberation-fonts/LiberationSans-Regular.ttf'
freetypeharfbuzz::str_width('PA', font_size = 11, font_file = path_to_font_from_windows_build)

which produces:

13.85938 although it should have produced 13.87500 according to the reference.

which provides another evidence suggesting that it is not because of a change in the fonts file, but due to another (potential) bug in freetypeharfbuzz (which should have been reproducible but is not).

krassowski commented 3 years ago

I presume that this is still the right place to report freetypeharfbuzz issues because it seems to have been created for vdiffr specifically (and it has no GitHub page mentioned on CRAN):

Unlike other tools that dynamically link to the 'Cairo' stack, 'freetypeharfbuzz' is statically linked to specific versions of the 'FreeType' and 'harfbuzz' libraries (2.9 and 1.7.6 respectively). This ensures deterministic computation of text box extents for situations where reproducible results are crucial (for instance unit tests of graphics).

I re-installed freetypeharfbuzz with: install.packages('freetypeharfbuzz') and the problem went away:

> path_to_font_from_windows_build = 'fonts/liberation-fonts/LiberationSans-Regular.ttf'
> freetypeharfbuzz::str_width('PA', font_size = 11, font_file = path_to_font_from_windows_build)
[1] 13.875

This suggests that the system-wide installation of freetypeharfbuzz was the culprit:

$ apt list --installed | grep r-cran-freetypeharfbuzz
r-cran-freetypeharfbuzz/groovy,now 0.2.5+dfsg-2build1 amd64 [installed,automatic]

Maybe Ubuntu build used a a different version of static libraries that freetypeharfbuzz depends on?

The package lives here: r-cran-freetypeharfbuzz and was most recently updated on 2020-10-23 (0.2.5+dfsg-2build1), but as I only recently updated to Ubuntu 20.10 so I was not exposed to it earlier.

lionel- commented 3 years ago

I'm glad you could solve it, hopefully that unblocks you.

FWIW the next version of vdiffr (to be released around the time R 4.1 will be released) will drop the freetypeharfbuzz dependency. We will only support ascii characters in vdiffr snapshots and the font extent computations will be fully hardcoded. This will make things much simpler and robest.

krassowski commented 3 years ago

I reported this as a potential bug in the Ubuntu version of the package: 1906836. I guess we can close this issue, unless you wish to rename it ("Font width reproducibility issues on Ubuntu"?) and keep it open for visibility until the upstream issue is confirmed/fixed.

lionel- commented 3 years ago

Fixed by #106 which removes the freetypeharfbuzz dependency.