144669 - Code subsetted Postscript Font Code

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[kill this account]

Code that actually generates the core Type11 tables and the incremental glyphs.

Comment 1

[kill this account]

In bug 144669 I suggest that we continue to use to use Type11 but do not 
incrementally load the glyphs.

Comment 2

[kill this account]

The current plan is to use subsetted Type42 instead of incrementally loaded 
Type11 (see bug 144668) so changed title from

  Code Type11 Specific Font Code

to

  Code subsetted Type42 Specific Font Code

Comment 3

[Pete Zha]

Need to decide what kind of ps font type we want to use

Comment 4

[Pete Zha]

Seems we decide to subset font in first working version, so this bug could
include in bug 144668

Comment 5

[kill this account]

You are right that we are reevaluating what Postscript font type to use (Type 1, 
Type3, Type42, Type8, Type9, or Type11). As such we should probably change the 
title to not specify the font type.

However, regardless of which type we decide to use, the code can be checked in
separately.

For the purpose of breaking the task into smaller parts to make review easier,
I'd like to keep this bug open.

Comment 6

[kill this account]

Reassigning to myself. The code should be ready soon.

Comment 7

[kill this account]

Attachment: patch; has routines to generate Type 8

Comment 8

[kill this account]

this patch is requires the patch in bug 180473

Comment 9

[Louie Zhao]

Comment on attachment 107323 [details] [diff] [review]
patch; has routines to generate Type 8

>+
>+FT_Error FT2GlyphToType1CharString(nsIFreeType2 *aFt2, FT_Face aFace,
>+                                   PRUint32 aGlyphID, int aWmode, int aLenIV,
>+                                   unsigned char *aBuf);
>+
>+#endif /* TYPE1_H */

In many functions, there is argument -- nsIFreeType2*, that means the invoker
will be responsible for getting nsIFreeType2. Can we get rid of this argument
and get the interface internally.
Actually, not all functions need this changing. But the main entry
"FT2SubsetToType8" need get nsIFreeType2 itself, and can pass it to other
function like "FT2GlyphToType1CharString".

>+
>+
>+static char *
>+FT2ToType1FontName(nsIFreeType2 *aFt2, FT_Face aFace, int aWmode)
>+{
>+  char *fontname;
>+  int name_len;

This function doesn't use nsIFreeType2. It's superfluous.

>+char *
>+FT2ToType8CidFontName(nsIFreeType2 *aFt2, FT_Face aFace, int aWmode)
>+{

This function should be exposed and defined in nsType8.h because when
generating ps body, we need know the actually Type8 font name (bstell, in ps
body, we need use the name generated by this function, right?).  

>+
>+static int
>+FT2SubsetToCidKeyedType1(nsIFreeType2 *aFt2, FT_Face aFace, PRUnichar *aCharIDs,
>+                         int aLen, const char *aFontName,
>+                         const char *aRegistry, const char *aEncoding,
>+                         int aSupplement, int aWmode, int aLenIV, FILE *aFile)
>+{
>+  int i, charstring_len, charstrings_len, data_offset;
>+  PRUint32 glyphID, data_len, data_len2, max_charstring;
>+  unsigned int cmapinfo_buf[DEFAULT_CMAP_SIZE+1];
>+  unsigned int *cmapinfo = cmapinfo_buf;
>+  unsigned char charstring_buf[1024];
>+  unsigned char *charstring = charstring_buf;
>+  int fd_len, gd_len, cidmap_len, cmap_array_len, num_charstrings;
>+  FT_UShort upm = aFace->units_per_EM;
>+  nsCOMPtr<nsIFreeType2> mFt2;
>+

function return PR_TRUE and PR_FALSE, so the PR_Bool may be more suitable for
the type of return value, although it's "int" indeed.

mFt2 is never used. we can delete it.

Comment 10

[kill this account]

Thanks for the comments! There is only one item I need to discuss.

>+FT_Error FT2GlyphToType1CharString(nsIFreeType2 *aFt2, FT_Face aFace,
> ...
> In many functions, there is argument -- nsIFreeType2*, that means the 
> invoker will be responsible for getting nsIFreeType2. Can we get rid of 
> this argument and get the interface internally.

The only routines really exposed are FT2SubsetToType8 and FT2ToType8CidFontName. 
If you would like I'd be happy to take it out of the these. Inside of this body 
of code I don't see an advantage to calling do_GetService versus passing it as a 
parameter. In general I'd prefer pass it as a parameter rather than have to 
initialize a nsCOMPtr and call do_GetService in each subroutine.

Comment 11

[Louie Zhao]

actually, not need "initialize a nsCOMPtr and call do_GetService in each
subroutine", that is really unefficient. "FT2SubsetToType8" can do_GetService
and pass it to any other function. 

Comment 12

[kill this account]

Attachment: patch with changes requested in review

note: this patch is requires the patch in bug 180473

Comment 13

[kill this account]

Attachment: diff of the previous and current patch

Comment 14

[kill this account]

Attachment: patch with minor fix to return status of FT2SubsetToType8

Comment 15

[kill this account]

Attachment: diff to previous patch

Comment 16

[kill this account]

Attachment: patch; output hexascii data instead of binary data

changed output to hexascii glyph data since printers seen to have issues with 
binary glyph data

Comment 17

[kill this account]

Attachment: diff from attachment 107323

Louie: to make re-reviewing easier this is a diff to the 1st attachment 107323 [details] [diff] [review]

Comment 18

[kill this account]

Attachment: patch de-rotted to match bug 180473 and code to detect printer postscript version and tell user what to do

de-rotted to match changes to bug 180473
and added routine to detect printer's postscript level < 2015 and tell user
what
to do

Comment 19

[kill this account]

Attachment: diff between attachment 107721 and attachment 108168

shows the changes since Louie's review

Comment 20

[Pete Zha]

Comment on attachment 108168 [details] [diff] [review]
patch de-rotted to match bug 180473 and code to detect printer postscript version and tell user what to do

1)
+CidCharMap(PRUnichar *aCharIDs, PRUint32 *aCIDs, 
+	      int aLen, FILE *aFile)

+void
+CmapHeader(const char *aName, const char *aRegistry, 
+	     const char *aEncoding, int aSupplement,
+	     int aType, int aWmode, FILE *aFile)

Please fix the above two indent problems

2)
+    ftinfo->len += ECSI(&ftinfo->buf, (int)(x3-x2));
+    ftinfo->len += CSC(&ftinfo->buf, T1_VHCURVETO);
+  } else {
+    ftinfo->len += ECSI(&ftinfo->buf,	(int)(x1-cur_x));
+    ftinfo->len += ECSI(&ftinfo->buf,	(int)(y1-cur_y));
Should use the same "if else" style as other place in this file:
if () {
}
else{
}

3)
+  //
+  for (PRUint32 i=0; i<aNumChars; i++) {
+    CIDs[i] = i+1;
+  }

I think this will not be compiled on HP-UX since the "i" be init after goto and
in for sentence.

4) Many place, evaluate sentence and comparison sentence are not using same
style. for example:
We should always use a = b or a=b and a==b or a == b in same file. I prefer to
use the first style which has a white space between variable and operators.

Anyway, the last one is my personal habits. So, r=[e-mail] if your fix
1) 2) and 3)

Comment 21

[Pete Zha]

Change to 1.3b since 1.3a already freezed

Comment 22

[kill this account]

Attachment: patch with req changes

Comment 23

[kill this account]

Attachment: for reference: diff between attachment 108168 and attachment 108331

Comment 24

[kill this account]

Comment on attachment 108331 [details] [diff] [review]
patch with req changes

the changes were minor so carrying forward Pete's (and Louies's) r=

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 108331 [details] [diff] [review]
patch with req changes

I didn't really find anything wrong with this code, but I did come up with some
comments, mostly just style nits...

The new files in this patch seem to be missing the standard emcas Mode line
found in most (all?) Mozilla code, it should be (on the first line in the
file):

/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */


- In nsCidMap.cpp:

+void
+CidCharMap(PRUnichar *aCharIDs, PRUint32 *aCIDs, 
+	    int aLen, FILE *aFile)

+void
+CidRangeMapUnicode(FILE *aFile)

+void
+CmapHeader(const char *aName, const char *aRegistry, 
+	    const char *aEncoding, int aSupplement,
+	    int aType, int aWmode, FILE *aFile)

...

All those functions take a FILE* and write stuff out to that file, IMO the
names of those functions should reflect that fact, for readability's sake. I.e.
WriteCidCharMap(), WriteCidRangeMapUnicode(), and so on...

- In nsType1.cpp:

+static PRUint16 c1 = TYPE1_ENCRYPTION_C1;
+static PRUint16 c2 = TYPE1_ENCRYPTION_C2;

Shouldn't these at the very least be const? And global static's named 'c1' and
'c2' seem like a bad idea to me. Why do we need these in the first place? Can't
users simply use the #defines?

Maybe I'm missing something obvious here, but what's this "encryption" about?
What do we need that for?

+/* define convenient short names for these functions */
+#define ECSI Type1EncodeCharStringInt
+#define CSC  Type1CharStringCommand

This is IMO not cool. If you want more convenient names, then why not name the
methods something more convenient in the first place? Or create an inline
thunk, don't #define unless you really must...

+static unsigned char
+Type1Encrypt(unsigned char aPlain, PRUint16 *aKeyPtr)
+{
+  unsigned char cipher;
+  cipher = (aPlain ^ (*aKeyPtr >> 8));
+  *aKeyPtr = (cipher + *aKeyPtr) * c1 + c2;
+  return cipher;
+}
+
+static void
+Type1EncryptString(unsigned char *aInBuf, unsigned char *aOutBuf, int aLen)
+{
+  int i;
+  PRUint16 key = TYPE1_ENCRYPTION_KEY;
+
+  for (i=0; i<aLen; i++)
+    aOutBuf[i] = Type1Encrypt(aInBuf[i], &key);
+}

Seems to me that Type1Encrypt() really should be inline, a trivial ammount of
code called in one place only, and in a place that looks pretty performance
critical to me.

- In sideWidthAndBearing():

+  int aw=0, ah=0;
+  FT_UShort upm = aFti->face->units_per_EM;

For consistency, add spaces around '=' on the first line there.

And shouldn't the function sideWidthAndBearing() return a PRBool and not an
int?

- In conicto():

+  cur_x = ftinfo->cur_x;
+  cur_y = ftinfo->cur_y;
+  ctl_x  = toCS(upm, aControlPt->x);
+  ctl_y  = toCS(upm, aControlPt->y);

Extra spaces before '='...

+static FT_Outline_Funcs ft_outline_funcs = {
+	 moveto,
+	 lineto,
...

Stick to 2-space indentation.

- In nsType1.h:

+#define toCS(upm,x)   ((int)(((x)*1000.0)/(upm)))
+#define fromCS(upm,x) ((int)(((double)x)*((double)upm)/1000.0))

How about making these inline functions in stead of macro's? And add
space-after-comma in the argument list. If you don't want to make them
inline's, then at least follow common naming convention for macro's and make
the names all UPPER case.

- In FT2SubsetToEncoding():

+	 rawDigest = (unsigned char*)PR_MALLOC(
+					   nsISignatureVerifier::SHA1_LENGTH);

In most places you use PR_Malloc(), here (more than one place) you use
PR_MALLOC(), pick one and stick with it.

+	 if (rawDigest) {
+	   rv = verifier->HashEnd(id, &rawDigest, &len,
+				 nsISignatureVerifier::SHA1_LENGTH);

Indentation off-by-one.

- In FT2SubsetToType8():

+  char *fontname=NULL;
+  char *cmapname=NULL;
+  char *cidfontname=NULL;
+  char *registry;
+  char *encoding = NULL;

Add spaces around '=' where missing.

- In FT2SubsetToCidKeyedType1():

+      buf[l++] = (data_offset>> 16) & 0xFF;
+      buf[l++] = (data_offset>> 8) & 0xFF;

If you add a space on one side of an operator, add it on the other side too!

- In FT2ToType1FontName():

+  char *fontname;
...
+  spaces_to_underlines(fontname, (char *)fontname);

No need to cast fontname to what it already is.

- In spaces_to_underlines()

+  for (; *aFromName; aToName++, aFromName++) {
+    if (*aFromName == ' ')
+      *aToName = '_';
+    else
+      *aToName = *aFromName;

Wouldn't it be worth branching this code off if aFromName == aToName? In that
case it's pointless to assign *aToName = *aFromName if *aFromName != ' '. IOW,
how about this in stead:

static void
spaces_to_underlines(const char *aFromName, char *aToName)
{
  if (aFromNAme == aToName) {
    for (; *aFromName; aToName++, aFromName++) {
      if (*aFromName == ' ')
	*aToName = '_';
    }
  } else {
    for (; *aFromName; aToName++, aFromName++) {
      if (*aFromName == ' ')
	*aToName = '_';
      else
	*aToName = *aFromName;
    }
    *aToName = *aFromName;
  }
}

Then again, I don't see this function ever being called with aFromName !=
aToName, so maybe one argument is enough here?

Other than that, sr=jst

Comment 26

[kill this account]

>+static PRUint16 c1 = TYPE1_ENCRYPTION_C1;
>+static PRUint16 c2 = TYPE1_ENCRYPTION_C2;
>
> Why do we need these in the first place? Can't users simply use the #defines?

I'm just being very careful to implement the "encryption" exactly as defined by
adobe.

> Maybe I'm missing something obvious here, but what's this "encryption" about?

Adobe in an attempt to protect the intellectual rights obfuscates the glyph 
data. As encryption goes this is very poor quality encryption.

> What do we need that for?

This is the format that the Postscript interpreter expects.

>+/* define convenient short names for these functions */
>+#define ECSI Type1EncodeCharStringInt
>+#define CSC  Type1CharStringCommand
>
> This is IMO not cool.

Could you explain why?

> If you want more convenient names, then why not name the methods something 
> more convenient in the first place?

Would ecsi and csc be acceptable function names? These get used alot in a place
where other items are interesting and these functions are not particularly 
interesting; eg:

    cubicto(...)
    {
      ...  
      /* horizontal to vertical curve */
      if (((int)(y1) == (int)(cur_y)) && ((int)(x3) == (int)(x2))) {
        ftinfo->len += ECSI(&ftinfo->buf, (int)(x1-cur_x));
        ftinfo->len += ECSI(&ftinfo->buf, (int)(x2-x1));
        ftinfo->len += ECSI(&ftinfo->buf, (int)(y2-y1));
        ftinfo->len += ECSI(&ftinfo->buf, (int)(y3-y2));
        ftinfo->len += CSC(&ftinfo->buf, T1_HVCURVETO);
      }
      /* vertical to horizontal curve */
      else if (((int)(x1) == (int)(cur_x)) && ((int)(y3) == (int)(y2))) {
        ftinfo->len += ECSI(&ftinfo->buf, (int)(y1-cur_y));
        ftinfo->len += ECSI(&ftinfo->buf, (int)(x2-x1));
        ftinfo->len += ECSI(&ftinfo->buf, (int)(y2-y1));
        ftinfo->len += ECSI(&ftinfo->buf, (int)(x3-x2));
        ftinfo->len += CSC(&ftinfo->buf, T1_VHCURVETO);
      }
      else {
        ftinfo->len += ECSI(&ftinfo->buf,  (int)(x1-cur_x));
        ftinfo->len += ECSI(&ftinfo->buf,  (int)(y1-cur_y));
        ftinfo->len += ECSI(&ftinfo->buf,  (int)(x2-x1));
        ftinfo->len += ECSI(&ftinfo->buf,  (int)(y2-y1));
        ftinfo->len += ECSI(&ftinfo->buf,  (int)(x3-x2));
        ftinfo->len += ECSI(&ftinfo->buf,  (int)(y3-y2));
        ftinfo->len += CSC(&ftinfo->buf, T1_RRCURVETO);
      }
      ...
    }

but if you want I can use the long function names.

> Or create an inline thunk

What is an "inline thunk"?

For the rest of the items I'll make the requested changes.

Comment 27

[kill this account]

checked in last night