Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Werner LEMBERG]

> I think the enumeration values should be FT_DIAGNOSTIC_XXX too.
> Sometimes it's a good idea to spell things out.

This is fine with me.


Werner

___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Werner LEMBERG]

>> Do people still care about restricting to the convention of 8.3
>> file names?
>
> Not this century.

Wll...  I like consistency, so I prefer `ftdiag.h' – people would
include this as `FT_DIAGNOSTICS_H' anyway.

BTW, instead of using `diagnose' we could base the used names on
`validate'; this would make `ftvalid.h', FT_VALIDATE_XXX, etc.


Werner
___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Werner LEMBERG]

> I'd likely just use your patch instead, when I do the win32/64 and
> mac os x binaries in the next couple of days.

This is just a suggestion, so please massage it as needed!

> I thought about the enum vs string issue (FT_DIAG_XXX) a bit, as I
> mentioned at the beginning - the problem I am facing is that it is 6
> out of 77 out of about 800 of the master list.  The master enum list
> will change when new opentype-table tests are added, so the raster
> test subset of enum will change.  If I have it as a enum on the
> freetype side, that means I have to have another on the C# side to
> do a small enum to large enum look-up, and maintaining two
> additional tables, one on freetype side and one on C# side; and
> forever keeping them in sync...

I have no idea about C# programming, but wouldn't it be possible to
share a file between C and C#, using the preprocessor to provide the
necessary glue to create valid C code?

> I didn't think a FT_DIAGNOSTICS except the first one is necessary,
> as '#define DIAGNOSTICS( message, context ) do { } while ( 0 )'
> should kill off the whole patch to nothing.  I didn't want to add
> #ifdef in the latter parts, for readability, like you just did; but
> if your like a few more #ifdef FT_DIAGNOSTICS I'm okay with that
> (when I add more things, in the future).

IIRC you get compiler warnings if you have code in conditionals that
does nothing, so I think the use of FT_DIAGNOSTICS is justified.

> Sadly I know of no other linux/unix users of this code other than
> me :-(.  So the issue of bundled patched freetype vs upstream/system
> freetype affects exactly one person...

:-)

> Speaking of which, I am a bit surprised that with freetype 2.6.4,
> subpixel hinting is enabled by default - are you sure about that?

Actually yes.  Why not?  Today, you get superior rendering results for
most recent fonts.

> I have opted to revert that in my bundled patched freetype: I want
> subpixel hinting available and dynamically switchable between B/W,
> gray, cleartype testing, via property_set, but kept the older
> default of subpixel hinting off, for now.

You still can switch at runtime between all modes, cf. the demo
programs.


Werner
___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Hin-Tak Leung]

On Wed, 6/7/16, Graham Asher  wrote:

> Please
 consider calling it
   ftdiagnostics.h, which is more explanatory. To my
   native-English-speaking mind, 'diag' looks as
 if it stands for
   'diagonal' or 'diagram'.


On the subject of additional header for enum types of errors, an earlier 
iteration of the patch I did, have it as an additional header 
("tt_msvalidate_errors.h" was one choice I had, of naming it). I threw away and 
deleted it after I choose to pass string instead... but I noticed that files in 
freetype are still of 8.3 convention.

Do people still care about restricting to the convention of 8.3 file names?

Hin-Tak 

  

___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Graham Asher]

Not this century.

On 06/07/2016 10:37, Hin-Tak Leung wrote:

Do people still care about restricting to the convention of 8.3 file names?


--
Graham Asher
founder and CTO
CartoType Ltd
graham.as...@cartotype.com
+44 (0) 7718 895191
___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Hin-Tak Leung]

Thanks for reviewing and revising the patch. I'd likely just use your patch 
instead, when I do the win32/64 and mac os x binaries in the next couple of 
days. Right now I still have a few things to do on the C# side.

I thought about the enum vs string issue (FT_DIAG_XXX) a bit, as I mentioned at 
the beginning - the problem I am facing is that it is 6 out of 77 out of about 
800 of the master list. The master enum list will change when new 
opentype-table tests are added, so the raster test subset of enum will change. 
If I have it as a enum on the freetype side, that means I have to have another 
on the C# side to do a small enum to large enum look-up, and maintaining two 
additional tables, one on freetype side and one on C# side; and forever keeping 
them in sync...

I didn't think a FT_DIAGNOSTICS except the first one is necessary, as '#define 
DIAGNOSTICS( message, context )  do { } while ( 0 )' should kill off the whole 
patch to nothing. I didn't want to add #ifdef in the latter parts, for 
readability, like you just did; but if your like a few more #ifdef 
FT_DIAGNOSTICS I'm okay with that (when I add more things, in the future).

Sadly I know of no other linux/unix users of this code other than me :-(. So 
the issue of bundled patched freetype vs upstream/system freetype affects 
exactly one person... Speaking of which, I am a bit surprised that with 
freetype 2.6.4, subpixel hinting is enabled by default - are you sure about 
that? I have opted to revert that in my bundled patched freetype: I want 
subpixel hinting available and dynamically switchable between B/W, gray, 
cleartype testing, via property_set, but kept the older default of subpixel 
hinting off, for now. Just less surprising, as subpixel-hinting did affect 
HDMX/VDMX/LTSH calculations, from testing last year.

Hence I just filed two issues for SharpFont ( 
https://github.com/Robmaister/SharpFont/issues/82 , 
https://github.com/Robmaister/SharpFont/issues/83 ) - 
and supplied a patch pull to half of 1 issue. That's just for dynamically 
switching interpreter behavior via property_set not working :-(. Not looking 
forward to upstreaming the C# side of the diagnostic enhancement :-(.


On Wed, 6/7/16, Werner LEMBERG  wrote:

 >
 Here is the updated patch for 2.6.4 - there are some
 minor
 > collisions with the new subpixel
 hinting mode.
 
 Attached is
 something that would fit better into FreeType
 w.r.t. formatting, macro definitions, and
 function names.  I suggest
 to provide a
 public header file `ftdiag.h' that defines, similar
 to
 `fterrdef.h', enumeration values
 `FT_DIAG_XXX' instead of the
 currently
 used strings, together with proper declarations for
 `TT_Diagnostics_{Unset,Set}' (which
 probably should use an `FT_'
 prefix
 instead).
 
 > while
 updating the diff I looked into the global variable
 issue.
 > Putting the diagnostic messaging
 pointer inside TT_Face is fairly
 >
 straight forward, and it isn't too hard to do it per
 TT_Face, which
 > is even better than per
 FT_Library.
 
 OK.
 
 > However
 > 
 > - diagnostics on
 parallel threads seem a bit of over-design - most
 >   people are unlikely ever to
 test multiple faces in parallel.
 >   Testing on single face is
 demanding enough - did I mention that it
 >   took about 5 days of CPU time
 to run the MS 2003 binary through
 >   the MS shipped fonts in win
 8.1?
 
 It's not about
 running the diagnostics in parallel but simply avoiding
 global variables in general in case you want to
 have it integrated
 into upstream FreeType. 
 Right now, your changes are very small and
 non-invasive, so I could easily add them.
 
 > - extracting the face handle and passing
 it back increases the
 >   complexity of the
 C<->C# interface. Before implementing the
 >   rasterization test, I have
 relied on SharpFont (
 >   https://github.com/Robmaister/SharpFont
 ) to handle the C<->C#
 >   interaction.  And I have
 enhanced SharpFont on the way as needed.
 >   The current change by-passes
 SharpFont.  I do not want to spend
 >   time increasing the
 complexity of what's really a temporary
 >   by-pass.
 
 Well, I can't comment on
 this.
 
 
  
   Werner
 
 -Inline Attachment Follows-
 

___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Graham Asher]

Please consider calling it ftdiagnostics.h, which is more explanatory. 
To my native-English-speaking mind, 'diag' looks as if it stands for 
'diagonal' or 'diagram'.


I think the enumeration values should be FT_DIAGNOSTIC_XXX too. 
Sometimes it's a good idea to spell things out.


- Graham

On 06/07/2016 07:59, Werner LEMBERG wrote:

Here is the updated patch for 2.6.4 - there are some minor
collisions with the new subpixel hinting mode.

Attached is something that would fit better into FreeType
w.r.t. formatting, macro definitions, and function names.  I suggest
to provide a public header file `ftdiag.h' that defines, similar to
`fterrdef.h', enumeration values `FT_DIAG_XXX' instead of the
currently used strings, together with proper declarations for
`TT_Diagnostics_{Unset,Set}' (which probably should use an `FT_'
prefix instead).


while updating the diff I looked into the global variable issue.
Putting the diagnostic messaging pointer inside TT_Face is fairly
straight forward, and it isn't too hard to do it per TT_Face, which
is even better than per FT_Library.

OK.


However

- diagnostics on parallel threads seem a bit of over-design - most
   people are unlikely ever to test multiple faces in parallel.
   Testing on single face is demanding enough - did I mention that it
   took about 5 days of CPU time to run the MS 2003 binary through
   the MS shipped fonts in win 8.1?

It's not about running the diagnostics in parallel but simply avoiding
global variables in general in case you want to have it integrated
into upstream FreeType.  Right now, your changes are very small and
non-invasive, so I could easily add them.


- extracting the face handle and passing it back increases the
   complexity of the C<->C# interface. Before implementing the
   rasterization test, I have relied on SharpFont (
   https://github.com/Robmaister/SharpFont ) to handle the C<->C#
   interaction.  And I have enhanced SharpFont on the way as needed.
   The current change by-passes SharpFont.  I do not want to spend
   time increasing the complexity of what's really a temporary
   by-pass.

Well, I can't comment on this.


 Werner


___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel


___
Freetype-devel mailing list
Freetype-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/freetype-devel

Re: [ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics

[Werner LEMBERG]

> Here is the updated patch for 2.6.4 - there are some minor
> collisions with the new subpixel hinting mode.

Attached is something that would fit better into FreeType
w.r.t. formatting, macro definitions, and function names.  I suggest
to provide a public header file `ftdiag.h' that defines, similar to
`fterrdef.h', enumeration values `FT_DIAG_XXX' instead of the
currently used strings, together with proper declarations for
`TT_Diagnostics_{Unset,Set}' (which probably should use an `FT_'
prefix instead).

> while updating the diff I looked into the global variable issue.
> Putting the diagnostic messaging pointer inside TT_Face is fairly
> straight forward, and it isn't too hard to do it per TT_Face, which
> is even better than per FT_Library.

OK.

> However
> 
> - diagnostics on parallel threads seem a bit of over-design - most
>   people are unlikely ever to test multiple faces in parallel.
>   Testing on single face is demanding enough - did I mention that it
>   took about 5 days of CPU time to run the MS 2003 binary through
>   the MS shipped fonts in win 8.1?

It's not about running the diagnostics in parallel but simply avoiding
global variables in general in case you want to have it integrated
into upstream FreeType.  Right now, your changes are very small and
non-invasive, so I could easily add them.

> - extracting the face handle and passing it back increases the
>   complexity of the C<->C# interface. Before implementing the
>   rasterization test, I have relied on SharpFont (
>   https://github.com/Robmaister/SharpFont ) to handle the C<->C#
>   interaction.  And I have enhanced SharpFont on the way as needed.
>   The current change by-passes SharpFont.  I do not want to spend
>   time increasing the complexity of what's really a temporary
>   by-pass.

Well, I can't comment on this.


Werner
diff --git a/src/truetype/ttinterp.c b/src/truetype/ttinterp.c
index 8fe83c5..e75bcfa 100644
--- a/src/truetype/ttinterp.c
+++ b/src/truetype/ttinterp.c
@@ -82,6 +82,19 @@
 #define BOUNDSL( x, n )  ( (FT_ULong)(x) >= (FT_ULong)(n) )
 
 
+  typedef int
+  (*diagnosticsFunc)( const char*message,
+  const char* const  opcode,
+  intrange_base,
+  intis_composite,
+  intIP,
+  intcallTop,
+  intopc,
+  intstart );
+
+  static diagnosticsFunc  diagnostics = NULL;
+
+
 #undef  SUCCESS
 #define SUCCESS  0
 
@@ -89,6 +102,47 @@
 #define FAILURE  1
 
 
+  FT_EXPORT_DEF( void )
+  TT_Diagnostics_Unset( void )
+  {
+diagnostics = NULL;
+  }
+
+
+  FT_EXPORT_DEF( void )
+  TT_Diagnostics_Set( diagnosticsFunc  funcptr )
+  {
+diagnostics = funcptr;
+  }
+
+
+#ifdef FT_DIAGNOSTICS
+#define DIAGNOSTICS( message, context )\
+  do   \
+  {\
+if ( diagnostics ) \
+  (*diagnostics)( message, \
+  opcode_name[(context)->opcode] + 2,  \
+  ( (context)->callTop \
+? (context)->callStack->Caller_Range   \
+: (context)->curRange ),   \
+  (context)->is_composite, \
+  (context)->IP,   \
+  (context)->callTop,  \
+  ( (context)->callTop \
+? ( (context)->callStack + \
+  (context)->callTop - 1 )->Def->opc   \
+: 0 ), \
+  ( (context)->callTop \
+? ( (context)->callStack + \
+  (context)->callTop - 1 )->Def->start \
+: 0 ) )\
+  } while ( 0 )
+#else
+#define DIAGNOSTICS( message, context )  do { } while ( 0 )
+#endif
+
+
   /*/
   /*   */
   /*CODERANGE FUNCTIONS*/
@@ -902,7 +956,7 @@
   };
 
 
-#ifdef FT_DEBUG_LEVEL_TRACE
+#if defined( FT_DEBUG_LEVEL_TRACE ) || defined( FT_DIAGNOSTICS )
 
   /* the first hex digit gives the length of the opcode name; the space */
   /* after the 

[ft-devel] updated patch for 2.6.4 Re: proposed patch for diagnostics (Re: implementing detection of hinting/rasterization errors in fonts)

[Hin-Tak Leung]

Here is the updated patch for 2.6.4 - there are some minor collisions with the 
new subpixel hinting  mode.

while updating the diff I looked into the global variable issue. Putting the 
diagnostic messaging pointer inside TT_Face is fairly straight forward, and it 
isn't too hard to do it per TT_Face, which is even better than per FT_Library. 
However

- diagnostics on parallel threads seem a bit of over-design - most people are 
unlikely ever to test multiple faces in parallel. Testing on single face is 
demanding enough - did I mention that it took about 5 days of CPU time to run 
the MS 2003 binary through the MS shipped fonts in win 8.1?

- extracting the face handle and passing it back increases the complexity of 
the C<->C# interface. Before implementing the rasterization test, I have relied 
on SharpFont ( https://github.com/Robmaister/SharpFont ) to handle the C<->C# 
interaction. And I have enhanced SharpFont on the way as needed. The current 
change by-passes SharpFont. I do not want to spend time increasing the 
complexity of what's really a temporary by-pass.

Anyway, you or others are welcomed to improve the patch. I'm going to start 
packaging after this e-mail, and hope to release the next Font Val (with a 
patched freetype 2.6.4 backend) in the next few days.


On Mon, 4/7/16, Behdad Esfahbod  wrote:

 global variable is not desirable.  Put it on the FT_Library
 at least.diff --git a/src/truetype/ttinterp.c b/src/truetype/ttinterp.c
index 2e3f9ef..534e30f 100644
--- a/src/truetype/ttinterp.c
+++ b/src/truetype/ttinterp.c
@@ -90,12 +90,29 @@
 #define BOUNDSL( x, n )  ( (FT_ULong)(x) >= (FT_ULong)(n) )
 
 
+typedef int (*diagnostics_Function)(const char* message, const char* const opcode, int range_base, int is_composite, int IP, int callTop, int opc, int start);
+
+static diagnostics_Function diagnostics = NULL;
+
 #undef  SUCCESS
 #define SUCCESS  0
 
 #undef  FAILURE
 #define FAILURE  1
 
+  FT_EXPORT_DEF( void )
+  TT_diagnostics_unset( void )
+  {
+diagnostics = NULL;
+  }
+
+  FT_EXPORT_DEF( void )
+  TT_diagnostics_set( diagnostics_Function funcptr )
+  {
+diagnostics = funcptr;
+  }
+
+#define DIAGNOSTICS(message, context) if (diagnostics) (*diagnostics)(message, opcode_name[(context)->opcode] + 2, ( (context)->callTop ? (context)->callStack->Caller_Range : (context)->curRange ), (context)->is_composite , (context)->IP, (context)->callTop, ((context)->callTop ? ((context)->callStack + (context)->callTop - 1)->Def->opc : 0), ((context)->callTop ? ((context)->callStack + (context)->callTop - 1)->Def->start : 0))
 
   /*/
   /*   */
@@ -910,7 +927,7 @@
   };
 
 
-#ifdef FT_DEBUG_LEVEL_TRACE
+#if defined(FT_DEBUG_LEVEL_TRACE) || defined(DIAGNOSTICS)
 
   /* the first hex digit gives the length of the opcode name; the space */
   /* after the digit is here just to increase readability of the source */
@@ -1664,17 +1681,19 @@
FT_F26Dot6  distance )
   {
 FT_F26Dot6  v;
+FT_Long F_dot_P = 0, moved_x = 0, moved_y = 0;
 
 
 v = exc->GS.freeVector.x;
 
 if ( v != 0 )
 {
+moved_x = FT_MulDiv( distance, v, exc->F_dot_P );
 #ifdef TT_SUPPORT_SUBPIXEL_HINTING_INFINALITY
   if ( SUBPIXEL_HINTING_INFINALITY&&
( !exc->ignore_x_mode||
  ( exc->sph_tweak_flags & SPH_TWEAK_ALLOW_X_DMOVE ) ) )
-zone->cur[point].x += FT_MulDiv( distance, v, exc->F_dot_P );
+zone->cur[point].x += moved_x;
   else
 #endif /* TT_SUPPORT_SUBPIXEL_HINTING_INFINALITY */
 
@@ -1683,12 +1702,12 @@
   /* diagonal moves, but only post-IUP.  DejaVu tries to adjust */
   /* diagonal stems like on `Z' and `z' post-IUP.   */
   if ( SUBPIXEL_HINTING_MINIMAL && !exc->backwards_compatibility )
-zone->cur[point].x += FT_MulDiv( distance, v, exc->F_dot_P );
+zone->cur[point].x += moved_x;
   else
 #endif
 
   if ( NO_SUBPIXEL_HINTING )
-zone->cur[point].x += FT_MulDiv( distance, v, exc->F_dot_P );
+zone->cur[point].x += moved_x;
 
   zone->tags[point] |= FT_CURVE_TAG_TOUCH_X;
 }
@@ -1697,16 +1716,27 @@
 
 if ( v != 0 )
 {
+  moved_y = FT_MulDiv( distance, v, exc->F_dot_P );
 #ifdef TT_SUPPORT_SUBPIXEL_HINTING_MINIMAL
   if ( !( SUBPIXEL_HINTING_MINIMAL &&
   exc->backwards_compatibility &&
   exc->iupx_called &&
   exc->iupy_called ) )
 #endif
-zone->cur[point].y += FT_MulDiv( distance, v, exc->F_dot_P );
+zone->cur[point].y += moved_y;
 
   zone->tags[point] |= FT_CURVE_TAG_TOUCH_Y;
 }
+
+F_dot_P =
+  ( (FT_Long)exc->GS.projVector.x * exc->GS.freeVector.x +
+