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

Always run full FontVal rendering tests (its now a hard requirement for all future pushes) #1524

Closed
m4rc1e opened this issue Aug 17, 2017 · 34 comments
Assignees

Comments

@m4rc1e
Copy link
Collaborator

m4rc1e commented Aug 17, 2017

Due to issue google/fonts#1137 we need to work on font rendering tests. MS Font Validator used to be able to run rendering tests, however the newest version produces the following errors when +rendering-tests argument is added.
screen shot 2017-08-16 at 16 20 21

This argument is also not used when we run test com.google.fonts/test/037, check_with_msfontvalidator, https://github.com/googlefonts/fontbakery/blob/master/Lib/fontbakery/specifications/googlefonts.py#L1326-L1329

I wrote a screenshot generator for Roboto which uses the BrowserStack screenshot api, https://github.com/googlefonts/robototester/blob/master/screenshots.py. It may be wise to do something similar.

@HinTak
Copy link

HinTak commented Aug 18, 2017

Hey, you could ping me on these issues, you know? The rasterization test depends on a fairly customized version of freetype. Pre-built ones for win32/win64 and Darwin are here - https://github.com/HinTak/Font-Validator/tree/master/bin . On windows/wine, it should just find them. If you are on Mac OS X and running with mono, setting LD_LIBRARY_PATH to the Darwin directory should work too. linux binary is too hard, and there is current no plan to upstream the rather substantial patch.

BTW, the prioprietary 1.0 backend does not run correctly under stock wine (ie. wine + wine-mono) but do work okay under wine + ms dotnet.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Aug 18, 2017

@HinTak I was going to ping you once I understood the problem better. First, I need to compare your version against legacy Font Validator before I waste your time. Anyways, thank you for the explanation, this is very helpful.

@HinTak
Copy link

HinTak commented Aug 18, 2017

I haven't quite kept thoso branches in sync - the "hybrid*" branches uses the old legacy 1.0 backend, and give you a command line wrapper around it. I occasionally use it to compare results. As of v2.1, output should be superset of legacy 1.0 more or less, and I 'd interested to document notable differences.

The problem of running the legacy backend in stock wine (i.e. Wine-mono) is
madewokherd/wine-mono#29

There is a url for a binary there also.

@davelab6 davelab6 changed the title Add font rendering tests Always run full Hin-Tak FontVal rendering tests (its now a hard requirement for all future pushes) Aug 22, 2017
@felipesanches
Copy link
Collaborator

@HinTak I have built FontValidator again from the latest commit on master branch as of today (HinTak/Font-Validator@da9029f) and I can confirm that running it on GNU/Linux with mono and with the +raster-tests flag, causes a message similar to the one @m4rc1e has got.

screenshot at 2017-08-23 19 12 46

Could you please describe the changes that were applied to "fairly customized version of freetype" and where I can take a look at the patched source code? This is important enough for us that I'm willing to look at it more closely myself to make it work.

@felipesanches
Copy link
Collaborator

The "UnImplemented" message comes from
https://github.com/HinTak/Font-Validator/blob/22e1d47ec8f44e25945ab3a2ab3acc6be387cf7d/Compat/Compat.cs#L137-L144

            try
            {
                TT_Diagnostics_Unset();
            }
            catch (Exception e)
            {
                throw new NotImplementedException("UnImplemented in this version of Freetype: " + FTVersion);
            };

A Google search gives me this single query result:
http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00016.html

@HinTak
Copy link

HinTak commented Aug 23, 2017

The b06 patch was as from Werner:
http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00016.html

If you add -DFT_DIAGNOSTICS (which switches on the FontVal hooks) to your CFLAGS, you would discover that Werner made a mistake in the "else" clause - he missed a closing semi-colon. That's sufficient for you to run FontVal while setting LD_LIBRARY_PATH to the libtool directory (i.e. objs/.libs/ within your freetype source tree).

If you are installing such and packaging elsewhere (as I do when making the windows/mac releases), and/or stripping and does symbol hiding, you need to apply this also, to expose the two new symbols:

diff --git a/builds/exports.mk b/builds/exports.mk
index d5a5085..c95c6e8 100644
--- a/builds/exports.mk
+++ b/builds/exports.mk
@@ -68,6 +68,8 @@ ifneq ($(EXPORTS_LIST),)
 	  $(subst /,$(SEP),$(APINAMES_EXE)) -o$@ $(APINAMES_OPTIONS) $(PUBLIC_HEADERS)
 	  @echo TT_New_Context >> $(EXPORTS_LIST)
 	  @echo TT_RunIns >> $(EXPORTS_LIST)
+	  @echo TT_Diagnostics_Unset >> $(EXPORTS_LIST)
+	  @echo TT_Diagnostics_Set >> $(EXPORTS_LIST)
 
   $(PROJECT_LIBRARY): $(EXPORTS_LIST)
 

Just having b06 should give you the earliest FontVal prototype's capability and silence the message you saw.

As you see b06 has one foundamental problem, and a fairly big one: it has a couple of global symbols.
This does not stop the current use as non-system private freetype with the windows/mac usage, so that's how it continued. Been asking @davelab6 to lobby Google to fund further work for almost two years now... ( CC @anthrotype )

After b06, all the other patches built on top of b06.

b06 is 16k, patch 2 is 15k (b06 to b41), patch 3 is 13k (b41 to b54) ; there are some smaller patches between the major snapshot builds, but b06, b41 and b54 were the ones I made available to others - i.e. what appears in the public repository's bin directory. b54 was released as FontVal 2.0

b55 to b66 is another 13k patch (there are a few small ones between b54 and b55)
b66 to 68+6.5 is another 17k patch
b68 to b70 is another 12k patch.

b70 is Fontval 2.1.0

I am re-basing and trying to do 2.1.1 (which is just a mac-backend update built with a new cross-compiler:

$ git diff origin/master | dd of=/dev/null
179+1 records in
179+1 records out
91665 bytes (92 kB, 90 KiB) copied, 0.0891346 s, 1.0 MB/s

so that's about right - 92k is about 6 patches of 15k each plus smaller ones...

Anyway, having b06 alone should silence that error you saw.

@HinTak
Copy link

HinTak commented Aug 23, 2017

@felipesanches beats me to the url for b06 while I wrote the last lengthy message :-) . Summarizing:

  • add -DFT_DIAGNOSTICS to CFLAGS
  • add a missing semi-colon (it should be obvious...) at the end of the else clause enabled by the above.
  • massage the export symbol list.

That's good enough to get b06 running.

And remember - DO NOT copy the resulting libfreetype over your system one! put it elsewhere and Set LD_LIBRARY_PATH .

@HinTak
Copy link

HinTak commented Aug 24, 2017

I just pushed HinTak/Font-Validator@6c8b565 - which contains what's to be 2.1.1 @davelab6 @anthrotype - can't decide on which cross-compiler to use :-(. The largest one is triarch and built the old way, but does not have the math overflow checks; the clang one is larger but depends on libgcc_s - the gcc one is smallest and also does not depend on libgcc_s, but it is a two-stage build and a manual lipo (more work for me)... I'd prefer the clang one if it works okay. So you guys on the mac, give it a try, with perhaps the python wrapper ( https://github.com/HinTak/freetype-py/tree/fontval-diag ) if you prefer.

@felipesanches
Copy link
Collaborator

felipesanches commented Aug 24, 2017

If there's a known typo in that freetype source file, then it seems that the ideal thing to do would be to send a patch fixing that or, at least, to inform the devteam that there's a typo to be fixed (maybe opening a bug-report on the freetype bug tracker ?)

@HinTak Have you done it? Or would you like me to do it?

@HinTak
Copy link

HinTak commented Aug 24, 2017

It is not a typo in freetype's source. I posted an earlier version of the b06 patch for review 2 years ago, Werner made a fair number of stylistic changes as an illustration of how it should be changed to be more in freetype coding style, to be upstreamed eventually. Werner's version does patch and compile cleanly without -DFT_DIAGNOSTICS . The typo only shows with you add -DFT_DIAGNOSTICS to CFLAGS. It still applies cleanly to current, and I have been using it as is (plus another 5 patches of similiar size, and a lot of smaller ones). While merging the semi-colon and re-posting it would be nice, it isn't necessary. Considering that what is likelt to be 2.1.1 ( HinTak/Font-Validator@6c8b565 ) is 33 patches on top of freetype 2.8, cutting the number down to 32 isn't going to make any difference to anybody...

I already gave you 4 of the 33 - the other 3 are the CFLAGS define, the export list, and the typo - they exist in my scripted build as patches. There is a 5th you can do yourself, the culmulatiive one from 2.8 to current devel - I wanted/needed one or two changes after 2.8. A 6th is also posted in public (https://github.com/HinTak/Font-Validator/blob/master/freetype-win64-2.8.patch) but that's 64-bit windows only, and also seems to be subtly buggy ( HinTak/Font-Validator#27 ).

@HinTak
Copy link

HinTak commented Aug 24, 2017

You might guess from my 2.1.1 backend commit message that my freetype dev repo is 81 commits ahead of upstream. That collapses to 33 patches at the moment when I cross-compile for windows and mac. There are some differences between the two and the sum-total of 81 isn't identical to the sum total of the 33. Notably is the win64 patch which is only kept in the cross-compile setting, and the dev repo have some extra comments and notes and experimental code and printf's.

@felipesanches
Copy link
Collaborator

felipesanches commented Aug 24, 2017

It puzzles me why do you keep such a large number of patches instead of integrating them into upstream. Did the patches get rejected by the freetype team? Or do you perhaps have any reason to not want to upstream the patches ?

Upstreaming the patches would make life much easier. But first of all, I still do not understand exactly what is the role these 33-ish patches. What kind of additional features are you adding with this patch-set that vanilla freetype currently lacks?

@HinTak
Copy link

HinTak commented Aug 24, 2017

you can follow the discussion of b06 on freetype-devel . Yes, it was rejected. Understandably the goals are different: for diagnostic you don't worry about speed, and for a private copy of freetype (not shared with the system's font rendering), have global variables is not a problem - it is a feature. "Not trying to upstream" is crucial and important to allows it to reach its current state, which supercedes the legacy backend . I just decide not to spend time in that direction. Please feel free to lobby google to fund the work.

And I already gave you 6 of the 33. You can decide what they are for. As Yoda says, "use the source, Luke!".

Oh, the win64 patch has also been brought up a few times (at least twice, as far as I remember) and been rejected.

@HinTak
Copy link

HinTak commented Aug 24, 2017

Also until quite recently, the patch set wasn't "complete" - the number refers to what proportion of the legacy backend is implemented. There are 70 in total. b68 to b70 itself was a 12k diff, and requires switching to a newer cross-compiler.

(FontVal 2.1 is officially b70+15)

@felipesanches
Copy link
Collaborator

Could you please provide us with a copy of the full patch-set mentioned in this thread ? I'd like to study them in order to better understand what are we talking about here in more detail. Sorry in case these patches are already publicly available in some version control system (if that's the case an URL would be enough). I did not find that, so I concluded that it would be better to ask for it explicitly here.

@HinTak
Copy link

HinTak commented Aug 24, 2017

@felipesanches : Thanks for agree to help. Here are what you can do:

CC @davelab6 @anthrotype

@HinTak
Copy link

HinTak commented Aug 24, 2017

@felipesanches As I mentioned earlier, the win64 patch and why it was rejected was raised at least twice. The 2nd time was May 2017 this year. The beginning of that thread is
http://lists.nongnu.org/archive/html/freetype-devel/2017-05/msg00073.html
I am sorry, according to my e-mails, apparently the first time it was raised , it was not raised in public - it was about a dozen e-mails between 3 people, me and Werner and the SharpFont developer Robert Rouhani in September 2015, titled "FreeType patches", then morf'ed into "4 patches, 2 for freetype and 2 for SharpFont (Re: FreeType patches)". That was because it was even before Microsoft opened the FontVal code (later in November), and we didn't know at the time if it would indeed be opened later.
There is nothing really secret about that thread and I could put the whole thread somewhere public if you feel like reading. @davelab6 and @anthrotype could both tell you that I had earlier access to the code by a couple of months at the beginning of August.

@HinTak
Copy link

HinTak commented Aug 24, 2017

There is also some discussion about the win64 patch on
Robmaister/SharpFont.Dependencies#5 - it happened about a week after the 2nd discussion on freetype-devel . You can consider it either the 3rd discussion, or a follow-up of the 2nd happening elsewhere.

@HinTak
Copy link

HinTak commented Aug 28, 2017

I have managed to make the whole thing into one binary, FontVal-2.1.1-py-ubuntu-14.04-x64.tgz ( and the gpg signature FontVal-2.1.1-py-ubuntu-14.04-x64.tgz.sig )
https://sourceforge.net/projects/hp-pxl-jetready/files/Microsoft%20Font%20Validator/misc/

As the name implies, it targets Ubuntu 14.04, but it does work on fedora 26 and perhaps most recent 64-bit linux too.

You do not need any mono bits nor the heavily customized freetype. They are all hidden inside the one file. This is still slightly behind the windows/mac backend as Ubuntu 14.04 only has gcc 4.8.x .

edit: I uploaded a Ubuntu 16.04 binary also. It is considerd preferable as Ubuntu 16.04 ships gcc 5.x .

@HinTak
Copy link

HinTak commented Aug 31, 2017

I uploaded FontVal-2.1.1-osx-10.7-x64.tgz and FontVal-2.1.1-py-ubuntu-16.04-x64.tgz to
https://sourceforge.net/projects/hp-pxl-jetready/files/Microsoft%20Font%20Validator/
Most people want the chm file FontValidatorHelp-2.1.chm for reference also. Here is the relevant part of the release note:

FontVal 2.1.1
=============

- This is an update on the FreeType backend only, to VER-2-8-100-g587264cfd .
  We now also offer all-in-one command-line binaries for Ubuntu Linux x86_64
  14.04 and 16.04, in addition to Mac OS X. ( Thanks to new functionality
  in mono introduced in 4.6.0 and matured in 4.8.0 in the last 12 months. )
  The Ubuntu 16.04 binary is preferred, as the 14.04 binary is limited
  by older compiler as mentioned in FontVal 2.1.

I am just waiting to obtain the last name of the person who helped testing it multiple times for honourary mention before posting to freetype-devel/create.

@m4rc1e @felipesanches @davelab6 @anthrotype

felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
... freetype builds on this repo anymore!
(issue fonttools#1524)
@felipesanches
Copy link
Collaborator

Thanks, @anthrotype ! That's exactly what was wrong in my code. Now it is running nicely.

felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
... required by my build of FontValidator.exe
(issue fonttools#1524)
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
... required by my build of FontValidator.exe
(issue fonttools#1524)

I'm ammending this now to retrigger a Travis build (due to issue fonttools#1566)
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
... the freetype backend.
Right now this test will fail on Travis because I have not setup freetype there yet.
(issue fonttools#1524)
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Sep 6, 2017
@HinTak
Copy link

HinTak commented Sep 6, 2017

The passage:

"If your vanilla system freetype is used instead, then all FontValidator tests will still be run, except for the hinting validation ones."

is wrong. A few of the table tests requires FreeType 2.6.1 minimum. That's called the "ComputeMetrics Patch".

@felipesanches
Copy link
Collaborator

Thanks, @HinTak

Right now I'm satisfied with the current setup. More on that at #1567 (comment)

I may work more on this on the future, but right now I'm moving on to other unrelated Font Bakery issues.

@HinTak
Copy link

HinTak commented Sep 6, 2017

@felipesanches I think that a lot of things I said are simply not registering with you. Having b06 supressing the "Unimplmented Exception" you see; but I don't think that was @davelab6 's only goal. For most useful purposes, including detecting some of the endless loops, etc, you need FontVal 2.1. FontVal 2.1 is also a lot neater to use also, no mono bits, no LD_* variables, and finally "works on Linux too". And I gpg-signed it, which is as good as anything - I don't gauranteed that it is bug-free, but things from distros are not either. (FWIW, one of the bugs I filed at Xamarin last week concerns their "official" mono binaries).

@felipesanches
Copy link
Collaborator

I'm fully aware of the extra features that your (currently) proprietary binary offer. I can't trust that binary though, without corresponding source code. I made it clear several times that this is my criteria. Other people (including some users of Font Bakery) may want to use your non-free binary and the current setup does not forbid that (even though I discourage such usage).

I won't spend more time on this because now I feel I'm wasting my time repeating myself here.

@fonttools fonttools locked and limited conversation to collaborators Sep 6, 2017
@felipesanches
Copy link
Collaborator

And since the title mentions your name ("HinTak" referring to your partially private patch-set), I'll reopen this so we don't forget to somehow address this in the future.

@felipesanches felipesanches reopened this Sep 6, 2017
@felipesanches felipesanches modified the milestones: 0.4.0 - Future major release, 0.3.2 - Further improvements on reliability of checks and fixers Sep 6, 2017
@fonttools fonttools unlocked this conversation Sep 6, 2017
@HinTak
Copy link

HinTak commented Sep 6, 2017

Yes, I do think that if you put my name in it, you should listen to my recommendation. I don't object to you spending your time on whatever, but just don't call it Hin-Tak's when you don't offer the "whole experience" :).

Also for the Nth time, it is not "non-free" nor "proprietary" - it is just not (yet) suitable for public consumption. Consider that it is "alpha" software which is useful enough and therefore made available "early", for a value of "alpha" and a value of "early". You are free to spend time improving it and going in that direction. I have enough to do, and serving a linux user of one (me), just isn't a priority. I don't count you as a user, since you have not used it for what it does, yet.

@felipesanches felipesanches changed the title Always run full Hin-Tak FontVal rendering tests (its now a hard requirement for all future pushes) Always run full FontVal rendering tests (its now a hard requirement for all future pushes) Sep 6, 2017
@felipesanches
Copy link
Collaborator

While source code is missing, I can't trust it.

@HinTak
Copy link

HinTak commented Sep 6, 2017

You are asking me to remind you again, that you cannot read code when it is put in front of your eyes for two days multiple times.

@felipesanches
Copy link
Collaborator

Please stop !

@fonttools fonttools locked and limited conversation to collaborators Sep 6, 2017
@davelab6 davelab6 modified the milestones: 0.4.0 - Future major release, 0.3.3 (upcoming minor release) Nov 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants