332649 - [Win] need better font switching mechanism

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, we are switching font when the text segment doesn't has some glyphs even if some glyphs are existing. We should switch only the missing glyph characters.

Comment 1

[Jean-Marc Desperrier]

As I sayed in bug 328940, what you describe here seem exactly like what should *not* be done in order to avoid the "letters cut out of the newspaper" look that has been repeatedly denounced as ugly on earlier versions of Mozilla.

I think there's a need for some serious reflexion about what is the best font selection strategy before any implementation of new fix. We should try to get the most meaningful behaviour by default, but still be configurable.

Bugzilla might not be the best place to do this.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #1)
> As I sayed in bug 328940, what you describe here seem exactly like what should
> *not* be done in order to avoid the "letters cut out of the newspaper" look
> that has been repeatedly denounced as ugly on earlier versions of Mozilla.

Sorry, I cannot understand your comment.
We should do "We want to only use a new font for the glyphs that we are unable to render and use the current font for the rest".(bug 328940 comment 14)

Comment 3

[Jungshik Shin]

Please, don't try to reinvent a wheel. Gfx:Win does a reasonably good job and that should be 'the' starting point. 

Comment 4

[Stuart Parmenter]

the old windows font code may do a good job, but it isn't a good starting point.  the code is impossible to follow and the new code does everything completely different.

We're using Uniscribe now and avoiding CMAPs and such entirely at the moment.  We aren't doing any of the caching that the old code does.

If you have suggestions or patches on how to fix the new code, that would be great, but pointing at the old code isn't very helpful.

Comment 5

[Justin Kerk]

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

Comment 6

[Stuart Parmenter]

font switching is pretty good now, although it doesn't do per-uniscribe-item-cluster font selection like it should.  i'll leave this bug open for that work.

Comment 7

[Justin Kerk]

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

Comment 8

[Justin Kerk]

Seems fixed by bug 370588?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #8)
> Seems fixed by bug 370588?

probably, no.

Comment 10

[Justin Kerk]

Ah, bug 372636 was making it appear to be fixed since the out-of-font characters weren't being displayed at all. So now that that's out of the way this is once again visible.

Comment 12

[j.j.]

Attachment: testcase

from a duplicate bug

Comment 14

[Jean-Marc Desperrier]

About attachement 259241 : It is useful to have a concrete exemple of what effect font switching will have and to help decide what the best boundary is for font switching.

In that case, I'm getting convinced it's a bad decision to replace all the text inside the span. But generally when you tell the browser to use a given font family, and that font family does not contain the required characters, something will break. I'm not sure in general it's bad that what break is the font family requirement, if really required, in exchange for consistent font use in display.
Still in that case the ⇓ character is quite independant from the rest and the effects are definitively too wide.

Comment 15

[Stuart Parmenter]

Per the CSS spec we have to do font fallback o na character by character basis which means we are supposed to change fonts in the middle of words and then go back to the previous font for the rest of it.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

yes. the gfxPangoFonts does it on linux.

# I think that per character may be wrong, we should do it per cluster.
# But we cannot do it on current code. And we may not have the trouble in most cases.

Comment 17

[Jean-Marc Desperrier]

OK Stuart, CSS does specify that in the "Font matching algorithm". Well if that's the spec, then there's little choice. I'd be tempted to suggest to do it per cluster instead of character by character at least when the two fonts are equivalent match for the specified CSS properties, but that might be complex and costly to implement.

Comment 21

[Kent Paul Dolan]

Bug 367639 has been commented upon in its comment
#10 to be closely related to this bug. Do any of the
developers here want to go check if it is or is not
a duplicate? The overstrike behavior is seen, the
"too wide" of this bug's comment #14 may mean what
is seen in bug 367639's comment #5 attachment at
line 3 of the onscreen body text.

Comments on the rest of your discussion here:

Based on my experience as a one time text rendering
software developer, switching fonts character by
character if needed _is_ doable, but you get some
fairly icky problems with line spacing/leading as
X-heights for supposedly "same size" fonts vary,
that you just end up having to endure.

I have no idea how your current code works (nor do I
want to learn), but I found that a five deep
backtracking recursion [for page, paragraph, line,
word, character] sufficed to handle every bad
circumstance I could think up to throw at my then
text rendering engine. That was on 1989 hardware,
and ran a bit slow. On today's hardware you'd
probably never notice processing every character up
to 32 times in the worst case to get a correct final
image.

Quantum valeat.

xanthian.

Comment 23

[Stuart Parmenter]

Attachment: v0.3

first pass at a patch.  this fixes the problem but introduces a bunch of new ones. WIP.

Comment 24

[Stuart Parmenter]

Attachment: v0.4

Comment 25

[Stuart Parmenter]

Attachment: v0.9

Comment 26

[Stuart Parmenter]

this may regress performance a bit on pages that have lots and lots of i18n text that we don't hit pref font searches for since we could possibly be doing more searches through all the fonts on your machine than we did previously.  I can certainly cache previously found fonts from prior character searches and try them first.  This should work well in most cases but may cause some weird results in other cases. (i.e. if say code2000 got selected for the first character but we'd normally render the 2nd-10th characters with a better font)

This patch also wants a followup patch or two to get the other platforms setup with some sort of system font setup as well as move the GetFontAt* gfxFontGroup apis over to ones like I added for windows.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm.. so maybe instead of adding to the list of base fonts, add any fonts selected from the full system list to a list of fonts for whatever unicode range the glyph that we had to search for was in?  Or, better yet, just keep a list of previously used fonts from the full list, and insert them before you search the full list of fonts again.  That way lang pref fonts will still work fine (which is the only thing that could break if you appended them to the base font list, right?)

I haven't looked at the patch in detail, but it seems like we could lazily compute the arrays for GetPrefFontsFor, cache them, and have that function just return a nsTArray<nsRefPtr<FontEntry> > *  (with a typedef helper).. right now it looks like this does the string->gfxFont lookup and conversion each time the lang fonts are requested.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note that we need to render the garbaged text at failing the auto-detect. In this time, some code points may not be valid, so they may not be had in all system fonts.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    virtual gfxFont *GetFontAt(PRInt32 i) {
+        nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(mFonts2[i]->mName, &mStyle);
+        if (!font) {
+            font = new gfxWindowsFont(mFonts2[i]->mName, &mStyle);
+            if (!font)
+                return nsnull;
+            gfxFontCache::GetCache()->AddNew(font);
+

I suspect having GetFontAt cache gfxWindowsFonts in mFonts would be both trivial and worthwhile. We call GetFontAt(0) quite often (e.g. textrun cache).

Why aren't you calling GetOrMakeFont here? You might as well make GetFontAt a non-inline function. 

> mFonts2

I reflexively barf on identifiers with "2" at the end. How about "mFontEntries"?

+        if (list->IndexOf(fe) == list->NoIndex)

How big is this list going to get? This linear search could be slow. I guess it'll be OK though since it seems the list will only contain CSS and pref specified fonts at this stage.

 gfxWindowsFontGroup::~gfxWindowsFontGroup()
 {
+    mFonts2.Clear();
 }

Not needed.

 gfxWindowsFontGroup::InitTextRunGDI(gfxContext *aContext, gfxTextRun *aRun,
                                     const char *aString, PRUint32 aLength)
 {
-    gfxWindowsFont *font = GetFontAt(0);
+    nsRefPtr<gfxWindowsFont> font = GetOrMakeFont(GetFontEntryAt(0), GetStyle());

You don't need to do this, especially if GetFontAt is going to cache fonts in mFonts.

-    gfxWindowsFont *font = GetFontAt(0);
+    nsRefPtr<gfxWindowsFont> font = GetOrMakeFont(GetFontEntryAt(0), GetStyle());

Ditto

+        if (!selectedFont) {
+            /* first check with the script properties to see what they think */
+            const SCRIPT_PROPERTIES *sp = ScriptProperties();
+            if (!sp->fAmbiguousCharSet) {
+                WORD primaryId = PRIMARYLANGID(sp->langid);
+                const char *langGroup = gScriptToText[primaryId].langCode;
+                if (langGroup) {
+                    PR_LOG(gFontLog, PR_LOG_DEBUG, (" - Trying to find fonts for: %s (%s)", langGroup, gScriptToText[primaryId].value));
+
+                    nsTArray<nsRefPtr<FontEntry> > fonts;
+                    this->GetPrefFonts(langGroup, fonts);

Wait ... we're going to do this for every single character that isn't in the main font list? Why aren't we adding these fonts to the main font list?

Ditto for the CJK block... and the FindFontForString block.

+                TextRange r(0,0);

You don't need to initialize the length here, but if you want to, it should be 1.

+                    prevRange.end = (ch <= 65536) ? i : i - 1; // should this be i-1 for ucs4 chars?

I think this is wrong, the test should be ch < 65536. But it would be cleaner to just save the current value of i before we adjust it for surrogates.

+        mRanges[mRanges.Length()-1].end = mLength;

Are we sure that mLength is > 0? If not, this will corrupt memory. Maybe insert an early check for that.

Why not keep the ranges out of the UniscribeItem and instead just create one UniscribeItem per range?

Comment 30

[Stuart Parmenter]

Attachment: v1.0

Comment 31

[Stuart Parmenter]

I believe my latest patch addresses everything in your comments except these:

> +        if (!selectedFont) {
> +            /* first check with the script properties to see what they think
> */
> +            const SCRIPT_PROPERTIES *sp = ScriptProperties();
> +            if (!sp->fAmbiguousCharSet) {
> +                WORD primaryId = PRIMARYLANGID(sp->langid);
> +                const char *langGroup = gScriptToText[primaryId].langCode;
> +                if (langGroup) {
> +                    PR_LOG(gFontLog, PR_LOG_DEBUG, (" - Trying to find fonts
> for: %s (%s)", langGroup, gScriptToText[primaryId].value));
> +
> +                    nsTArray<nsRefPtr<FontEntry> > fonts;
> +                    this->GetPrefFonts(langGroup, fonts);
> 
> Wait ... we're going to do this for every single character that isn't in the
> main font list? Why aren't we adding these fonts to the main font list?
> 

You really want to do it this way otherwise you can end up with some weird looking results.  I have added something that passes in the font that was used for the last range and we will now take a look at it before we do final fallback but after we've done pref lookups.  We could likely cache a lot of the prefs but we should do that in another patch.


> Why not keep the ranges out of the UniscribeItem and instead just create one
> UniscribeItem per range?
> 

I started to do it this way originally but I decided I liked this way better.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> You really want to do it this way otherwise you can end up with some weird
> looking results.

Is that because the language is different for different items and the pref fonts for one language would affect font selection for other items?

> I started to do it this way originally but I decided I liked this way better.

Can you explain why?

Comment 33

[Stuart Parmenter]

(In reply to comment #32)
> > You really want to do it this way otherwise you can end up with some weird
> > looking results.
> 
> Is that because the language is different for different items and the pref
> fonts for one language would affect font selection for other items?
> 
Well, the old code used to copy the list of fonts from the group to the UniscribeItem.  We could certainly do that again, but I don't see the point.  The better thing to do (in another patch) would be to just have a hashtable of langgroups to an array of fontentries so we don't have to do the string junk over and over again.

Comment 34

[Stuart Parmenter]

Attachment: v1.1

...

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 267674 [details] [diff] [review]
v1.1

+        nsRefPtr<gfxWindowsFont> font = GetOrMakeFont(mFontEntries[i], &mStyle);
+        mFonts[i] = font;

Do a direct assignment, it actually saves an addref+release

-    gfxWindowsFont *font = GetFontAt(0);
+    nsRefPtr<gfxWindowsFont> font = NS_STATIC_CAST(gfxWindowsFont*, GetFontAt(0));

Not needed if you change the return type of GetFontAt as we discussed on IRC (and the nsRefPtr part is not needed in any case)

In gfxFontSelectionTest can you add a comment after those UTF8 strings saying what the characters actually are, for those of us who don't read UTF8? :-)