102706 - change nsFontMetricsWin.cpp to use nsCompressedCharMap

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Frank Tang]

we should update nsFontMetricsWin.cpp to use nsCompressedCharMap to 
1. save space
2. prepare for Surrogate support

Comment 1

[Frank Tang]

here is the information bstell provide:

Change the map from PRUint32* to PRUint16*

  old: PRUint32* mMap;

  new: PRUint16* mCCMap;

Change the alloc/set-chars call:

  old: aSelf->mMap = (PRUint32*) PR_Calloc(2048, 4);
  old: res = mapper->FillInfo(aSelf->mMap);
  old: if (NS_SUCCEEDED(res)) {

  new: aSelf->mCCMap = MapperToCCMap(mapper);
  new: if (aSelf->mCCMap) 

Change the char test macro:

  old: if (FONT_HAS_GLYPH(font->mMap, aChar)) {

  new: if (CCMAP_HAS_CHAR(font->mCCMap, aChar)) {

Change the map free call:

  old: PR_FREEIF(charsetMap->mInfo->mMap);

  new: FreeCCMap(charsetMap->mInfo->mCCMap);

Comment 2

[Frank Tang]

shanjian, can you work with bstell to do this ?

Comment 3

[Shanjian Li]

accepting.

Comment 4

[Shanjian Li]

After I looked into nsFontMetricsWin, I found the change is far more complicated than I expected. I need to allocate myself enough time to do this. 

Comment 5

[Frank Tang]

we are still not sure how this support interace with 102557 yet. But we
definitly don't want to create 17 x 8K bytes for each font. We probably need to
implement this on window first before we add surrogate font support at 102557. 

>After I looked into nsFontMetricsWin, I found the change is far more 
>complicated than I expected. I need to allocate myself enough time to do this. 
Not trival 

Comment 6

[Frank Tang]

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

Comment 7

[Shanjian Li]

Attachment: patch 1.0

Comment 8

[Shanjian Li]

Attachment: patch 2.0

Comment 9

[Shanjian Li]

Attachment: new added file, together with patch2.0

Comment 10

[Shanjian Li]

bstell, can you code view my patch, especially the part in 
nsCompressedCharMap.cpp?
rbs, can you code review my patch for the windows part? 

Comment 11

[Shanjian Li]

Attachment: patch 3.0

Comment 12

[Shanjian Li]

Attachment: patch 4.0

Comment 13

[rbs]

+	.\$(OBJDIR)\nsCCMapWrap.obj          \
>>>you can get rid of this strange '#include "..\nsCompressedCharMap.cpp"'
by adding nsCompressedCharMap to gfx/src/makefile.win

============
+  nsCRT::memset(map, 0, sizeof(PRUint32)*UCS2_MAP_LEN);
>>>nit:
+  nsCRT::memset(map, 0, sizeof(map));

============
+  ccmap = MapToCCMap(map);
+  return ccmap;
>>>nit:
+  return MapToCCMap(map);

============
+  PRUint16* ccmap = GetFontCCMAP(aDC, aShortName, aFontType, aCharset);
+  if (ccmap == nsnull) {
>>>nit:
+ if (!ccmap)

============
       font = LoadSubstituteFont(aDC, &name);
       if (font) {
-        ADD_GLYPH(font->mMap, c);
         mSubstituteFont = font;
         return font;
       }
>>>it is incorrect to remove that, it is the very first char that causes
the creation of the substitution font, and it needs to be recorded that it is 
representable in the substitution font that was just created.

=====
-      nsFontWin* font = new nsFontWinSubstitute(&logFont, hfont, map, 
displayUnicode);
+    nsFontWinSubstitute* font = new nsFontWinSubstitute(&logFont, hfont, 
nsnull, displayUnicode);
>>>see my comments about the constructor below

==============
   if (mSubstituteFont) {
-    nsFontWinA* substituteFont = (nsFontWinA*)mSubstituteFont;
-    ADD_GLYPH(substituteFont->mMap, aChar);
-    ADD_GLYPH(substituteFont->mSubsets[0]->mMap, aChar);
     return mSubstituteFont;
   }
>>>need to set that the char is representable to keep fontHasGlyph() in sync

==============
             if (substituteSubset->Load(aDC, this, substituteFont)) {
-              ADD_GLYPH(substituteFont->mMap, aChar);
-              ADD_GLYPH(substituteFont->mSubsets[0]->mMap, aChar);
               mSubstituteFont = (nsFontWin*)substituteFont;
               return mSubstituteFont;
             }
>>>ditto here

==========================
@@ -4418,27 +4313,13 @@
   HFONT hfont = aFontMetricsWinA->CreateFontHandle(aDC, logFont);
   if (hfont) {
     int i = gCharsetToIndex[mCharset];
-    PRUint32* charsetMap = gCharsetInfo[i].mMap;
-    if (!charsetMap) {
-      charsetMap = (PRUint32*) PR_Calloc(2048, 4); // deleted in FreeGlobals()
-      if (charsetMap) {
-        gCharsetInfo[i].mMap = charsetMap;
-        gCharsetInfo[i].GenerateMap(&gCharsetInfo[i]);
-      }
-      else {
-        ::DeleteObject(hfont);
-        return 0;
-      }
-    }
-    mMap = (PRUint32*) PR_Calloc(2048, 4); // deleted in ~nsFontSubset()
-    if (!mMap) {
+    if (!gCharsetInfo[i].mCCMap)
+      gCharsetInfo[i].mCCMap = gCharsetInfo[i].GenerateMap(&gCharsetInfo[i]);
+    if (!gCharsetInfo[i].mCCMap) {
       ::DeleteObject(hfont);
       return 0;
     }
-    PRUint32* fontMap = aFont->mMap;
-    for (int j = 0; j < 2048; ++j) {
-      mMap[j] = (charsetMap[j] & fontMap[j]);
-    }
+    mCCMap = gCharsetInfo[i].mCCMap;
     mCodePage = gCharsetInfo[i].mCodePage;
     mFont = hfont;
>>> you are changing the semantics here. The previous code didn't assume that
all the glyphs for a charset were available. But I think the change is ok. In 
case of problems we will see the empty squares and can revisit the change.

======
+// A "substitute font" to deal with missing glyphs -- see bug 6585
+// Ultimately, this "font" should map Unicodes to other character codes
+// for which we *do* have glyphs on the system.
+// But, for now, this "font" is only mapping all characters to 
+// a REPLACEMENT CHAR. If the member variable mDisplayUnicode set,
+// the font will instead render unicode points of missing glyphs in
+// the format: &xNNNN;
>>> these comments seem outdated now that there is transliteration + fallback 
to the REPLACEMENT CHAR + hex representation

============
+  nsFontWinSubstitute[A](LOGFONT* aLogFont, HFONT aFont, PRUint16* aCCMap, 
PRBool aDisplayUnicode);
>>>you can get rid of |PRUint16* aCCMap| since it is always null

============
+  virtual inline PRBool HasGlyph(PRUnichar ch) {return 
IS_REPRESENTABLE(mRepresentableCharMap, ch);};
+  virtual inline void SetRepresentable(PRUnichar ch) 
{SET_REPRESENTABLE(mRepresentableCharMap, ch);};
>>>no need to add 'inline' since the method is already inline by virtue of the 
fact its body is in the class declaration.

I suggest to get rid of |CCMAP_HAS_CHAR(font->mCCMap, aChar)| and to consistenly
use |font->HasGlyph(aChar)| everywhere.

Comment 14

[Shanjian Li]

Attachment: patch 5.0

Comment 15

[Shanjian Li]

rbs, thanks a lot for your careful code reviewing. You caught many good points. 
There are several issue I want to discuss with you or clarify. 
. How to include "nsCompressedCharMap.cpp" in makefile.win. I can't find a way 
to do it. Can you show me how? 
. Yes, I did change the semantic for nsFontMetricsWinA and its subset. 
Originally subset->charmap = fontA->map & subset->charset->map. When it comes to 
selecting subset, we already know the fond has such a glyph, so only charset map 
need to be checked. That saved some operation. (And solve the problem that ccmap 
can't be operated directly.)

Comment 16

[rbs]

   if (mSubstituteFont) {
-      if (!FONT_HAS_GLYPH(currFont->mMap, *currChar)) {
+      if (!(currFont->HasGlyph(*currChar))) {
>>>nit: excessive parentheses

=========
-    nsFontWinA* substituteFont = (nsFontWinA*)mSubstituteFont;
-    ADD_GLYPH(substituteFont->mMap, aChar);
-    ADD_GLYPH(substituteFont->mSubsets[0]->mMap, aChar);
+    //make the char representable so that we don't have to go over all font 
before fallback to 
+    //subsituteFont.
+    ((nsFontWinSubstitute*)mSubstituteFont)->SetRepresentable(aChar);
     return mSubstituteFont;
   }
>>>>this still wouldn't work as expected. substituteFont->mSubsets[0] also needs 
to be updated. The way to go about this is, like you did in nsFontWinSubstitute, 
tweak the |class nsFontSubsetSubstitute| to add a normal map[2048] there, and 
then overload fontHasGlyph() there, then change the tests for subset->mMap to 
subset->HasGlyph() everywhere.

======
re: mekefile.win
not sure what is the build problem that you are getting here, but it is the same 
setup as nsFont.h/.cpp (maybe class declarations need NS_COM or something). One 
of the build folks may help here.

Comment 17

[Frank Tang]

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

Comment 18

[Shanjian Li]

Attachment: one more update (Includes many catches by rbs and an optimization)

Comment 19

[rbs]

Getting there, but a few things are still missing.

=========
   if (mSubstituteFont) {
-    nsFontWinA* substituteFont = (nsFontWinA*)mSubstituteFont;
-    ADD_GLYPH(substituteFont->mMap, aChar);
-    ADD_GLYPH(substituteFont->mSubsets[0]->mMap, aChar);
+    //make the char representable so that we don't have to go over all font 
before fallback to 
+    //subsituteFont.
+    ((nsFontWinSubstitute*)mSubstituteFont)->SetRepresentable(aChar);
     return mSubstituteFont;
   }
[...]
             if (substituteSubset->Load(aDC, this, substituteFont)) {
-              ADD_GLYPH(substituteFont->mMap, aChar);
-              ADD_GLYPH(substituteFont->mSubsets[0]->mMap, aChar);
+              substituteFont->SetRepresentable(aChar);
               mSubstituteFont = (nsFontWin*)substituteFont;
               return mSubstituteFont;
             }
>>>see my previous comments. Look for the class |nsFontSubstituteSubset|
and also add what is needed there. Then, you could do:
+ substituteFont->SetRepresentable(aChar);
+ substituteFont->mSubsets[0]->SetRepresentable(aChar);
The _two_ settings are needed for a transparent integration of the 'A' functions 
higher up.

===========
               for (; j < i-1; ++j) {
                 // NOTE: we are overwriting variables here (we now *know* that 
we
                 // will break out of the bigger outer loop after the 
callback...)
                 prevFont = (nsFontWinA*)mLoadedFonts[j];
-                if (FONT_HAS_GLYPH(prevFont->mMap, *currChar)) {
-                  subset = prevFont->mSubsets;
-                  endSubsets = subset + prevFont->mSubsetsCount;
-                  while (subset < endSubsets) {
-                    if (!(*subset)->mMap) {
-                      if (!(*subset)->Load(aDC, this, prevFont)) {
-                        ++subset;
-                        continue;
-                      }
-                    }
-                    if (FONT_HAS_GLYPH((*subset)->mMap, *currChar)) {
-                      // Another font was found, we cannot use the
-                      // current font for this char
-                      goto callback;
-                    }
-                    ++subset;
-                  }
+                if (prevFont->HasGlyph(*currChar)) {
+                  //as long as we know prevFont cover this char, there is 
really no need to 
+                  //find which subset here
+                  goto callback;
                 }
               }
>>>This assumes that prevFont also has a subset with the char. However, as the 
topcrash in bug 103777 showed, this assumption is not always valid. Even when 
some fonts claim to have a glyph, it could happen that the subset of interest 
hasn't been constructed because GenerateMap() doesn't cover all the possible 
charsets. Might be best to revert to correctness here since the savings are not 
really significant (this particular code only gets called in the Win95-Ja 
situation).

=======
The makefile.win hack is a must-fix especially given that gtk doesn't have
the hack. (BTW, do well to elaborate on the errors that you get when you hook 
nsCompressedCharMap.cpp to "gfx/src/makefile.win" in the same way as nsFont.cpp)

Comment 20

[Shanjian Li]

Attachment: patch 6.0, update for latest source tree

Comment 21

[Shanjian Li]

rbs, I update my patch to accomodate your recent change in nsFontMetricsWin.cpp.  
Could you check one more time?  thanks.

Comment 22

[rbs]

I am still seeing loose ends in the handling of the font substitute in the 'A' 
case. C.f. to my comments above.

nsFontWinA::ResolveForwards() will break with the current patch. Indeed, suppose 
that a mLoadedFonts[i] happens to be a nsFontWinSubstituteA, then the code will 
attempt to find the subset of interest for a currchar that was substituted, that 
will fail because the substitute subset hasn't be properly setup and kept in 
sync.

Also, the makefile.win hack (with the strange wrap of '#include 
"..\nsCompressedCharMap.cpp"') is still lurking there.

Comment 23

[Shanjian Li]

Attachment: patch 7.0

Comment 24

[Shanjian Li]

rbs, 
For your first concern, it needs some explaination. In my new code, 
charmap of subset is no longer updatable. In original code, 
  subset->charmap = font->charmap & (charmap of charset)
Since in most situation, before we get down to subset, we already 
know that font->charmap contains that char. nsFontWinSubstituteA 
does give me some problem in this approach. charmap of both substitute 
font and its subsets need to be updated. I ignored to update charmap 
of subsets (because ccmap update is way too expensive), and system will 
fallback to findFont to get it resolved. There might be a performance 
side-impact because of this. So now I put am find subset operations into 
nsFontWinA/nsFontWinSubstituteA. (I was planning to do this later. But as 
you raise the question, I updated my patch. )

As for you second concern, there seems to be no perfect solution yet. 
As suggested by cls, I borrowed existing practice and upate makefile.win. 

Comment 25

[rbs]

I understand what you were aiming at regarding my first concern. It was that you 
were not taking full advantage of the abstraction.

Now that you have added FindSubset(), I took notice that you have wisely added:

+nsFontSubset* 
+nsFontWinSubstituteA::FindSubset(HDC aDC, PRUnichar aChar, nsFontMetricsWinA* 
aFontMetrics)
+{
+  return mSubsets[0];
+}

To complete the trick, you can also add:

nsFontSubsetSubstitute::HasGlyph(aChar) 
{
  return PR_TRUE;
}

With that, you can then get rid of this special-casing in the higher code:

+            if (currFont == mSubstituteFont) {
+              //for substitute font, subset is not updated and can't be checked
+              if (!currFont->HasGlyph(*currChar))
+                break;
             }

Comment 26

[rbs]

+nsFontSubset* 
+nsFontWinA::FindSubset(HDC aDC, PRUnichar aChar, nsFontMetricsWinA* 
aFontMetrics)
+{
+  for (PRUint16 i = 0; i < mSubsetsCount; i++) {
+    if (!mSubsets[i]->mCCMap)
+      mSubsets[i]->Load(aDC, aFontMetrics, this);
+    if (mSubsets[i]->HasGlyph(aChar))
+      return mSubsets[i];
+  }
+  return nsnull;
+}
>>>need to handle the case where the load fails (the 'while' version was 
slightly faster).

Comment 27

[Frank Tang]

P2, this block a lot of work (new OpenType format 12, CJK Extension A, Extension
B, surrogate support, etc)

Comment 28

[Shanjian Li]

Attachment: patch 7.1

Comment 29

[Shanjian Li]

rbs, I updated my patch as you suggested. A "break" in original code wasted me 
almost a whole day. Please take one more look. Thanks.

(substituteFont handling in A-version is a pain, SystemSupportsChar should 
prepare a charmap instead of enumerating each charset for each character. 
I will file a bug later. Would you mind if I assign it to you, or you prefer me 
to handle it? I am afraid my bug count is way too high.) 

Comment 30

[rbs]

+static PRUint16* gUserDefinedCCMap;
>>>init: gUserDefinedCCMap = nsnull;

+nsFontSubset* 
+nsFontWinA::FindSubset(HDC aDC, PRUnichar aChar, nsFontMetricsWinA* 
aFontMetrics)
+{
+  nsFontSubset **subsetPtr = mSubsets;
+  nsFontSubset **endSubset = subsetPtr + mSubsetsCount;
+
+  while (subsetPtr < endSubset) {
+    if (!(*subsetPtr)->mCCMap)
+      if (!(*subsetPtr)->Load(aDC, aFontMetrics, this))
+        return nsnull;
>>>why return early here, maybe the next subset will succeed
>>>and will have the needed glyph
+    if ((*subsetPtr)->HasGlyph(aChar))
+      return (*subsetPtr);
+    subsetPtr++;
>>>nit: FYI, see the "lecture" in bug 78032 about developing an habit
>>>for ++subsetPtr -- although they often optimize to the same thing
+  }
+  return nsnull;
+}
+

=====================
@@ -4504,21 +4409,8 @@
+    if (font && font->HasGlyph(aChar)) {
+      return font->FindSubset(aDC, aChar, this);
>>>replacement for more robustness (keep looking if something
>>>goes wrong with that particular subset):
       nsFontSubset* subset = FindSubset(aDC, aChar, this);
       if (subset)
         return subset;
     }
   }

=======================
@@ -4534,21 +4426,8 @@
+  if (font && font->HasGlyph(aChar)) {
+    return font->FindSubset(aDC, aChar, this);
>>>ditto here

=======================
@@ -4559,25 +4438,18 @@
 {
   for (int i = 0; i < sizeof(gBitToCharset); ++i) {
     PRUint8 charset = gBitToCharset[i];
+    int j = gCharsetToIndex[charset];
+    if (!gCharsetInfo[j].mCCMap) {
+      if (charset == DEFAULT_CHARSET) 
+        continue;
+      if (!HaveConverterFor(charset)) 
+        return 0;
>>>>The two inner |if| only depend on the 'charset', therefore they can be
>>>>factored out as follows:
   for (int i = 0; i < sizeof(gBitToCharset); ++i) {
     PRUint8 charset = gBitToCharset[i];
+    if (charset == DEFAULT_CHARSET) 
+      continue;
+    if (!HaveConverterFor(charset)) 
+      return 0; //>>> BTW, why 'return' here, the previous code did 'continue'
+    int j = gCharsetToIndex[charset];
+    if (!gCharsetInfo[j].mCCMap) {
+      gCharsetInfo[j].mCCMap = gCharsetInfo[j].GenerateMap(&gCharsetInfo[j]);
+      if (!gCharsetInfo[j].mCCMap) 
+          return 0;
     }
+    if (CCMAP_HAS_CHAR(gCharsetInfo[j].mCCMap, aChar)) 
+      return 1;
   }

=========
@@ -4627,19 +4499,9 @@
+      nsFontSubset* subset = font->FindSubset(aDC, c, this);
+      if (subset) {
+        return subset;
>>>ah, that's what I was mentioning above... it looks alright here.

==================
@@ -4659,39 +4531,33 @@
+    nsFontSubset* substituteSubset = 
((nsFontWinA*)mSubstituteFont)->mSubsets[0];
+    ((nsFontWinSubstitute*)mSubstituteFont)->SetRepresentable(aChar);
     return substituteSubset;
>>>nit: maybe you could get rid of the temporary variable and just 
>>>return ((nsFontWinA*)mSubstituteFont)->mSubsets[0]

==================
@@ -4779,65 +4638,40 @@
+            for (; j < i-1; ++j) {
+              // NOTE: we are overwriting variables here (we now *know* that we
+              // will break out of the bigger outer loop after the callback...)
>>>Now that you added FindSubset(), these comments are outdated and can go away
>>>(and the infamous "break" mentioned there became obsolete too, as you saw ;-)

Nice job.

Comment 31

[rbs]

>I will file a bug later. Would you mind if I assign it to you, or you prefer
>me to handle it? I am afraid my bug count is way too high.)

I have filed bug 105592, targeted at Future for the record.

Comment 32

[Shanjian Li]

Attachment: patch 7.2

Comment 33

[rbs]

I still see

+    if (!HaveConverterFor(charset)) 
+      return 0; //>>> BTW, why 'return' here, the previous code did 'continue'

and cases of:

+  if (font && font->HasGlyph(aChar)) {
+    return font->FindSubset(aDC, aChar, this);
   }
which I suggested to replace with:
     subset = font->FindSubset(aDC, aChar, this);
     if (subset) 
        return subset;

BTW, for speedy reviews, it helps to point out what you didn't fix and why.
This way, I can quickly tell whether it is on purpose or whether it is just
an omission.

The reason why I am wondering about the early return is because one may
wish to keep trying when going through the loop of the font-family list, 
"font-family: font1, font2, font3" :

  while (mFontsIndex < mFonts.Count()) {
[...]
+  if (font && font->HasGlyph(aChar)) {
+    return font->FindSubset(aDC, aChar, this);
   }
[...]
}

Comment 34

[Shanjian Li]

Attachment: patch 7.3

Comment 35

[Shanjian Li]

I updated my patch once again. I missed the "HaveConverter" line, that is a 
mistake. The instance of "FindSubset" left there is intentional. That is because 
that call is not within a loop.

Comment 36

[rbs]

Comment on attachment 54303 [details] [diff] [review]
patch 7.3

r=rbs

Comment 37

[kill this account]

Is this being done on a nsCompressedCharMap (C++) object?

> ((nsFontWinSubstitute*)mSubstituteFont)->SetRepresentable(c);

In general, it is not valid to turn on a bit in a PRUint16* compressed char map 
since the map is not fully populated.

Comment 38

[kill this account]

As an option for faster map comparison:

I had considered adding 8 bytes to each map that
contained the char count and a simple checksum of the chars. Comparing
these would quickly eliminate most non matching maps. Somethine like this:

  // offsets to the empty mid and empty page
- #define CCMAP_EMPTY_MID  CCMAP_NUM_UPPER_POINTERS
+ #define CCMAP_COUNT      CCMAP_NUM_UPPER_POINTERS
+ #define CCMAP_CKSUM      CCMAP_COUNT + 4
+ #define CCMAP_EMPTY_MID  CCMAP_CKSUM + 4

Then increment(decrement) the count as chars are added(subtracted) and
add(subtract) the new char to(from) the checksum.

Comment 39

[rbs]

> Is this being done on a nsCompressedCharMap (C++) object?

No.

> > ((nsFontWinSubstitute*)mSubstituteFont)->SetRepresentable(c);
>
>In general, it is not valid to turn on a bit in a PRUint16* compressed char map 
>since the map is not fully populated.

Not a problem here since the substitute font uses a "normal" map. 

Comment 40

[Shanjian Li]

I suggest to leave further optimization of charmap operation to other bugs. 

Comment 41

[Shanjian Li]

Marc, Could you sr this bug?

Comment 42

[Marc Attinasi]

Comment on attachment 54303 [details] [diff] [review]
patch 7.3

rs=attinasi

Comment 43

[Shanjian Li]

fix checked in.

Comment 44

[rbs]

I updated one of my trees to catch up with the tip and your changes. I am seeing 
something strange at: http://www.joins.com/ (a Korean site)

Not sure if it is due to something strange in my tree. In of of my older trees
that are not yet updated to the tip, the page renders fine. Do you see any 
problem at that site?

Comment 45

[rbs]

False alarm. It renders fine after another launch. Maybe something wasn't 
registered properly at my first launch after the build completed.