333357 - layout.css.dpi ignored at start, changing after start only changes relative font size

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Andrew Schultz]

With bug 332926 fixed, fonts in cairo builds are not tiny for me (I don't use gnome), but the layout.css.dpi pref is ignored at startup; the UI fonts are a bit too big.  If I change the value after I start (via about:config), I get a corresponding change in the relative size of the UI fonts.  

I start with layout.css.dpi=100 and the fonts are too big
I switch to dpi=90, the fonts are ok.
I quit and restart, the fonts are too big, but dpi=90
I switch to dpi=80, the fonts are ok
I quit and restart, the fonts are too big, but dpi=80

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm... really odd.  The code here looks like it should work...

Comment 2

[Felix Miata]

Was Gecko designed to implement a DPI change without a restart?

Comment 3

[Andrew Schultz]

I also see this under gnome.  Looking at the code, I don't see how anyone on linux with the pref set is /not/ seeing this bug.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/thebes/nsSystemFontsGTK2.cpp&rev=1.9&mark=207,210#176

If "size" is going to be "pixels" on line 210, then line 207 needs to use the DPI specied as layout.css.dpi rather than what GTK claims.  In fact, I don't know why X/GTK should ever be asked for its DPI so long as the pref is set.  nsThebesDeviceContext gets it right, but just uses dpi to calculate pixels/twips conversion.

The effect of the pref is used for is to convert from pixels to twips (line 212).  But since the pixels are wrong, the twips are also wrong.  And if anything uses the twips, it uses the pref DPI to convert back to pixels.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I believe that code is correct.

We're asking for the system font, which might be specified in points or pixels.  In the end, we're then going to try to draw that font in pixels.  So if the system font is specified in points, we want to convert it to pixels according to the concept of DPI in which it's specified in points, and then back to Mozilla's twips (twentieths of a point) using the DPI that Mozilla uses internally.  (Mozilla's planning to switch to pixels internally once the switch to cairo is complete.)

Then, when we draw the font, we convert the twips back to pixels, again using Mozilla's pixels-to-twips, and draw the font.

There are two potential problems here:
 (1) some things that ought to observe changes to layout.css.dpi don't, so while (as long as we're using twips internally rather than some pixel-based unit), the cached system font values (I think that's where the problem is) in Mozilla-twips become incorrect when the pref changes
 (2) if our method of determining what GTK means in pixels when it tells us the system font is a particular value in points is wrong, and the system fonts are in points, then we'll show the system fonts at the wrong size.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The key point there is really that in http://lxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxPangoFonts.cpp we tell the system to draw fonts using pixel sizes (unless you have pango < 1.6 (or is that <= ?), in which case we use the same ratio that we used above).  So if the system gives us a system font in points, and we're going to give it back to the system in pixels, we need to use the system's ratio of points to pixels, not ours.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

A third potential problem, by the way, is rounding errors along the way.  I see one loss of precision in gfxPangoFont::GetMetrics, where I'd probably want to make the change (I didn't compile this):

-    int size;
-    if (FcPatternGetInteger(fcfont->font_pattern, FC_PIXEL_SIZE, 0, &size) != FcResultMatch)
-        size = 12;
-    mMetrics.emHeight = PR_MAX(1, size);
+    double size;
+    if (FcPatternGetDouble(fcfont->font_pattern, FC_PIXEL_SIZE, 0, &size) != FcResultMatch)
+        size = 12.0;
+    mMetrics.emHeight = PR_MAX(1.0, size);

I've also run into issues that repeated conversions lead to floats being slightly too small and then having other code round down; we might need to make a change to match this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/gtk/nsFontMetricsXft.cpp&rev=1.73&mark=1107-1110#1107

And we definitely need to make changes to layout.css.dpi clear cached system fonts -- across platforms -- as long as we're caching system font sizes in twips rather than pixels.  But we might be better off caching them in pixels.

That said, there could easily also be remaining bugs with gfxPlatformGTK::DPI() that we need to ferret out.  Whenever system fonts are reported to us in points and we're working with a new enough pango that we can set the font sizes in pixels, it needs to match exactly the value that the system would use to convert the numbers it gives us (in points) into pixels.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: partial patch to convert nsSystemFont* to pixels (GTK only so far)

This starts the conversion of nsSystemFont* to storing the system fonts in pixels so we don't need to worry about DPI changes.  It converts them to the new API.

I find it annoying that:
 * gfxFontStyle doesn't have the name
 * gfxFontStyle doesn't have a default constructor, so I can't use it in arrays, and I can't construct one easily for use as an out parameter (and having the name separate strongly encourages me to keep it an out parameter)

Comment 8

[Boris Zbarsky [:bzbarsky]]

roc, pav, see gfxFontStyle stuff in comment 7.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(Although, really, name should be an array rather than a string.)

Comment 10

[Stuart Parmenter]

that will happen once i merge gfxFontStyle and gfxFontGroup together.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch to convert nsSystemFont* to pixels

This converts all platforms.  However, I haven't compiled any of the changes for other platforms yet.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch to convert nsSystemFont* to pixels

Oops, attached wrong version of patch.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch to convert nsSystemFont* to pixels

This fixes some typos in the Windows part so that it actually compiles.

This has been compiled and tested on Linux and Windows, and compiled on Mac (but not yet tested -- my build is still running).

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Works on mac, too.

And, for what it's worth, this patch is relatively straightforward.  It converts the nsSystemFonts* classes from storing the system fonts in an nsFont to storing them in an nsString and a gfxFontStyle -- the main difference being that the size is stored in pixels rather than twips.  It then converts that to an nsFont in nsDeviceContextThebes, based on the current mPixelsToTwips for that device context.  (Note that when screen resolution changes, we can end up with some device contexts operating using one DPI and some using another.)

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

OK, the patch is checked in.

I'm pretty sure it'll fix the problems related to weird things happening when you change layout.css.dpi dynamically.

However, there may still be some problems with the initial DPI being wrong.  Could you try out a new build and let me know?

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 244526 [details] [diff] [review]
patch to convert nsSystemFont* to pixels

I'm a little surprised at the magnitude of the codesize change, for what it's worth:

>  firefox-bin
>  	Total:	      +2076 (+4216/-2140)
>  	Code:	      +2014 (+0/+0)
>  	Data:	        +62 (+4216/-2140)
>  	      +2012 (+4152/-2140)	text (DATA)
>  		      +2012 (+4152/-2140)	UNDEF:firefox-bin:text
>  			      +2888	nsSystemFontsGTK2::nsSystemFontsGTK2()
>  			       +504	nsSystemFontsGTK2::GetSystemFont(nsSystemFontID, nsString*, gfxFontStyle*) const
>  			       +449	nsSystemFontsGTK2::GetSystemFontInfo(_GtkWidget*, nsString*, gfxFontStyle*) const
>  			       +311	nsThebesDeviceContext::GetSystemFont(nsSystemFontID, nsFont*) const
>  			         -2	.nosyms.text
>  			        -71	nsSystemFontsGTK2::GetSystemFont(nsSystemFontID, nsFont*) const
>  			       -529	nsSystemFontsGTK2::GetSystemFontInfo(_GtkWidget*, nsFont*, float) const
>  			      -1538	nsSystemFontsGTK2::nsSystemFontsGTK2(float)
>  	        +64 (+64/+0)	rodata (DATA)
>  		        +64 (+64/+0)	UNDEF:firefox-bin:rodata
>  			        +64	.nosyms.rodata

Comment 17

[Andrew Schultz]

seamonkey build 2006-11-04-08-trunk correctly handles changing layout.css.dpi within the session (no UI font size change) and uses the appropriate size after restart.

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

You use the profile manager, don't you?  That must be why it was wrong initially...