432062 - Windows vector font rendering is broken

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tom Kloos]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042609 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050311 Minefield/3.0pre

Recent changes have cause the "vector" fonts supplied with Windows to be displayed with the characters compressed together.   Fortunately these fonts aren't used too often, but IE and Opera get them right!

Reproducible: Always

Steps to Reproduce:
1. Display test case test_fonts_2.html with recent minefield.

Actual Results:  
See attachment ff3_bug_0.png, lines 10 and 11.

Expected Results:  
See attached screen-shot sm0426.png from Seamonkey 26 May 08 tinderbox.  (Current tinderbox Seamonkey is now broken just like Firefox.)

This change appears to have taken place with recent tweaks to fix Type 1 printing and other font problems?  (Type 1 printing is still broken!)

Comment 1

[Tom Kloos]

Attachment: Test case

Comment 2

[Tom Kloos]

Attachment: Display of problem... Lines 10 and 11.

Comment 3

[Tom Kloos]

Attachment: Expected display of lines 10 and 11 from older tinderbox.

Comment 4

[Kai Liu]

It's a recent regression.

Works:  20080429_2037
Broken: 20080429_2221

So... bug 427411.

Comment 5

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

I don't think this should block since the fonts are rarely used and the text is still readable.

Comment 6

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

It's really hard to see how the patch in bug 427411 could have done this.

Comment 7

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

I just made a build with 427411 backed out, and it still showed this bug.

Comment 8

[Kai Liu]

Hmm, that's very odd, then.  The only other checkin during that regression range was bug 429510.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1209526620&maxdate=1209532859

Comment 9

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

Can you post a link to the nightly build where you did not see the bug?

Comment 10

[Kai Liu]

I used the builds at the archive here: <http://hourly-archive.localgho.st/>

20080429_2037_firefox-3.0pre.en-US.win32.zip (works)
20080429_2221_firefox-3.0pre.en-US.win32.zip (broken)

Comment 11

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

> 20080429_2037_firefox-3.0pre.en-US.win32.zip (works)

It's broken in this build for me.

Comment 12

[Kai Liu]

(In reply to comment #11)
> > 20080429_2037_firefox-3.0pre.en-US.win32.zip (works)
> 
> It's broken in this build for me.
> 

I just tried that build with a new profile on two different computers (my main XPSP2 box and a VistaSP1 test install that I have in a VPC), and it rendered correctly both times...

Comment 13

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

Hmm. Anyone else want to try it? :-)

Comment 14

[Ryan A. C.]

20080429_2037 WFM on XP SP2. (though, at first I was clicking the screen shot and wondering why it didn't :P)

Comment 15

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

I strongly feel this should not be a blocker. These fonts are very rarely used because they look crap even if there isn't a spacing bug. They've been obsolete ever since Windows got Truetype support (i.e. 1991 or thereabouts).

Comment 16

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

Attachment: my testcase

This is my testcase.

Comment 17

[Ryan A. C.]

OK, this is strange:
-The first testcase always works if I load it first.
-rocs testcase works only if I load the first testcase in this bug (loading the 2 at the same time via session restore seems to work as long as I look at the first testcase tab first). 
-if I load rocs testcase first then the first one doesn't work.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #10)
> I used the builds at the archive here: <http://hourly-archive.localgho.st/>
> 
> 20080429_2037_firefox-3.0pre.en-US.win32.zip (works)
> 20080429_2221_firefox-3.0pre.en-US.win32.zip (broken)
> 

I get the same result on Vista (without SP1).

Backing out bug 427411's patch fixes the regression.

Comment 19

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

(In reply to comment #17)
> OK, this is strange:
> -The first testcase always works if I load it first.
> -rocs testcase works only if I load the first testcase in this bug (loading the
> 2 at the same time via session restore seems to work as long as I look at the
> first testcase tab first). 
> -if I load rocs testcase first then the first one doesn't work.

OK, I see this too. Thanks for tracking that down.

So it looks like there's a deeper bug here that's being exposed in more situations by the patch in bug 427411. More reason to not block IMHO.

Comment 20

[Kai Liu]

(In reply to comment #15)
> I strongly feel this should not be a blocker. These fonts are very rarely used
> because they look crap even if there isn't a spacing bug. They've been obsolete
> ever since Windows got Truetype support (i.e. 1991 or thereabouts).
> 
In addition to that, Windows vector fonts weren't supported at all in Firefox 2, so this isn't a big loss.

Comment 21

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

Attachment: testcase 3-A

Comment 22

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

Attachment: testcase 3-B

Comment 23

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

Testing using the 20080429_2037_firefox-3.0pre.en-US.win32.zip build: 3-A works, 3-B doesn't. The only difference is that in 3-A the font-family is set on the <span>, in 3-B the font-family is set on the <body>. Disturbing...

Comment 24

[Tom Kloos]

(In reply to comment #20)
> In addition to that, Windows vector fonts weren't supported at all in Firefox
> 2, so this isn't a big loss.
> 

But it works fine in IE7 and Opera (haven't tried Safari yet).  Don't give folks ammunition for dumping Firefox.  At the very least, the characteristics shouldn't change depending upon previously loaded pages.

There are problems with other font rendering too.  "Courier" sometimes renders as the default font (very bad, since the default is proportionally spaced versed fixed -- line 3 should NOT look the same a line 7), or sometimes as garbage characters, or sometimes fine.

See bug 432065 and bug 432071 too.

Comment 25

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

> Don't give folks ammunition for dumping Firefox.

This is silly. You could use this "argument" to make almost any bug a priority.

Comment 26

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

We're initializing the gfxWindowsFont with the same parameters both times.

I tried always calling SetupFont and setting mScriptCache to nsnull before we call ScriptShape and ScriptPlace. That doesn't make the bug go away, which suggests this is not a problem with SCRIPT_CACHE.

I tried forcing the InitTextRunGDI path, where instead of GetGlyphIndices I just set each glyph to the character value. That displays the right glyphs and avoids Uniscribe, but the bug still happens. (In fact, it's worse.)

Stuart, is it possible the text advances are coming back in the wrong units somehow?

Comment 27

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

Is there any font size at which the text advances are correct?  Are you getting advances from a smaller font size, and then some magic scaling on draw?

Comment 28

[Ryan A. C.]

It might be due to the printer, but I just noticed while testing bug 432071 that these fonts print properly (although very light.)

Comment 29

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

I tried making DCFromContext always create a new DC instead of reusing the surface DC (if available) and using the GDI path. That made no difference.

Comment 30

[Ryan A. C.]

Interestingly these fonts had the exact opposite problem in bug 425336, the spacing was too wide except for the numbers and the 'mp' in jumps for some reason.

Comment 31

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

I've been trying various things. I tried to get Uniscribe out of the path, by using GDI with GetTextExtentPointW instead of GetTextExtentPointI. GetTextExtentPointW gives better results than GetTextExtentPointI but it doesn't solve the problem, the characters are still too close together. GetTextExtentPointA doesn't help. I changed cairo-win32-font.c to not use ETO_GLYPH_INDEX for non-Truetype fonts, but that hasn't helped either.

I compared our rendering of Modern against IE's and Notepad's. Turns out we're rendering the glyphs horizontally stretched compared to IE at the same font-size --- the vertical sizes are identical. I think the advances we're getting would be about right for the unstretched glyphs.

Comment 32

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

It seems that this horizontal stretching is due to cairo_win32_scaled_font_select_font performing SetGraphicsMode (hdc, GM_ADVANCED). If I comment that out, plus the _win32_scaled_font_set_world_transform call following (which fails if we don't use GM_ADVANCED), this bug goes away. It's not the call to _win32_scaled_font_set_world_transform, because if I comment out just that call we still have the bug.

Comment 33

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

Attachment: victory

Okay! So the key thing here is that setting GM_ADVANCED on the DC stretches the text horizontally, both for drawing and metrics. Therefore it is essential that we either always have GM_ADVANCED set when drawing or measuring text, or else we never have it set. It's easier to have it always set because that will let us do world-transform effects like rotation and shearing. So this patch establishes the invariant that the DC in a cairo surface is always in GM_ADVANCED mode. We should probably document this somewhere, but I'm not sure where --- any idea, Vlad? We also need to make sure temporary DCs used in gfxWindowsFonts are also in GM_ADVANCED mode.

I think the key offender here was in gfxWindowsFont::ComputeMetrics... I'm not sure, but my theory is that the first DC you use an HFONT with does some font metrics caching that depends on the mode of that DC. If the first thing we did was get metrics, we used the HFONT first with the GetDC() result from ComputeMetrics which was in GM_COMPATIBLE mode. I think this made GDI always return narrow metrics for the HFONT but still draw wide glyphs when the target DC is in GM_ADVANCED mode (which it always is when we're drawing). On the other hand, if the first thing you did with the font was build a textrun, we called down into cairo_win32_scaled_font_select_font which put the DC into GM_ADVANCED mode before we did anything with the font so we got wide metrics and wide glyphs and everything's fine.

So, putting the font style on the block made us compute line-height (requiring font metrics) before building the textruns inside the block, triggering the bad case, whereas putting the font style on the span we were building textruns before font metrics were needed to reflow the span, not triggering badness. Then bug 427411 changed things so we computed metrics eagerly for the first font in the fontgroup, which meant we always hit the bad case.

This patch does mean we still get different rendering from IE. I don't much care since the text is perfectly readable, in fact I think "Modern" looks better when it's wider, and in any case it is definitely a GDI issue and I refuse to care.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Decided not to block on this at the triage during the Firefox meeting.

Comment 35

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

This patch regresses printing. We ain't taking it.

Comment 36

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

Attachment: victory v2

This fixes printing. Very simple ommission.

Comment 37

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

This still breaks printing of native theme stuff. I think we can't rely on our DCs staying in GM_ADVANCED mode. We need to persist with the current approach which sets them to GM_ADVANCED mode just before an operation that needs that mode.

Comment 38

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

Attachment: victory v3

Much safer patch.

This is enough to fix the bug. We don't make assumptions about the DC staying in GM_ADVANCED mode. We just make sure that the extra DCs we get in gfxWindowsFonts are put in GM_ADVANCED mode. This is super-safe.

Comment 39

[Stuart Parmenter]

Comment on attachment 319733 [details] [diff] [review]
victory v3

I'd like to take this.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

mozilla/gfx/thebes/src/gfxWindowsFonts.cpp 	1.206