127063 - add special converters for MathML TrueType fonts

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[kill this account]

The MathML TrueType fonts use a custom "Unicode" cmap. The Linux TrueType
font code does not handle this. When the cmex10/cmsy10 TrueType fonts
are found the font search code spends so *long* searching for the glyphs
that it appears to be in an infinite loop.

To add support for TrueType MathML fonts we need to:

 add fontEncoding.properties to the Linux distribution

 add code to detect the MathML fonts

 add code to instanciate the needed converter

 add code to get the CCMap from the converter instead of the font's cmap

 add a subclass to the linux TrueType code to call the converter in the 
 measure/draw code

Comment 1

[kill this account]

*** Bug 126465 has been marked as a duplicate of this bug. ***

Comment 2

[Frank Tang]

it is very very slow but not hang. But I think we need to fix this. 
->nsbeta1

Comment 3

[kill this account]

the "hang" issue is addressed in bug 127283

Comment 4

[kill this account]

status report: 
I believe I am beginning to see MathML using TrueType.
I hope to have a patch late today or early tomorrow. 
Currently I have about -80 lines and +600 lines.

Comment 5

[kill this account]

I'm seeing MathML displayed. There are still some issues but I am seeing
reasonable (not perfect) display.

Comment 6

[rbs]

Got a preliminary screenshot? The requirements/precision involved in the
rendering of MathML are somewhat overkill.

Comment 7

[kill this account]

Attachment: prelim screen shot

This screen shot shows basic functioning.
I have not worked on tuning the metrics yet.

Comment 8

[kill this account]

Attachment: patch is rough form

this patch show basic functioning but it still needs some work

Comment 9

[rbs]

To cheer you up... I have already had my hard-disk replaced twice. The numerous
compilations that it takes to get these MathML alignment & spacing right took a
toll on them (fortunately there were still under warranty).

Comment 10

[kill this account]

Attachment: working MathML

I believe this is a usable verion. 

known bugs:

Fails to draw the 'less than or equal' char, 0xB7, in the cmsy10 font.
Interestingly, the windows client does draw it but the moz 
web page screenshot shows it, 0xB7, missing.
http://www.mozilla.org/projects/mathml/fonts/encoding/cmsy-ttf.gif

Layout is not perfect. There is a gap showing when certian chars are stacked
vertically.

The drawing code is complaining that it is unable to get the background
when the text area is partially showing (sub_image). It appears to be
drawing okay so this does not seem critical but should be fixed.

Given another week I can address these.

Roger: would you kindly review this?

Comment 11

[kill this account]

Attachment: added pref for library name

Roger: would you kindly review this?

Comment 12

[kill this account]

Attachment: changed string handling per jag's suggestion

I talked to jag about the string handling

Comment 13

[rbs]

Comment on attachment 71581 [details] [diff] [review]
changed string handling per jag's suggestion

======================
+PRBool
+nsFT2FontCatalog::UsesCustomEncoder(const char *aFamilyName)

-> HasCustomEncoder()

+  if (ffei)
+    return PR_TRUE;
+  else
+    return PR_FALSE;

-> return ffei != nsnull;
   (But I noted that nobody is using this UsesCustomEncoder() anyway.
    It might be a remnant as you were coding and trying things out, and
    might go away if not needed anymore)

======================
+nsTTFontEncoderInfo FEI_Adobe_Symbol_Encoding = {
+  // I do not have this font so I have not checked the PlatformID/Encoding

-> This is the standard Symbol font which you can get from any windows box
  (same for wingdings & webdings which are mapped to windows_1252)

+  "Adobe-Symbol-Encoding", 1, 0, nsnull

-> Need symbolic constants for these '1', '0', '3', ...

========================
+typedef struct nsTTFontEncoderInfo {
+  const char		  *mConverterName;
+  PRUint8		   mCmapPlatformID;
+  PRUint8		   mCmapEncoding;
+  nsIUnicodeEncoder*	   mConverter;
+} nsTTFontEncoderInfo;

-> duplicate /nsTTFontEncoderInfo/

=========================
+#define FCE_FLAGS_UNICODE 1
+#define FCE_FLAGS_SYMBOL  2

-> 0x01, 0x02 (so that a reader can think twice and see that the 
   next isn't the numeral '3')

=================
NS_ASSERTION(0, "...");
-> NS_ERROR("...")

=========================
+  // in this hash each ttc face has a unique key
+  nsCAutoString faceTag(nsFT2FontCatalog::GetFamilyName(aFce));
   nsCStringKey key(faceTag);

-> nsCStringKey key(nsFT2FontCatalog::GetFamilyName(aFce));
   to avoid multiple string copying

Due to the time difference, I may not follow up -- but the patch looks okay and
includes some other niceties like pref'ing the libname.

I have updated cmsy-ttf.gif with a version that shows up the less-or-equal
glyph. 

The patch didn't tune the metrics (math people will complaint about that --
they are very regardful about such things) but even a long journey starts with
a single step.

So r=rbs applies from now on.

Comment 14

[kill this account]

Attachment: changes for rbs

> +nsFT2FontCatalog::UsesCustomEncoder(const char *aFamilyName)

Removed

> -> Need symbolic constants for these '1', '0', '3', ...

Used FreeType2 defines

> +typedef struct nsTTFontEncoderInfo {
...
> +} nsTTFontEncoderInfo;
>
> -> duplicate /nsTTFontEncoderInfo/

Removed struct tag

> +#define FCE_FLAGS_UNICODE 1
> +#define FCE_FLAGS_SYMBOL  2
>
> -> 0x01, 0x02 (so that a reader can think twice and see that the
>    next isn't the numeral '3')

Done

> NS_ASSERTION(0, "...");
> -> NS_ERROR("...")

Thanks, fixed

> +  nsCAutoString faceTag(nsFT2FontCatalog::GetFamilyName(aFce));
>    nsCStringKey key(faceTag);
>
> -> nsCStringKey key(nsFT2FontCatalog::GetFamilyName(aFce));
>    to avoid multiple string copying

Thanks. fixed

> I have updated cmsy-ttf.gif with a version that shows up the less-or-equal
> glyph.

Thanks. I'll check it out.

> The patch didn't tune the metrics (math people will complaint about that --
> they are very regardful about such things) but even a long journey starts
with
> a single step.

If we had more time I'd like to fix this. I've opened bug 128000 to address
this.

> So r=rbs applies from now on.

Thanks

Comment 15

[kill this account]

Comment on attachment 71660 [details] [diff] [review]
changes for rbs

marking it reviewed per rbs' comment

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 71660 [details] [diff] [review]
changes for rbs

>+  if (gFreeType2SharedLibraryName) {
>+    free(gFreeType2SharedLibraryName);
>+    gFreeType2SharedLibraryName = nsnull;
>+  }

I remember when it was safe to pass NULL to free(3).  Ah, the good old days.

>+  rv = gPref->GetCharPref("font.freetype2.shared-library", 
>+                          &gFreeType2SharedLibraryName);
>+  if (NS_FAILED(rv)) {
>+    enable_freetype2 = PR_FALSE;
>+    FREETYPE_FONT_PRINTF((
>+                   "gFreeType2SharedLibraryName missing, FreeType2 disabled"));
>+    gFreeType2SharedLibraryName = nsnull;
>+  }
>+  else if (!gFreeType2SharedLibraryName) {
>+    FreeGlobals();
>+    return NS_ERROR_OUT_OF_MEMORY;
>   }

Will the pref service really return a success code and a NULL out param to
indicate OOM?  Doesn't look that way from a quick scan of nsPrefBranch.cpp.

>+nsTTFontEncoderInfo FEI_Adobe_Symbol_Encoding = {
>+  // I do not have this font so I have not checked the PlatformID/Encoding
>+  "Adobe-Symbol-Encoding", TT_PLATFORM_MACINTOSH, TT_MAC_ID_ROMAN, nsnull
>+};
>+nsTTFontEncoderInfo FEI_x_ttf_cmr = {
>+  // I do not have this font so I have not checked the PlatformID/Encoding
>+  "x-ttf-cmr", TT_PLATFORM_MACINTOSH, TT_MAC_ID_ROMAN, nsnull
>+};
>+nsTTFontEncoderInfo FEI_x_ttf_cmmi = {
>+  // I do not have this font so I have not checked the PlatformID/Encoding
>+  "x-ttf-cmmi", TT_PLATFORM_MACINTOSH, TT_MAC_ID_ROMAN, nsnull
>+};

School me: what are the effects of having bogus PlatformID/Encoding? Can you
get someone who has the fonts to tell you the correct values?

> +    nsCOMPtr<nsIAtom> charset;
> +    charset = getter_AddRefs(NS_NewAtom(fei->mConverterName));

Prefer initialization over assignment:

nsCOMPtr<nsIAtom> charset(dont_AddRef(NS_NewAtom(fei->mConverterName)));

>+  for (i=0; gFontFamilyEncoderInfo[i].mFamilyName; i++) {
>+    nsTTFontFamilyEncoderInfo *ffei = &gFontFamilyEncoderInfo[i];
>+    nsTTFontEncoderInfo *fei = ffei->mEncodingInfo;
>+    NS_IF_RELEASE(fei->mConverter);

Use nsCOMPtr for fei->mConverter?

>+  nsServiceManager::GetService(kPrefCID, NS_GET_IID(nsIPref),
>+                                 (nsISupports**) &mPref);
>+  if (!mPref)
>+    goto cleanup_and_return;

nsCOMPtr<nsIPref> mPref;
mPref = do_GetService(NS_PREF_CONTRACTID);

?  Better to use the contract ID, and always better to use nsCOMPtr where
possible.

>   strncpy((char*)fce->mVendorID, (char*)tt_os2->achVendID, 4);

(Do you assure null termination for the strncpy'd string?)

> typedef struct {
>   const char   *mFontFileName;
>   time_t        mMTime;
>   PRBool        mIsValid;
>+  PRUint32      mFlags;
>   const char   *mFontType;
>   int           mFaceIndex;
>   int           mNumFaces;

Do we have a lot of these structs around?  Would it be worthwhile to
make mIsValid be a bit in mFlags, and save a word per struct?

>   nsHashtable   *mFreeTypeNodes;
>+  nsHashtable   *mFontFamilies;

Mmm, nsHashtable.  Have you considered using PLDHash or PLHash for these,
for speed and space wins?

>   // since the library may not be available on any given system
>   // failing to load is not considered a fatal error
>   if (!sSharedLib) {
>-    NS_ASSERTION(0, "freetype library not found");
>+    NS_ERROR("freetype library not found");
>     return PR_FALSE;

NS_WARNING, for non-fatal diagnostics?

>@@ -449,7 +470,7 @@
>   for (p=FtFuncs; p->FuncName; p++) {
>     func = PR_FindFunctionSymbol(sSharedLib, p->FuncName);
>     if (!func) {
>-      NS_ASSERTION(0, "nsFreeType::LoadSharedLib Error");
>+      NS_ERROR("nsFreeType::LoadSharedLib Error");

Likewise.

>+gint
>+nsFreeTypeXImageSBC::GetWidth(const PRUnichar* aString, PRUint32 aLength)
>+{
>+  nsresult res;
>+  char buf[512];
>+  PRInt32 bufLen = sizeof(buf);
>+  PRInt32 stringLen = aLength;
>+  nsFontCatalogEntry* fce = mFaceID->GetFce();
>+  nsTTFontFamilyEncoderInfo *ffei = nsFT2FontCatalog::GetCustomEncoderInfo(fce);
>+  NS_ASSERTION(ffei,"failed to find font encoder info");
>+  if (!ffei)
>+    return NS_ERROR_FAILURE;
>+  res = ffei->mEncodingInfo->mConverter->Convert(aString, &stringLen, 
>+                                                 buf, &bufLen);
>+  NS_ASSERTION((aLength&&bufLen)||(!aLength&&!bufLen), "converter failed");
>+
>+  //
>+  // Widen to 16 bit
>+  //
>+  PRUnichar unibuf[512];
>+  int i;
>+  for (i=0; i<bufLen; i++) {
>+    unibuf[i] = (unsigned char)buf[i];
>+  }

Can GetWidth and GetBoundingMetrics and DrawString share some of this 
copy-and-pasted conversion code, via a helper?

Comment 17

[kill this account]

Attachment: various changes

> >+  if (NS_FAILED(rv)) {
...
> >+  }
> >+  else if (!gFreeType2SharedLibraryName) {

deleted unnecessary code

> >+nsTTFontEncoderInfo FEI_x_ttf_cmr = {
> >+  // I do not have this font so I have not checked the PlatformID/Encoding
> >+  "x-ttf-cmr", TT_PLATFORM_MACINTOSH, TT_MAC_ID_ROMAN, nsnull
> >+};
...
> what are the effects of having bogus PlatformID/Encoding?

blank or wrong glyphs but no crash

> Can you get someone who has the fonts to tell you the correct values?

I found the fonts and edited the table as best I understand it.
Surprisingly, neither rbs or erik know which table should be used.
It appears to just work on windows (I think this lack of knowledge is scary).

> Prefer initialization over assignment:
> nsCOMPtr<nsIAtom> charset(dont_AddRef(NS_NewAtom(fei->mConverterName)));

Done

> >+	NS_IF_RELEASE(fei->mConverter);
>
> Use nsCOMPtr for fei->mConverter?

These are in a static table that maps charset names to converter
names. The rest of the GTK code does not do this so it might work
but it might not. Because Brendan wants this in for 0.9.9 I'm on a
fairly tight deadline. This is one of several items I would like to
defer this to bug 128000 whick I have already opened for future 
TrueType MathML work.

> nsCOMPtr<nsIPref> mPref;
> mPref = do_GetService(NS_PREF_CONTRACTID);

Done

> >   strncpy((char*)fce->mVendorID, (char*)tt_os2->achVendID, 4);
> (Do you assure null termination for the strncpy'd string?)

added memset to clear the array (it is 5 bytes long)

> >   PRBool	    mIsValid;
> >+  PRUint32	    mFlags;
> Would it be worthwhile to make mIsValid be a bit in mFlags,

Done

> Mmm, nsHashtable.  Have you considered using PLDHash or PLHash for these,
> for speed and space wins?

I would like to defer this to bug 128000 whick I opened for future work in this

area.

> NS_WARNING, for non-fatal diagnostics?

changed

> >+gint
> >+nsFreeTypeXImageSBC::GetWidth(const PRUnichar* aString, PRUint32 aLength)
> >+{
...
> Can GetWidth and GetBoundingMetrics and DrawString share some of this
> copy-and-pasted conversion code, via a helper?

This needs to also handle strings larger that the fixed stack buffer. I did
this for the nsFontMetricsGTK code and others ported it to the other
X FEs. Again since I am on a deadline I would like to defer this to bug
128000.

Comment 18

[kill this account]

Comment on attachment 71779 [details] [diff] [review]
various changes

marking has review per rbs' comment

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 71779 [details] [diff] [review]
various changes

How can I argue with the round-number righteousness of 128000?	Make sure you
copy all these to-do items into there, though!

sr=shaver, thanks for the quick response to review.

Comment 20

[kill this account]

I'll add them this evening

Comment 21

[Daniel (Leaf) Nunes]

Comment on attachment 71779 [details] [diff] [review]
various changes

a=leaf on behalf of drivers, our math professors will be so proud.

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 71779 [details] [diff] [review]
various changes

a=shaver for 0.9.9.

Comment 23

[kill this account]

checked in

Comment 24

[Jim Dunn]

Ok, I am going to probably "upset" people by reopening this... sorry.
My only issue is with the unix.js change:
+pref("font.freetype2.shared-library", "libfreetype.so.6");

HP's default extension is sl... so on HP this would be libfreetype.sl
It is also "hardcoded" to version .6, which I think is a bad idea...
(the version of freetype on my hp box is 8... so I have a libfreetype.sl.8)

SO my question is there ANY way to put an ifdef in a unix.js file
ifdef hpux
+pref("font.freetype2.shared-library", "libfreetype.sl");
else
+pref("font.freetype2.shared-library", "libfreetype.so.6");
endif

Otherwise the solution as it is, ONLY works for for a limited case.
I will post either today or tomorrow morning... I am willing to go
along with unix.js, IFF we have a fallback in case we can't load this
something like PR_LoadLibrary("libfreetype" MOZ_DLL_SUFFIX); as I have
mentioned before... 

diff shortly

Comment 25

[rbs]

Or alternatively, have both
pref("font.freetype2.shared-library.hpux", "libfreetype.sl");
pref("font.freetype2.shared-library", "libfreetype.so.6");
and do the #ifdef from the back-end side.

Comment 26

[kill this account]

Jim: I think the library name issue belongs in bug 127553.

Could we address it there and close this bug?

Comment 27

[kill this account]

Having not heard anything in a couple of days I am re-closing this bug.

Please reopen if appropiate.