Closed Bug 401988 Opened 17 years ago Closed 16 years ago

gfxPangoFontGroup::CreateGlyphRunsItemizing must use gfxPangoFont corresponding to the PangoFont of pango_shape (wrong glyphs selected when falling back to fonts of different style)

Categories

(Core :: Graphics, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: karlt, Assigned: karlt)

References

()

Details

(Keywords: testcase)

Attachments

(6 files, 6 obsolete files)

12.41 KB, patch
Details | Diff | Splinter Review
7.31 KB, patch
Details | Diff | Splinter Review
3.46 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.97 KB, patch
pavlov
: review+
Details | Diff | Splinter Review
92.34 KB, text/plain
Details
23.16 KB, image/png
Details
The glyph indices from pango_shape are used with a gfxPangoFont so we need to make sure that these correspond to the same font face.

Currently GfxPangoFontGroup::GetStyle() is used to create the gfxPangoFont but this may not correspond to the PangoFontDescription of the PangoFont used in the pango_shape.  For example if Pango used a PangoFont with a different style because the character could not be found in a font with the same style.

Testcase in URL requires STIX fonts.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Target Milestone: mozilla1.9 M10 → ---
Blocks: 403618
No longer blocks: 403618
In fact, Pango may provide two different PangoFonts with the same PangoFontDescription if there is more than one face with the same family name and style but only a subset of those faces contain glyphs for some code points.
Blocks: 324857
Summary: gfxPangoFontGroup::CreateGlyphRunsItemizing must use gfxPangoFont corresponding to the PangoFont of pango_shape → gfxPangoFontGroup::CreateGlyphRunsItemizing must use gfxPangoFont corresponding to the PangoFont of pango_shape (wrong glyphs selected when falling back to fonts of different style)
The PangoFontDescription and PangoContext were only used to create the PangoFont.
Removing them makes it easier to construct a gfxPangoFont from a PangoFont (and saves some memory).

This also removes the cached map of PangoFontDescriptions to PangoFonts that
grows to hold references to 5000 PangoFonts.  (There are an almost infinite
number of possible sizes in the PangoFontDescription key.)  If we wanted to
keep this map, it should include the language in the key, but I don't think we
want it.

The cache was added in bug 357637, but I believe the real gain was in
maintaining a reference to the PangoFont in the gfxPangoFont.  As
gfxPangoFonts are cached, the cache of PangoFonts should not be required.  If
this second cache provides a benefit then it is an indicator that we should be
increasing the expiration time on the gfxFont cache.

A few other unused methods/classes are also removed.
Attachment #297872 - Flags: review?(pavlov)
Priority: P3 → P2
Comment on attachment 297872 [details] [diff] [review]
don't store PangoFontDescription and PangoContext in gfxPangoFonts and remove gfxPangoFontCache

not directly related to your patch, but since you touched it would you break up the return ifs in CanTakeFastPath in to variables so the function is a little more readable?

otherwise, this looks great.
Attachment #297872 - Flags: review?(pavlov) → review+
patch for check-in.  Includes CanTakeFastPath readability changes and removes a couple of #includes.
Attachment #297872 - Attachment is obsolete: true
Comment on attachment 297902 [details] [diff] [review]
don't store PangoFontDescription and PangoContext in gfxPangoFonts and remove gfxPangoFontCache v1.1

Checked in this patch.
(It doesn't fix the bug; just makes fixing easier.)
Attachment #297902 - Attachment description: don't store PangoFontDescription and PangoContext in gfxPangoFonts and remove gfxPangoFontCache v1.1 → don't store PangoFontDescription and PangoContext in gfxPangoFonts and remove gfxPangoFontCache v1.1 [checked-in]
Attached patch remove gfxPangoFontCache — — Splinter Review
This is what caused a 3% Tp regression, and so was backed out.

Two possible reasons why the gfxPangoFontCache is more effective that the gfxFontCache:

1) The expiration time on the gfxFontCache is not optimal for Tp speed.

2) The key on familyname and nsFontStyle is more specific than the key on
   PangoFontDescription.
   Perhaps, most significantly the size is an integer, whereas the nsFontStyle
   size keys with 3 decimal places.
Attachment #297902 - Attachment is obsolete: true
Attachment #297902 - Attachment description: don't store PangoFontDescription and PangoContext in gfxPangoFonts and remove gfxPangoFontCache v1.1 [checked-in] → don't store PangoFontDescription and PangoContext in gfxPangoFonts and remove gfxPangoFontCache v1.1
>    Perhaps, most significantly the size is an integer, whereas the nsFontStyle
>    size keys with 3 decimal places.

Actually I forgot the PangoFontDescription size is scaled by PANGO_SCALE = 1024, so both caches are similar wrt size.

PangoFontDescription doesn't include language (but the cache should).
The Tp set doesn't seem multilingual.  Perhaps sometimes language is NULL.

Weights might be compressed a bit in the PangoFontDescription but I wouldn't expect that to make a big difference.

It seems that 20-30 seconds expiration time may not be optimal for Tp speed on Linux.
Target Milestone: --- → mozilla1.9 M11
Touch ups to gfxFontCache, (mainly) for gfxFonts that are not accessed through AddNew() and Lookup().  The patch that provides most of the need for this is yet to come.

gfxFonts that increase their ref count from zero need to remove themselves from the expiration tracker (or they would be destroyed).

When destroying a font, only remove the hash table entry if it is the same gfxFont.  (More than one gfxFont can have the same family and style.)

If a new gfxFont replaces an existing entry in the hash table and the old font is removed from the expiration tracker, then the old font should be destroyed.
Attachment #298350 - Flags: review?(roc)
The gfxPangoFont records itself in the PangoFont and removes the record on
destruction (of the gfxFont).

The gfxPangoFontGroup constructor now only constructs one gfxPangoFont,
because that is all that is going to be used.  (The fontconfig generic name "serif" was being resolved to a long list of 78 fonts, so most font groups had this many names).

This constructor now provides more information to fontconfig to select the
font.

Linux users often (usually?) do not have common Postscript fonts (Helvetica, Times, Courier) or Monotype metric-compatibles (Arial, Times New Roman, Courier New) but have other metric-compatible fonts (URW and Liberation fonts).  However, the metric-compatible fonts were not being used because information about the requested family was being removed before the font was selected.

gfxFontconfigUtils was resolving fontconfig generic font names with no
language, which was not going to work well if users had "sans" as their
preferred font for both Chinese and Japanese.

A small fix for font weight is included.  FONT_WEIGHT_NORMAL (400) was being
mapped to 499, which Pango would use to select a Medium weight (500) instead
of Regular (400) (for fonts that had both weights).  I don't know what all the
x49s are for, but I'm leaving that for now.

This patch should fix this bug as well as bug 400578 and maybe bug 404857.
Attachment #298411 - Flags: review?(pavlov)
Attachment #298350 - Attachment description: gfxFontCache changes → gfxFontCache changes [checked-in]
Attachment #297902 - Flags: review+
Comment on attachment 298411 [details] [diff] [review]
associate a gfxFont with a PangoFont [checked-in]

this looks good... I haven't tested it...  might want an extra pair of eyes on it before you land
Attachment #298411 - Flags: superreview?(roc)
Attachment #298411 - Flags: review?(pavlov)
Attachment #298411 - Flags: review+
Comment on attachment 298411 [details] [diff] [review]
associate a gfxFont with a PangoFont [checked-in]

+static GQuark gfxFontQuark()

Call it GetFontQuark? This looks like a gfx type.

+    // Not using g_quark_from_static_string because this module may be
+    // unloaded, so this will be a shutdown leak.

A dangling pointer actually, right?
Attachment #298411 - Flags: superreview?(roc) → superreview+
I have verified that the latest patch resolves the issue I reported in bug 410557.
(In reply to comment #13)
> Will this fix bug 412285?
> 

The testcase attached to that bug renders correctly using my build with this patch applied.
(In reply to comment #16)
> (In reply to comment #13)
> > Will this fix bug 412285?
> > 
> 
> The testcase attached to that bug renders correctly using my build with this
> patch applied.
> 

But then, I just checked, and that testcase renders correctly for me with the beta 2 build.  So perhaps it is just not a very good testcase.
Blocks: 398899
Thanks for looking at this.

(In reply to comment #14)
> (From update of attachment 298411 [details] [diff] [review])
> +static GQuark gfxFontQuark()
> 
> Call it GetFontQuark? This looks like a gfx type.

Sounds good.

> +    // Not using g_quark_from_static_string because this module may be
> +    // unloaded, so this will be a shutdown leak.
> 
> A dangling pointer actually, right?

The wording wasn't clear.  This is better

    // Not using g_quark_from_static_string() because this module may be
    // unloaded (which would leave a dangling pointer).  Using
    // g_quark_from_string() instead, which creates a small shutdown leak.
Could you avoid the leak by putting the quark in gfxPlatformGtk?
(In reply to comment #19)
> Could you avoid the leak by putting the quark in gfxPlatformGtk?

gfxPlatformGtk is also linked into libthebes.so, but, I imagine embedders may wish to unload libxul even so I want to be careful.

The shutdown leak of one small string is very small in the big GTK+ shutdown leaks picture, so I don't think it's worth cluttering the code with a separate case for embedding.

I only added the comment so that someone analysing shutdown leaks could see that the leak is expected.
> I imagine embedders may
> wish to unload libxul even so I want to be careful.

Yes but in that case we call gfxPlatform::Shutdown and delete the gfxPlatformGtk object.
(In reply to comment #21)

The memory for the GQuark is held by glib and I don't think there is a way to release it (from any shutdown function).  Reloading libxul will look for the same GQuark which will have the same pointer which will not point to the right place.
> Reloading libxul will look for the same GQuark which will have the same pointer 
> which will not point to the right place.
if g_quark_from_static_string were used that is.

_static_ GQuark quark is just an optimization.  The code would work the same if "quark" were automatic.
Comment on attachment 298411 [details] [diff] [review]
associate a gfxFont with a PangoFont [checked-in]

1.97        mozilla/gfx/thebes/public/gfxFont.h     
1.61        mozilla/gfx/thebes/public/gfxPangoFonts.h
1.83        mozilla/gfx/thebes/src/gfxFont.cpp      
1.27        mozilla/gfx/thebes/src/gfxOS2Fonts.cpp  
1.126       mozilla/gfx/thebes/src/gfxPangoFonts.cpp
Attachment #298411 - Attachment description: associate a gfxFont with a PangoFont → associate a gfxFont with a PangoFont [checked-in]
Performance result changes after check-in of attachment 298411 [details] [diff] [review]:

perf tests

Tp  249 -> 241
Tp2 183 -> 172
Ts  864 -> 860

talos

tp_loadtime_avg 631 -> 660
ts_avg 1540 -> 1480

Whether this was a win or a loss depends on how we weight the tests.
Do the Linux talos machines have font coverage for all the international pages?
er, why you use lang for generic fonts? I designed the non-using behavior. the current behavior is ugly for us.
Please don't throw the generic family names to pango, it's breaks western and Japanese mixed text. I fixed the issue in bug 339513. pango changes the fonts for each segment of text. I.e., pango might not use same font for same character if they are in different segment.
see the '4' in the screenshot.

Current trunk uses two fonts for '4'. And also 'a'.
If we need to cut the resolving cost for generic fonts, we need to find another way. The current way should be backed out now.
Attachment #299554 - Flags: review?(pavlov)
(In reply to comment #31)
> Created an attachment (id=299554) [details]
> Patch for backing-out the generic font name resolving part

I have verified that this patch does not regress the issue I had in bug 410557.
If we are going to remove non-existant fonts then we shouldn't need to have family names with more than one font.  (We should think of another way to avoid creating a large number of fonts for the font group.)
Attachment #299554 - Attachment is obsolete: true
Attachment #299589 - Flags: review?
Attachment #299554 - Flags: review?(pavlov)
(In reply to comment #27)
> er, why you use lang for generic fonts? I designed the non-using behavior. the
> current behavior is ugly for us.

How does mixed Chinese/Japanese work?  I had understood that the same Unicode code points were used, but the different language enabled different fonts to be used.  Do I understand correctly?

(In reply to comment #28)
> Please don't throw the generic family names to pango, it's breaks western and
> Japanese mixed text. I fixed the issue in bug 339513. pango changes the fonts
> for each segment of text. I.e., pango might not use same font for same
> character if they are in different segment.

I'll look into this.
(In reply to comment #35)
> (In reply to comment #27)
> > er, why you use lang for generic fonts? I designed the non-using behavior. the
> > current behavior is ugly for us.
> 
> How does mixed Chinese/Japanese work?  I had understood that the same Unicode
> code points were used, but the different language enabled different fonts to be
> used.  Do I understand correctly?

Yes. Chinese language and Japanese language share the same code points for many characters. But the glyphs are different between C and J. So, the generic fonts should be specified the language. However, the approach is not best way for us. Because Japanese users hate such rendering. Japanses users like Western characters should be rendered by Western fonts. (Note that UI rendering does so, therefore, we cannot specify the lang to generic fonts for the UI rendering compatibility.)

I think that if we can decide whether a font is for C or for J (or for others), we can fix the issue by changing the order for the font list of a generic font.

Note that ASCII and Japanese mixed text is used by *all* Japanese users, however, CJ mixed text is only needed by a few people. So, Western text rendering is more important than Chinese text for Japanese people.
Attachment #299589 - Flags: review?
(In reply to comment #36)
> Note that ASCII and Japanese mixed text is used by *all* Japanese users,
> however, CJ mixed text is only needed by a few people.

I don't think the issue is only CJ mixed text.
If a font list generated without a language provides fonts suitable for rendering Japanese, then I can't see how the same list could be used for Chinese.

The locale is even removed from the pattern for font list generation, so the only way I can imagine this could have been working for both C & J users is if they had different fonts installed on the system.

There may be more than a few C users with J fonts installed or vice versa.
(In reply to comment #37)
> (In reply to comment #36)
> > Note that ASCII and Japanese mixed text is used by *all* Japanese users,
> > however, CJ mixed text is only needed by a few people.
> 
> I don't think the issue is only CJ mixed text.

yes.

> If a font list generated without a language provides fonts suitable for
> rendering Japanese, then I can't see how the same list could be used for
> Chinese.

I thought that the order of font list depends on system locale. But I didn't check actually.

And I still hope that the backing out patch is landed now (even if it is temporary). Linux nightly testers hope that.
The problem of mixed CJK/Latin text has caused a few headaches over time.  And is discussed in this thread:

http://mail.gnome.org/archives/gtk-i18n-list/2007-December/msg00029.html

The problem with passing generic fonts to Pango is that the generic is resolved to a different font set for substrings containing Latin characters.

I spoke to Behdad about this and he explained that the reason that Pango uses a different lang property in the fontconfig pattern for Latin characters is that a pattern containing lang=ja will not look for fonts containing Latin characters (and so fallback to a number of different fonts may happen).

Behdad has proposed an extension to fontconfig so that it can support configuration of the preferred font for Latin characters when in Japanese text:

http://lists.freedesktop.org/archives/fontconfig/2008-January/002832.html

(Then if users have a good Japanese font with good Latin glyphs then that font can be used for both Japanese and Latin glyphs.  Or other preferences are possible if different fonts are preferred.)

But, until this is implemented, our best workaround it seems is to be careful about what font family names are passed to Pango.
It seems that using fontconfig generic names in font.name.serif.ja et al preferences may not be the best idea given the issue mentioned above.

I don't know the best Japanese font and perhaps that is best left to fontconfig.
We only need to explicitly list Latin fonts to avoid generic matching on Latin/common characters.  The Latin fonts here are the first in the list that would have been provided by fontconfig's default configuration files for a pattern with a Latin language.

The issue this won't resolve is that the Latin fonts will be used for punctuation, but that must have been what was happening before?

I'd appreciate it if nightly testers could try setting these preferences (context menu -> New -> String in about:config) to test their effect.
I don't like this, but this would be a way to quickly restore to the previous behavior wrt mixed Latin/CJK, if there are issues with using the font preferences.
Attachment #299589 - Attachment is obsolete: true
Attachment #299738 - Flags: review?
Attachment #299738 - Flags: review? → review?(pavlov)
Comment on attachment 299738 [details] [diff] [review]
resolve generic fonts to actual fonts with lang-independent pattern

This is not good.
FindGenericFontFromStyle should resolve to actual fonts for consistency, but doing this causes problems with system fonts that are fontconfig generics but not CSS generics so don't get resolved and then get overridden by FindGenericFontFromStyle.
Attachment #299738 - Attachment is obsolete: true
Attachment #299738 - Flags: review?(pavlov)
Comment on attachment 299589 [details] [diff] [review]
Back out part that provided fontconfig with information re requested font even when non-existant.

This is the best backout patch we have now.
Attachment #299589 - Flags: review?(pavlov)
Attachment #299589 - Attachment is obsolete: false
(In reply to comment #38)
> And I still hope that the backing out patch is landed now (even if it is
> temporary). Linux nightly testers hope that.
> 

My nightly testing builds available here:

http://www.wg9s.com/mozilla/firefox/

include Karl's partial backout patch (attachment 299589 [details] [diff] [review]).
(In reply to comment #44)
> My nightly testing builds available here:

They are en-US only though, so that might not help much.
Comment on attachment 299736 [details] [diff] [review]
Include Latin fonts explicitly in CJK preferred fonts

This seems to restore the requested mixed CJK/Latin behavior for me.  Does anyone familiar with CJK scripts see a regression with these preferences?
Attachment #299736 - Flags: review?(pavlov)
(In reply to comment #46)
> (From update of attachment 299736 [details] [diff] [review])
> This seems to restore the requested mixed CJK/Latin behavior for me.  Does
> anyone familiar with CJK scripts see a regression with these preferences?

This patch is not needed. Because font.name-list.* should have fallback fonts from font.name.*. So, if we need to append them, the values should be "serif" or "san-serif" or "monospace".
(In reply to comment #47)

Using attachment 299736 [details] [diff] [review] instead of attachment 299589 [details] [diff] [review] resolves the mixed CJK/Latin issue you reported while maintaining the abilities to select from Chinese and Japanese fonts appropriately and to let fontconfig choose the best font matching the families specified in the document.

> Because font.name-list.* should have fallback fonts
> from font.name.*. So, if we need to append them, the values should be "serif"
> or "san-serif" or "monospace".

I'm not sure I understand what you are saying here.

fontconfig generics ("sans-serif" etc) currently don't support specifying a prefered font for LATIN script in Japanese text.  (See comment 39 and comment 40.)  So we need to specify some non-generic fonts as suggested here:
http://mail.gnome.org/archives/gtk-i18n-list/2007-December/msg00029.html
(In reply to comment #48)
> > Because font.name-list.* should have fallback fonts
> > from font.name.*. So, if we need to append them, the values should be "serif"
> > or "san-serif" or "monospace".
> 
> I'm not sure I understand what you are saying here.

er, we are using the fonts with following order:

1. page author specified (via CSS).
2. font.name.* font
3. font.name-list.* fonts

So, I meant that even if we specify the western fonts to font.name-list.*.ja, they are not used at rendering.

> fontconfig generics ("sans-serif" etc) currently don't support specifying a
> prefered font for LATIN script in Japanese text.  (See comment 39 and comment
> 40.)  So we need to specify some non-generic fonts as suggested here:
> http://mail.gnome.org/archives/gtk-i18n-list/2007-December/msg00029.html

I meant that even if we don't find generics in 1. and 2., the generics in 3. helps us. (The previous our implementation can resolve the generics, ourselves.)
(In reply to comment #49)
> we are using the fonts with following order:
> 
> 1. page author specified (via CSS).
> 2. font.name.* font
> 3. font.name-list.* fonts
> 
> So, I meant that even if we specify the western fonts to font.name-list.*.ja,
> they are not used at rendering.

They are on my system:

% fc-match --sort "serif,Dejavu Sans" | head -n 5
DejaVuSans.ttf: "DejaVu Sans" "Book"
DejaVuSans-ExtraLight.ttf: "DejaVu Sans" "ExtraLight"
DejaVuSans-BoldOblique.ttf: "DejaVu Sans" "Bold Oblique"
DejaVuSerif.ttf: "DejaVu Serif" "Book"
DejaVuSerif-Italic.ttf: "DejaVu Serif" "Italic"

Does your fontconfig configuration behave differently?

> 
> > fontconfig generics ("sans-serif" etc) currently don't support specifying a
> > prefered font for LATIN script in Japanese text.  (See comment 39 and comment
> > 40.)  So we need to specify some non-generic fonts as suggested here:
> > http://mail.gnome.org/archives/gtk-i18n-list/2007-December/msg00029.html
> 
> I meant that even if we don't find generics in 1. and 2., the generics in 3.
> helps us. (The previous our implementation can resolve the generics,
> ourselves.)

We could add the generic names to 3 also, which could be useful if the preference were changed to a non-generic in 2.  (The only reason I didn't have them there is that they weren't there before.)

(But I'm claiming that the previous generic resolution pattern was not doing the right thing.)
(In reply to comment #50)
> Does your fontconfig configuration behave differently?

On my system (Fedora7-ja, default settings)

DejaVuLGCSans.ttf: "DejaVu Sans" "Book"
DejaVuLGCSans-BoldOblique.ttf: "DejaVu Sans" "Bold Oblique"
DejaVuLGCSans-ExtraLight.ttf: "DejaVu Sans" "ExtraLight"
sazanami-mincho.ttf: "Sazanami Mincho" "Regular"
DejaVuLGCSerif.ttf: "DejaVu Serif" "Book"

I think the Western characters are already rendered before 3.
(In reply to comment #51)
> On my system (Fedora7-ja, default settings)
> 
> DejaVuLGCSans.ttf: "DejaVu Sans" "Book"
> DejaVuLGCSans-BoldOblique.ttf: "DejaVu Sans" "Bold Oblique"
> DejaVuLGCSans-ExtraLight.ttf: "DejaVu Sans" "ExtraLight"
> sazanami-mincho.ttf: "Sazanami Mincho" "Regular"
> DejaVuLGCSerif.ttf: "DejaVu Serif" "Book"

Thanks for this.  Looks the same as my system (but locale is different):
non-generics occur earlier in the sorted font list than aliases for generics, even when the generic is specified first.

> I think the Western characters are already rendered before 3.

The will be rendered in 1 if the document specified font supports the character, which is the correct behavior.

They'll only be picked up by 2 if the user changes the preference or if the user has requested something different in their font configuration, in which cases the user will get what they request.
(In reply to comment #52)
> > I think the Western characters are already rendered before 3.
> 
> The will be rendered in 1 if the document specified font supports the
> character, which is the correct behavior.
> 
> They'll only be picked up by 2 if the user changes the preference or if the
> user has requested something different in their font configuration, in which
> cases the user will get what they request.

Great, I understand, thanks.
Attachment #299736 - Flags: review?(pavlov) → review+
Attachment #299736 - Attachment description: Include Latin fonts explicitly in CJK preferred fonts → Include Latin fonts explicitly in CJK preferred fonts [checked-in]
Attachment #299589 - Attachment is obsolete: true
Attachment #299589 - Flags: review?(pavlov)
This is now fixed.  Please open a new bug if there are any other issues.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Depends on: 414649
Depends on: 414388
Depends on: 414692
Blocks: 404857
Could this have caused bug 414239?
Depends on: 414239
Depends on: 416068
Comment on attachment 299736 [details] [diff] [review]
Include Latin fonts explicitly in CJK preferred fonts

Backed this out because it is a hidden pref that can interfere with people's fontconfig settings, and because of the adverse effect on talos page load time (bug 416068).

A possible alternative solution is suggested in bug 416068 comment 6, and users can achieve similar results to this patch if so wished by setting these prefs or with configuration in fontconfig.
Attachment #299736 - Attachment description: Include Latin fonts explicitly in CJK preferred fonts [checked-in] → Include Latin fonts explicitly in CJK preferred fonts
Attachment #299736 - Attachment is obsolete: true
It is also possible to write an extension that consists just of an additional preferences file that sets these preferences.  That might be easier for the users, just install the extension and the preferences get set, then uninstall it or disable it to go back to defaults.
(In reply to comment #58)
> It is also possible to write an extension that consists just of an additional
> preferences file that sets these preferences.  That might be easier for the
> users, just install the extension and the preferences get set, then uninstall
> it or disable it to go back to defaults.
> 
I decided to write such an extension.  It is available at:

http://www.wg9s.com/mozilla/firefox/extensions/cjk-latin/
So was it expected that this patch would change the rendering of a page that has:

  font-family: Verdana, Helvetica, Arial, sans-serif;

?  In particular, before the patch, I get a "right" rendering no matter what combination of those font-families I use ("right" in the sense that an 11px font has an ascent+descent of 15 or less), while after this patch I still see that behavior if I leave out the Helvetica or Arial, but not if I leave them in.

fc-list doesn't show any of Verdana, Helvetica, Arial on this machine, so I wouldn't have expected those family names to affect the rendering in any way whatsoever...
Or is the issue that the fonts.conf aliases are kicking in somehow and mapping those fonts to something totally different?  Would this patch affect how that's handled?
(In reply to comment #61)
> Or is the issue that the fonts.conf aliases are kicking in somehow

Most likely.  This patch would have affected that.

> and mapping those fonts to something totally different?

Hopefully not something totally different, but something closer than the sans-serif default for an empty language.

What does "fc-match Verdana,Helvetica,Arial,sans-serif:pixelsize=11" give you?

Try running that with the "-v" option to find the file, then running ftdump on the file to get the EM size, ascent and descent.

(In reply to comment #60)
> ("right" in the sense that an 11px font has an ascent+descent of 15 or less)

Currently there is nothing that means that an 11px font should have any maximum ascent+descent (but bug 402473 discusses that).

With ft metrics, (ascent - descent)/em_size would need to be greater than 15/11 = 1.36 for your test to produce "wrong" rendering.  I wouldn't normally expect this but, interestingly, if someone on Windows were to use Apple's Helvetica fon t, the ratio would be 1.65.  (Linux and Mac use different metrics from the font.)
> What does "fc-match Verdana,Helvetica,Arial,sans-serif:pixelsize=11" give you?

  fc-match: Command not found.

> Currently there is nothing that means that an 11px font should have any
> maximum

Yeah, that's why I put "right" in quotes.  ;)

As long as it's expected that this would have affected the font aliasing behavior, that explains the behavior change I'm seeing.  Just wanted to make sure that that was expected.
Depends on: 416725
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: