172260 - Integrate sil.org graphite into window gfx.

Status: RESOLVED WONTFIX

(Core :: Internationalization, defect)

Description

[Frank Tang]

ok, this is my pet project at my late night hacking time. Netscape/AOL won't
care about this but I will try to make this work as my personal interest at my
personal time. 
sil.org open sourece it's complex script rendering engine- Graphtie. And they
ask my help to intergrate it into mozilla so they can show it to someone
important in the world. 

http://graphite.sil.org/index.htm

I look at Graphite, it is not really ready yet, but I start my work earily.

Comment 1

[Frank Tang]

reassing QA to [e-mail] so it won't bother Netscape IQA for now.

Comment 2

[Frank Tang]

here is the basic desing
1. all the graphite font is standard TrueType font with additional table. All of
them have a 'Silf' table in the TrueType diectory. So we can base on that to
decide to call Graphite or not
2. I put down a COM (not xpcom based) ISpecialTTFFont interface and when we load
an Unicode TrueType font, if it has e 'Silf' table, we crate a subclass of
nsFontWinUnicode (which call nsFontWithSilGraphite) instead of  nsFontWinUnicode
. The nsFontWithSilGraphite will be implement inside nsFontMetricsWin.cpp . The
nsFontWithSilGraphite  will call try to create a COM object of ISpecialTTFFont 
interface with the sil cid. The nsFontWithSilGraphite  simply deldgate those
call (DrawText, GetBoundingMetris, GetTextWidth) to the ISpecialTTFFont. 

3. We land the GraphiteCID.h file (Which define the CID) and ISpecialTTFFont.h
(which define the COM interface and the IID) to both the mozilla source tree
(since it is written by me) AND the Graphite source tree. 

4. in the Graphite source tree (they are open source too). We implement a COM
object which implmenet the ISpecialTTFFont interface and call Graphite library
to do the rendering

in the nsFontWithSilGraphite implementation, if we failed to crate the
ISpecialTTFFont object, or if the forwarding code failed, we fallback to the
nsFontWinUnicode implementation.  In this way, in case the graphite library is
not there, we can still render text (could be incrrectly position the glyph)

I will submit the necssary change in  the mozilla tree to mozill and work with
sil.org folks to write that ISpecialTTFFont implementation based on Graphite. 

Comment 3

[Frank Tang]

Attachment: a new file ISpecialTTFFont.h this should be submit and added into both mozilla tree (mozilla/gfx/src/windows) and graphite open source tree. I will submit twice, once under mpl and once on sil license

COM base (not XPCOM based) interface file between mozilla and graphite

Comment 4

[Frank Tang]

Attachment: a new file GraphiteCID.h, this file should also be check into both mozilla tree (as mozilla/gfx/src/windows) and graphite tree. I will submit into mozilla under mpl and submit into graphite under si

Comment 5

[Frank Tang]

Attachment: diff file to hook up the graphite COM base interface, based on Mozilla 1.0 branch for now

Comment 6

[Frank Tang]

Attachment: stub file which implement a COM dll which implement the ISpecialTTFFont.h interface

Here is a VC6 project include a stub implementation of the ISpecialTTFFont.h 
People who familiar with Graphite should look at silFont.cpp and replace those
fake implement which use GDI with the Graphite calls

Comment 7

[Frank Tang]

ok, here is how we going to integrate Graphite with mozilla
1. first, build the mozilla window with MOZILLA_1_0_BRANCH (we will back port to
the trunk later, for now, while we developed the code, let's stick with the 1.0
branch, we will never check INTO the 1.0 branch but this is a stabl branch that
we could start our work untill the prototype in good shape). Read the
documentation in www.mozilla.org to figure how to setup your tools, cvs and how
to build mozilla. make sure you use the new "make" build process instead of the
old "nmake" build process. 
2. once you build it, download the first 2 attachment ISpecialTTFFont.h and
GraphiteCID.h into mozilla/gfx/src/windows directory
3. save the 3rd attachemnt as diff.txt into mozilla/gfx/src/windows, this is the
patch
4. run patch < diff.txt to patch the mozilla source tree
5. in mozilla/gfx/src/ , run make to rebuld the code
now, your mozilla will looking for ISpecialTTFFont implementation with
GraphiteCID while it hit a font which have 'Silf' table in the TrueType directory
6. now open the  4th attachement, unzip it, and build the project
7. When the project finish building, it should register the dll COM registry

Comment 8

[Frank Tang]

Attachment: test html

Comment 9

[Frank Tang]

I forgot to include the test  html into the zip file, so I attache here
ok, how to test
8. open the mozilla from the msdev: open mozilla.exe as the workspace in the msdev
9. set break point  in silFont.cpp ( mozgraphite.dll)
10. run mozilla from msdev
11. view the html page (the 5th attchment here)
if you have Graphite font installed in your system (those 3 fake font), you
should hit the break point in the silFont.cpp 

now you can replace those implementation in silFont.cpp to call Graphite from it. 
Currently, the OpenGraphite is lacking documentation, header file, SDK (header
file and DLL), sample code for me to continue the work to complete a protoype
implementation of silFont.cpp . Either someone know Graphite pretty will
continue that, or sil.org put out a good programmer guide, sample code, header
file, dll to let me (or someone else) to continue the work

Comment 10

[Frank Tang]

while I said the opengraphite is lacking of documentation, don't get me wrong, I
do look at http://fieldworks.sil.org/objectweb/ . However, the OpenGraphite rev8
release at Sept20, 2002 seems not sync with those documentation at
http://fieldworks.sil.org/objectweb/ . For example, the documentation there said
GrEngine implement IRenderEngine and GrSegment implement ILgSegment but the
source code in there does not show such Hierarchy . 

Comment 11

[Frank Tang]

in the test html, I divided into 3 section, each section contains a table.
The left one is using font-family to force a Graphite font and the right hand
one will not use Graphite font. 

Comment 12

[Frank Tang]

Attachment: mozgraphite2.zip 2nd version of the stub, it crash

here is the 2nd version of the dll, I try to call the DLLs in WorldPad through
COM interface. Somehow I still crash in IRenderEngine->NextBreakPoint . In
order to run this, you should install the WorldPad from
http://fieldworks.sil.org/WorldPad/setup.exe first. 
then you apply the last patch into the m1.0 branch and build this one
The patched mozilla will call the dll implement by this zip file, the dll in
this zip file will call the dlls in the WorldPad

I will put a test driver which let us debug this easier. That test app will
call the implementation of this zip so you won't need to build and run the
mozilla to see the crash.

Comment 13

[Frank Tang]

Attachment: testdriver.zip - test driver application

this is a zip file for test driver application.
unzip it and build it.
if the WorldPad is install (see previous comment) and the dll in
mozgraphite2.zip is build, then this application will call the dll which in the
mozgraphite2.zip and that will call the dll in the worldpad. It will CRASH in
the IRenderEngine->NextBreakPoint. I guess the reason is I don't know how to
implement ITextSource correctly.

Comment 14

[Frank Tang]

Attachment: stanalong "hello world" application show problem to make Graphite to draw text. a .zip file. Unzip and buid it.

Comment 15

[Frank Tang]

Attachment: patch for trunk in cvs diff -u format

This is the patch for mozilla trunk
please see http://sila.mozdev.org for more details
I need to change the license for the two .h file so they will be able to land
into both the Graphite and Mozilla source tree. But please review the technical
stuff first.

Comment 16

[Frank Tang]

shanjian- can you review the code now?

Comment 17

[Shanjian Li]

Comment on attachment 110826 [details] [diff] [review]
patch for trunk in cvs diff -u format

The code mostly looks fine to me. I have several small suggestions.

>@@ -2209,7 +2243,14 @@
>         PRUint16* ccmap = GetCCMAP(aDC, logFont.lfFaceName, &fontType, nsnull);
>         if (ccmap) {
>           if (eFontType_Unicode == fontType) {
>-            font = new nsFontWinUnicode(&logFont, hfont, ccmap);
>+            HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
>+            DWORD len = GetFontData(aDC, SILF, 0, nsnull, 0);
>+            if (len != GDI_ERROR) { // has 'Silf', so this is a SIL Graphite font
>+              font = new nsFontWinSilGraphite(&logFont, hfont, ccmap);
>+            } else {
>+              font = new nsFontWinUnicode(&logFont, hfont, ccmap);
>+            }
>+            ::SelectObject(aDC, (HGDIOBJ)oldFont);
This part of code was used later as well, so it would be better if you
separated as a function, IsSilcFont() or sth similar.

>@@ -2246,7 +2287,14 @@
>   if (hfont) {
>     nsFontWin* font = nsnull;
>     if (eFontType_Unicode == aGlobalFont->fonttype) {
>-      font = new nsFontWinUnicode(&logFont, hfont, aGlobalFont->ccmap);
>+      HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
>+      DWORD len = GetFontData(aDC, SILF, 0, nsnull, 0);
>+      if (len != GDI_ERROR) { // has 'Silf', so this is a SIL Graphite font
>+        font = new nsFontWinSilGraphite(&logFont, hfont, aGlobalFont->ccmap);
>+      } else {
>+        font = new nsFontWinUnicode(&logFont, hfont, aGlobalFont->ccmap);
>+      }
>+      ::SelectObject(aDC, (HGDIOBJ)oldFont);
>     }
>     else if (eFontType_NonUnicode == aGlobalFont->fonttype) {
>       nsCOMPtr<nsIUnicodeEncoder> converter;
Here it is again, see my comment above. 

>Index: windows/nsRenderingContextWin.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/windows/nsRenderingContextWin.cpp,v
>retrieving revision 3.157
>diff -u -r3.157 nsRenderingContextWin.cpp
>--- windows/nsRenderingContextWin.cpp	21 Dec 2002 16:39:57 -0000	3.157
>+++ windows/nsRenderingContextWin.cpp	26 Dec 2002 22:37:01 -0000
>@@ -1526,11 +1526,15 @@
>       return mFontMetrics->GetSpaceWidth(aWidth);
>     }
> 
>-    SIZE  size;
> 
>     SetupFontAndColor();
>-    ::GetTextExtentPoint32(mDC, aString, aLength, &size);
>-    aWidth = NSToCoordRound(float(size.cx) * mP2T);
>+    nsFontMetricsWin* metrics = (nsFontMetricsWin*)mFontMetrics;
>+    nsFontWin* font = (nsFontWin*)metrics->mLoadedFonts[0];
>+    NS_ASSERTION(font, "we should have at least one font loaded");
>+    if(font) {
>+      PRInt32 w = font->GetWidth(mDC, aString, aLength);
>+      aWidth = NSToCoordRound(float(w) * mP2T);
>+    }

Probably SetupFontAndColor is not necessary here.

>@@ -1668,9 +1672,14 @@
>         mFontMetrics->GetSpaceWidth(twWidth);
>       } 
>       else if (numChars > 0) {
>-        SIZE  size;
>-        ::GetTextExtentPoint32(mDC, &aString[start], numChars, &size);
>-        twWidth = NSToCoordRound(float(size.cx) * mP2T);
>+        nsFontMetricsWin* metrics = (nsFontMetricsWin*)mFontMetrics;
>+        nsFontWin* font = (nsFontWin*)metrics->mLoadedFonts[0];
>+        NS_ASSERTION(font, "we should have at least one font loaded");
>+        if(font) {
>+          PRInt32 w = font->GetWidth(mDC, &aString[start], numChars);
>+          twWidth = NSToCoordRound(float(w) * mP2T);
>+        }
>+
Would it be better if you add GetWidth() method to nsFontMetricsWin instead of
operation of its data member here? This repeats several times below.

Comment 18

[Frank Tang]

>This part of code was used later as well, so it would be better if you
>separated as a function, IsSilcFont() or sth similar.
Will do

>>-    SIZE  size;
>> 
>>     SetupFontAndColor();
>>-    ::GetTextExtentPoint32(mDC, aString, aLength, &size);
>>-    aWidth = NSToCoordRound(float(size.cx) * mP2T);
>>+    nsFontMetricsWin* metrics = (nsFontMetricsWin*)mFontMetrics;
>Probably SetupFontAndColor is not necessary here.

Not sure, I didn't touch the line of code here. 

>Would it be better if you add GetWidth() method to nsFontMetricsWin instead of
>operation of its data member here? This repeats several times below.

So... what happen is there are already a const PRUnichar* version of GetWidth of
nsFontWin, but there are no const char* version of GetWidth. I simply add the
const char* version to it. This is needed if we use Graphite to display
PigLatin. The origional code assume the Window's API can handle the const char*
version will, but in the case of PigLatin we want to pass that to the Graphite
code to render it. 
That is what we really try to solve. You can see there are already a const
PRUnichar* version do the similar thing there. 

Comment 19

[Frank Tang]

mark it as assigned and m1.3beta

Comment 20

[Frank Tang]

>Would it be better if you add GetWidth() method to nsFontMetricsWin instead of
>operation of its data member here? This repeats several times below.
I think I got your point.
I don't want to add a GetWidth to nsFontMetricWin because that will break the arch
But I can add a FindASCIIFont to nsFontMetricsWin to return the nsFontWin for
the ascii font instead of accessing the nsFontWin directly from the mLoadedFonts

Does that work for you. I once consider just use the nsFontMetricsWin::FindFont
(with PRUint32('a') to do that but that seems will impact the performance in a
big way. 

Look at my new patch to see it. 

Comment 21

[Frank Tang]

Attachment: patch v2

update patch
1. add public license text to ISpecialTTFFont.h and GraphiteCID.h
2. add a static function IsSilFontOnDC 
3. add a FindASCIIFont in nsFontMetricsWin so code in nsRenderingContextWin do
not 
need to access the member data directly.
I have consider use nsFontMetrcisWin::FindFont to do that but the performance
impact could be big. The FindASCIIFont implementation is much simpler to keep
performance. 
I do not want to add a GetWidth method to nsFontMetricsWin because I think that
will change the Architech.

I am not sure about the SetupFontAndColor. My code does not change anything
which will remove to need to have it there. If there are a reason it is there,
I think the reason is still there. I think remove that line now is risky.

Comment 22

[Shanjian Li]

Comment on attachment 111529 [details] [diff] [review]
patch v2

looks fine.

Comment 23

[Frank Tang]

Comment on attachment 111529 [details] [diff] [review]
patch v2

alecf- can you sr= this one.

Comment 24

[Frank Tang]

alecf talked to me in person and ask for the following number
1. binary size different after patch - before patch
2. cowtool number with sila enable but without mg2 or font installed.

Will do this soon.

Comment 25

[Alec Flett]

Comment on attachment 111529 [details] [diff] [review]
patch v2

+#undef SILF
+#define SILF (('S') | ('i' << 8) | ('l' << 16) | ('f' << 24))

can we call this something more verbose, like SILF_FONT_ID or something?
otherwise it is not obvious what they are for.
(same for LOCA and NAME I guess as well, though they aren't part of this patch)

   aFont->mMaxAscent = NSToCoordRound(metrics.tmAscent * dev2app);
   aFont->mMaxDescent = NSToCoordRound(metrics.tmDescent * dev2app);
+  aFont->mMaxAscent += NSToCoordRound( aFont->GetAdjustAscent(aDC) * dev2app);
+  aFont->mMaxDescent += NSToCoordRound( aFont->GetAdjustDescent(aDC) *
dev2app);

how about 
aFont->mMaxAscent = a + b;
instead of
aFont->mMaxAscent = a;
aFont->mMaxAscent += b;

it might be more obvious what is going on.

-	     font = new nsFontWinUnicode(&logFont, hfont, ccmap);
+	     HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
+	     // The nsFontWinSilGraphite constructor assume the font is
selected
+	     // on DC for performance reason
+	     if (IsSilFontOnDC(aDC)) {
+	       font = new nsFontWinSilGraphite(&logFont, hfont, ccmap);
+	     } else {
+	       font = new nsFontWinUnicode(&logFont, hfont, ccmap);
+	     }
+	     ::SelectObject(aDC, (HGDIOBJ)oldFont);

I see this code block duplicated almost exactly (with the exception of
aGlobalFont->ccmap for the ccmap) - can you combine these into a single
function?

   mMaxHeight = NSToCoordRound(metrics.tmHeight * dev2app);
+  mMaxHeight += adjustAscent;
+  mMaxHeight += adjustDescent;

more of the a + b stuff from above, and this goes for mEmAscent, and so forth.
This code needs to be consolidated and documented.

+  mDelegate = NULL;
+  ::CoInitialize(NULL);
+  ::CoCreateInstance(CLSID_silFont, NULL, CLSCTX_INPROC_SERVER,
IID_ISpecialTTFFont, (void**)&mDelegate);
+  if (mDelegate)
+  {
+    HRESULT res = mDelegate->InitInfo(aLogFont, aFont);
+  }
+}

should you at least NS_ASSERTION if the creation fails? (under the assumption
that the constructor shouldn't even be called if SILF isn't installed)

-    ::GetTextExtentPoint32(mDC, aString, aLength, &size);
-    aWidth = NSToCoordRound(float(size.cx) * mP2T);
+    nsFontMetricsWin* metrics = (nsFontMetricsWin*)mFontMetrics;
+    // find the nsFontWin for ASCII
+    nsFontWin* font = (nsFontWin*)metrics->FindASCIIFont(mDC);
+    NS_ASSERTION(font, "we should have at least one font loaded");
+    if(font) {
+      PRInt32 w = font->GetWidth(mDC, aString, aLength);
+      aWidth = NSToCoordRound(float(w) * mP2T);
+    }

again this code is duplicated almost exactly right below, TWICE, with only
minor differences. Please abstract this into a seperate function so as not to
cause unnecessary bloat.

One more block of almost-duplicated code below that most recent one (I won't
bother pasting)

it looks good overall, but those things have to be cleaned up.

Comment 26

[Frank Tang]

ok, I finally have chance to run the cowtools and size comparision with
disable-debug build

I check out the tip today and here is the result based on the latest patch
(patch v2, 2003-01-14 11:34:33)

About Size
Before the changes:
gkgfxwin.dll 450048
After the changes:
gkgfxwin.dll 450260

It is 212 Bytes bigger

Performance running on a machine which DO NOT have the sila dll instlled:

Without  Changes (I run twice)
http://cowtools.mcom.com/page-loader/report.pl?id=3E9DA7F9A1
See Notes at the bottom of this page for some details.

Test id: 3E9DA7F9A1
Avg. Median : 813 msec		Minimum     : 117 msec
Average     : 824 msec		Maximum     : 3303 msec
http://cowtools.mcom.com/page-loader/report.pl?id=3E9DA9F35A

Test id: 3E9DA9F35A
Avg. Median : 817 msec		Minimum     : 122 msec
Average     : 824 msec		Maximum     : 2731 msec


With Change
http://cowtools.mcom.com/page-loader/report.pl?id=3E9DA46146 
See Notes at the bottom of this page for some details.

Test id: 3E9DA46146
Avg. Median : 818 msec		Minimum     : 109 msec
Average     : 819 msec		Maximum     : 2891 msec

You can see there are no significent performance difference here. 

Comment 27

[Frank Tang]

thanks for alice review, I will provide a new patch soon.

Comment 28

[rbs]

+  // all font should include ASCII part so just return 
+  // the first one for performance issue

Not true (e.g., MathML fonts). It is going to break MathML. I think there are
correctness issues and further room for improvement to this patch.

Comment 29

[Frank Tang]

>About Size
>Before the changes:
>gkgfxwin.dll 450048

I have a local patch which merge the the function as alecf as, the size is
gkgfxwin.dll 455464
it increase size 5424 bytes now. 

I will attach the new patch once I fully test it. 

Comment 30

[Frank Tang]

>should you at least NS_ASSERTION if the creation fails? (under the assumption
>that the constructor shouldn't even be called if SILF isn't installed)
No, we should not. Because for all the machine which does not installed with the
sila dll, it WILL fails. It will only success if the sila dll got also installed
into the machine. And you can see if it fail, we should handle it gracefully,
regardless opt build or debug build, because the sila dll belong to a different
open source project. 

I could add something like 
#ifdef DEBUG_ftang
NS_ASSERTION()
#endif

there but I think I rather do it in my local tree instead of check it in to
bother everyone. 

Comment 31

[rbs]

Comment on attachment 111529 [details] [diff] [review]
patch v2

Obsoleting the r= since there are correctness issues with the patch.

+      HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
+      // The nsFontWinSilGraphite constructor assume the font is selected
+      // on DC for performance reason
+      if (IsSilFontOnDC(aDC)) {
+	 font = new nsFontWinSilGraphite(&logFont, hfont, aGlobalFont->ccmap);
+      } else {
+	 font = new nsFontWinUnicode(&logFont, hfont, aGlobalFont->ccmap);
+      }
+      ::SelectObject(aDC, (HGDIOBJ)oldFont);

Actually, a neater way might be just to add a new
|eFontType_SilGraphite| which gets initialized where the others are, and you
can simply query that later on. [This way, no need for those Select/Unselect].

Comment 32

[Frank Tang]

ok.... for the FindASCIIFont change issues. I think we can live without it. I 
added it there for the PigLatin demo in SIL and that is not useful for the real 
world

about eFontType_SilGraphite, I will take a closer look at it. 

Comment 33

[rbs]

> ok.... for the FindASCIIFont change issues. I think we can live without it.

The ASCII font is kept in |mFontHandle| and there is a helper function that can
help you out. You can implement FindASCIIFont as:

FindASCIIFont() {
  return GetFontFor(mFontHandle);
}

> about eFontType_SilGraphite, I will take a closer look at it. 

yep, you can just replace this chunk of code:

         PRUint16* ccmap = GetCCMAP(aDC, logFont.lfFaceName, &fontType, nsnull);
         if (ccmap) {
           if (eFontType_Unicode == fontType) {
-            font = new nsFontWinUnicode(&logFont, hfont, ccmap);
+            HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
+            // The nsFontWinSilGraphite constructor assume the font is selected
+            // on DC for performance reason
+            if (IsSilFontOnDC(aDC)) {
+              font = new nsFontWinSilGraphite(&logFont, hfont, ccmap);
+            } else {
+              font = new nsFontWinUnicode(&logFont, hfont, ccmap);
+            }
+            ::SelectObject(aDC, (HGDIOBJ)oldFont);
           }


// assuming you set |fontType| to |eFontType_SilGraphite| in GetCCMAP() [notice
// that the font is already selected in the DC at this stage]
         PRUint16* ccmap = GetCCMAP(aDC, logFont.lfFaceName, &fontType, nsnull);
         if (ccmap) {
           if (eFontType_Unicode == fontType) {
            font = new nsFontWinUnicode(&logFont, hfont, ccmap);
           }
           else if (eFontType_NonUnicode == fontType) {
            ...
           }
+          else if  if (eFontType_SilGraphite == fontType) {
+            font = new nsFontWinSilGraphite(&logFont, hfont, ccmap);
+          }
          ...

Comment 34

[Frank Tang]

what a hack. I have not touch mozilla code for 2 years. I didn't read these bugs
for 2 years. And they are still there. Just close them as won't fix to clean up.

Comment 35

[Travis Chase]

Mass Bug Re-Open of bugs Frank Tang Closed with no good reason. Spam is his
fault not my own

Comment 36

[Travis Chase]

Mass Re-assinging Frank Tangs old bugs that he closed won't fix and had to be
re-open. Spam is his fault not my own

Comment 37

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 38

[Simon Montagu :smontagu]

In this specific case, since it was Frank Tang's private project,  WONTFIX was the right resolution. There is a mozdev project to do to this http://sila.mozdev.org/, but I don't know how active it is.