116153 - review nsFontMetrics changes for FreeType checkin

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[kill this account]

this is to help manage the checkin of the nsFontMetrics changes for the
FreeType2 code

Comment 1

[kill this account]

Attachment: patch; changes to nsFontMetrics.cpp/h

Comment 2

[kill this account]

Attachment: font prefs dialog showing truetype font

The TrueType fonts start with uppercase.

Comment 3

[kill this account]

Attachment: TrueType display (aasb disabled)

http://home.netscape.com/ja with HG-GothicB selected for Japanese

Comment 4

[Masaki Katakai]

Attachment: TrueType DIsplay on Solaris with Solaris TrueType fonts

Comment 5

[Masaki Katakai]

I have succeeded to build binary with patch and freetype2
version=8.0.2 on Solaris environment. There are some problems,

- Underline's characters are display as RED color. When I
back to normal fonts, these are displayed as BLACK.

- HG-GothicB.ttf and HG-MinchoL.ttf can not be used for 2bytes
  japanese fonts (already reported to Brian)

- I simply tried purify. I'll attach the outputs around gfx
  codes. But have't investigated yet.

- I tried the cybercjk.ttf TryeType downloaded from Netsape
  site. But it seems that the fonts always loaded, checked
  af gfx init. Why? HG-Gothic fonts and other ascii TrueType
  fonts seem to be changed but only cybercjk.ttf is always
  being checked at gfx init.

  

Comment 6

[Masaki Katakai]

Attachment: purify outputs

Comment 7

[Masaki Katakai]

For cybercjk.ttf problem, it seems that some invalid glyhs
are contained and it may cause the problem??

Mozilla reports as below,

font /export/home1/Mozilla/build/ft2/mozilla/dist/bin/a/cybercjk.ttf           
30275 glyphs (2790 invalid)

Comment 8

[kill this account]

****  Purify instrumented ...:
  UMR: Uninitialized memory read ...
  * This is occurring while in:
        nsBlendMonoImage0888(...) [nsX11AlphaBlend.cpp:505]
        ...
  * Reading 4 bytes from 0xffbebdf0 on the stack.
  * Address 0xffbebdf0 is local variable "dst_pixel" in function 
      nsBlendMonoImage0888(...).

I've seen this UMR for "dst_pixel" and honestly it baffles me as dst_pixel
was just set a few lines back.

Comment 9

[kill this account]

I will debug why the underlined text is red.

Invalid glyphs are normal and should not cause a font to be re-scanned.

I will need to work with you on these issues.

Comment 10

[Masaki Katakai]

I could compile the binary on RH6.2 and as you already know,
red text with underline does not happen on Linux.

> - Underline's characters are display as RED color. When I
> back to normal fonts, these are displayed as BLACK.

Comment 11

[Masaki Katakai]

I found re-scanned issue also happens on only Solaris.
I'm not seeing on Linux.

I'm using the same freetype2 (0.8.2).

Comment 12

[kill this account]

The color is picked up in here:

http://lxr.mozilla.org/seamonkey/source/gfx/src/x11shared/nsXFontAAScaledBitmap.
cpp#193

  193   nscolor color = nsX11AlphaBlend::PixelToNSColor(values.foreground);

The drawing code is only given the foreground value which I have to convert 
to RGB.  At initialization the code selects the converter specific to the 
frame buffer on the X server.

Comment 13

[Masaki Katakai]

Brian,

I'm debugging red color problem and found the
GetColor() changes the value of color in Solaris.
What is the purpose of GetColor()?

nsFreeType.cpp:nsFreeTypeXImage::DrawString()

  aContext->GetColor(color);

I understand the color handling of FreeType is
the same with nsXFontAAScaledBitmap.cpp. However,
I can not find GetColor() in it. When I just
removed the line, everything works fine.

Comment 14

[kill this account]

Looks like a mistake to me.
I'll check this on my build at the office.

Comment 15

[Masaki Katakai]

For re-scanning problem, I found the following codes do not work on Solaris,

nsFontCatalog.cpp: nsFontCatalogAddFceIfCurrent()
  //
  // Check the time
  //
  PRInt64 fs_modtime = fce->mMTime;
  LL_MUL(fs_modtime, fs_modtime, 1000);
  if (LL_NE(aFileModTime, fs_modtime))
    return -1;

aFileModTime is from dirEntry->GetLastModifiedTime(), which is
   1011768113370 on Solaris
   1011768113000 on Linux
The initial fs_modtime is 1011768113. So we compare here
   1011768113370 vs. 1011768113000 Solaris
   1011768113000 vs. 1011768113000 Linux

fce->mMTime is file_info.st_mtime. Do we need to compare after MULL?
Why don't we compare after aFileModTime DIV 1000?

  //
  // Check the time
  //
  PRInt64 fs_modtime;
  LL_I2L(fs_modtime, fce->mMTime);
  LL_DIV(aFileModTime, aFileModTime, 1000L);
  if (LL_NE(aFileModTime, fs_modtime)) 
    return -1;

Comment 16

[kill this account]

I believe that was already done (at Simon's request). See bug 116154 and
compare attachment 62308 [details] [diff] [review] and attachment 63429 [details] [diff] [review]. 

Katakai, could you see if this is correct and fixes the unnecessary re-scan?

Comment 17

[Masaki Katakai]

The changes are working fine. Thank you.

Comment 18

[kill this account]

Katakai: if this is okay could you r= it?

Comment 19

[Masaki Katakai]

r=katakai

There are not so many new codes here. Most codes are moved for re-use.

I've tested Japanese, Korean, Chinese and iso-8859-1, 8859-2 web
pages through this patch. Looks good.

Sorry for late.

Comment 20

[kill this account]

Attachment: de-rotted

Katakai: sorry to bug you but the old patch rotted a bit. Could you diff
the old and new patches and r= the new patch?

Thanks

Comment 21

[Masaki Katakai]

r=katakai

you just simply backed the current codes around "BitmapScaleAlways".
No problem.

Comment 22

[kill this account]

Comment on attachment 67374 [details] [diff] [review]
de-rotted

set "has-review" for Katakai's review

Comment 23

[Brendan Eich [:brendan]]

rbs, would you be willing to do a delegated-from-me sr= here?  Thanks very much
if so.  Otherwise please let me know that you can't and I'll find another sr.  I
suspect you'll do the best SR job in the least time in this case, so I'm happy
to delegate.

/be

Comment 24

[rbs]

Logically, the changes looks alright here. I had troubles with the naming 
convention.

+  nsFreeTypeFreeGlobals();
+  rv = nsFreeTypeInitGlobals();

I often prefer using the namespace prefix |ns| for declarations (struct/class)
[But let say I haven't seen this since this is a continuation from the other 
sets of changes (as per the tracker bug 116147), and other reviewers seemed to 
have let it go.]

What I noted however is that the porting of the MathML's GetBoundingMetrics() 
was left out throughout the process. Now that MathML is part of the default 
builds and since MathML is obviously a key aspect from my perspective, I would 
expect some action in this respect. bstell has advised me by e-mail that he can 
come up with a fix for that pretty soon. So I will defer the review stamp for 
this bug & bug 116154 until then (as I noted, the patch by istelf already looks 
ok).

Any reason why you are using:
+// sample prefs listing TrueType font dirs
+//pref("TrueType.font.dir.1", "/u/sam/tt_font");
+//pref("TrueType.font.dir.2", "/u/sam/tt_font2");
+

rather than:
+//pref("font.TrueType.font.dir.1", "/u/sam/tt_font");
+//pref("font.TrueType.font.dir.2", "/u/sam/tt_font2");

|font.| is the recognized namespace for fonts's prefs. if these prefs were to be 
configurable via the UI one day, prefs' listeners won't need to be changed.

Comment 25

[kill this account]

I'll start working on the requested changes.
I already have working code for GetBoundingMetrics and will add it to the
patch.

Comment 26

[timeless]

why not one of:
pref("font.truetype.dir.1", "/u/sam/tt_font");
pref("font.truetype.directory.2", "/u/sam/tt_font2");
pref("font.dir.truetype.3", "/u/sam/tt_font2");
pref("font.directory.truetype.4", "/u/sam/tt_font2");

the only prefs that use uppercase are PICS and PICS was just generally evil.

if directories might be used for font systems other than truetype, it might 
make sense to use 3/4.  As for the 1,3 v 2,4 what were the general rules 
(esp wrt abbreviations) for naming pref branches?

Comment 27

[kill this account]

Attachment: pre request: added GetBoundingMetrics, changed the pref name, change the function names

Comment 28

[rbs]

Comment on attachment 70043 [details] [diff] [review]
pre request: added GetBoundingMetrics, changed the pref name, change the function names

Index: gfx/src/gtk/nsFontMetricsGTK.cpp
===================================================================

+PRBool  gEnableFreeType2 = PR_TRUE;
[...]
+  PRBool enable_freetype2 = PR_TRUE;
+  rv = gPref->GetBoolPref("font.FreeType2.enable", &enable_freetype2);
+  if (NS_SUCCEEDED(rv)) {
+    gEnableFreeType2 = enable_freetype2;
+    FREETYPE_FONT_PRINTF(("gEnableFreeType2 = %d", gEnableFreeType2));
+  }

-> so the default is going to be to enable at first checkin?

 void nsFontMetricsGTK::RealizeFont()
 {
+  float f;
+  mDeviceContext->GetDevUnitsToAppUnits(f);
+
+  if (mWesternFont->IsFreeTypeFont()) {
+    nsFreeTypeFont *ft = (nsFreeTypeFont *)mWesternFont;
+    if (!ft)
+      return;
+    // these should be moved to nsFontGTK

-> Why? Although it is sometimes unsastfactory w.r.t. intl layout, the
ASCII (Western) font remains the font to be used for default CSS metrics.

+#if (defined(MOZ_ENABLE_FREETYPE2))
+    int lineSpacing = ft->ascent() + ft->descent();
+    if (lineSpacing > mWesternFont->mSize) {
+      mLeading = nscoord((lineSpacing - mWesternFont->mSize) * f);
+    }
+    else {
+      mLeading = 0;
+    }
+    mEmHeight = PR_MAX(1, nscoord(mWesternFont->mSize * f));
+    mEmAscent = nscoord(ft->ascent() * mWesternFont->mSize * f / lineSpacing);
+    mEmDescent = mEmHeight - mEmAscent;
[...]
+    return;
+#endif /* (defined(MOZ_ENABLE_FREETYPE2)) */
+  }

Index: gfx/src/x11shared/nsFreeType.cpp
===================================================================
+  aBoundingMetrics.leftBearing  = 0;
+  aBoundingMetrics.rightBearing = 0;
+  aBoundingMetrics.width	 = 0;
+  aBoundingMetrics.ascent	 = 0;
+  aBoundingMetrics.descent	 = 0;

-> There is a helper function to do that: aBoundingMetrics.Clear();


@@ -879,7 +943,7 @@

 gint
 nsFreeTypeXImage::DrawString(nsRenderingContextGTK* aContext,
			     nsDrawingSurfaceGTK* aSurface, nscoord aX,
			     nscoord aY, const PRUnichar* aString,
			     PRUint32 aLength)
 {

[...]

-  bbox.xMax = MAX(bbox.xMax, pos+1);
+  nsresult rslt;
+  nsBoundingMetrics bbm;
+  rslt = GetBoundingMetrics(aString, aLength, bbm);
+  if (NS_FAILED(rslt))
+    return 0;

+  // make sure we bring down enough background for blending
+  bbm.rightBearing = MAX(bbm.rightBearing, bbm.width+1);

hmm... you are making DrawString() depend on  GetBoundingMetrics().
Beware that it will break for people (e.g., embeddors) who may not
want to enable MathML.

[... the other changes are to recover from the change of syntax]

Comment 29

[kill this account]

Attachment: req changes

> +PRBool  gEnableFreeType2 = PR_TRUE;
> ...
> -> so the default is going to be to enable at first checkin?

The default is disabled by the pref in unix.js:

  pref("font.FreeType2.enable", false); 
 
> ...
> +    // these should be moved to nsFontGTK
> ...
> -> Why? Although it is sometimes unsastfactory w.r.t. intl layout, the
> ASCII (Western) font remains the font to be used for default CSS metrics.
> ...
> +    int lineSpacing = ft->ascent() + ft->descent();

(Assumming I correctly understand your question)

I've reworded the comment:

   // now that there are multiple font types (eg: core X fonts 
   // and TrueType fonts) there should be a common set of methods 
   // to get the metrics info from the font object. These methods 
   // probably should be virtual functions defined in nsFontGTK.


> -> There is a helper function to do that: aBoundingMetrics.Clear();

Thanks for reminding me; changed.

>  nsFreeTypeXImage::DrawString(nsRenderingContextGTK* aContext,
> ...
> +  rslt = GetBoundingMetrics(aString, aLength, bbm);
> ...
> hmm... you are making DrawString() depend on	GetBoundingMetrics().

Moved the code to a function, doGetBoundingMetrics, which is outside of 
the '#ifdef MathML'.

Comment 30

[rbs]

Comment on attachment 70113 [details] [diff] [review]
req changes

+nsresult
+nsFreeTypeFont::doGetBoundingMetrics(const PRUnichar*	 aString,
+				    PRUint32	       aLength,
+				    nsBoundingMetrics& aBoundingMetrics)

The |nsBoundingMetrics| struct itsefl is declared in an #ifdef. So you need to
pass all the fields individually (and fix the callers to use them):

+nsFreeTypeFont::doGetBoundingMetrics(const PRUnichar*	 aString,
+				    PRUint32	       aLength,
+		 width, ascent, descent, lbearing, rbearing)

Marking sr=rbs on behalf of brendan.

Comment 31

[kill this account]

> The |nsBoundingMetrics| struct itsefl is declared in an #ifdef. So you need to
> pass all the fields individually (and fix the callers to use them):

Thanks for pointing this out. I will do this.

Comment 32

[kill this account]

Attachment: req changes

changed doGetBoundingMetrics to not use nsBoundingMetrics

Comment 33

[kill this account]

checked in

Comment 34

[kill this account]

set status

Comment 35

[kill this account]

set status

Comment 36

[Masaki Katakai]

Sorry for late comments... I got some a comment from japanese users.

Does Mozilla ft2 support multiple faces of ttc ?

It seems that the mono face of ttc is now being used.

When we add ttc files into font path by setting prefs and the
ttc has multiple faces (mono and propotional), can we handle
the font as different font?

Currently we use "vendor name" + "family name" as font name
in Linux preference, but do you think we should add "face"
to name field for ttc?

Should I file an RFE?

Comment 37

[Masaki Katakai]

filed bug as bug 126572 for ttc issue.

Comment 38

[kill this account]

marking fixed

Comment 39

[kill this account]

this code has been checked in so I'm closing out the "needs review" bug