418479 - CG glyph rendering always anti-aliases even when system preference is to disable for font size < x

Status: RESOLVED FIXED

(Core :: Graphics, defect, P4)

Description

[Alexander Kellett]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_2; en-us) AppleWebKit/525.9 (KHTML, like Gecko) Version/3.1 Safari/525.9
Build Identifier: Gecko/2008021715 Minefield/3.0b4pre

CoreGraphics text/glyph always renders using anti-aliases even tho I've set my OSX system preferences to only do so for fonts bigger than 8pt. A patch is attached to https://bugzilla.mozilla.org/show_bug.cgi?id=389074 which disables anti-aliasing for a hard-coded set of font sizes. I'm not sure if there are APIs in OSX that automatically use the system preference or if looking them up is required.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Confirming; I say this the other day and meant to file it.

1. Load the to-be-attached testcase in a branch build and Safari, and compare to the trunk.  

2. Check in Appearance in the System Prefs, see what font size you've set in the "Turn off text smoothing for sizes [   ] and smaller."

3. Note that Safari and the branch disable smoothing at N and below (10 in my case) whereas trunk AAs all font sizes.

4. Change the value in the prefs
4a. Switch to Safari, and notice the text has already updated for the new AA threshold.
4b. Switch to the branch, notice the text has not updated, then reload the page and notice the text update for the new AA threshold.
4c. Switch to the trunk and...yeah, nothing ;)

Also make sure your minimum font size in all 3 languages is either off or below your System Prefs AA threshold, or it will confuse the results ;)

We may want to spin live-without-reload changes into another bug, since it's not a regression, but we should still be able to have it apply on next pageload.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: testcase

Comment 3

[John Daggett (:jtd)]

Attachment: patch, v.0.1, disable antialiasing for text rendering based on pref setting

Patch includes a fix to Cairo code used on 10.4.  Based on the setting of 'AppleAntiAliasingThreshold' pref setting, disable antialiased text rendering for text equal to or smaller than the pref setting size.  Sizes assumed to be in pixels, same as Safari.

Comment 4

[John Daggett (:jtd)]

Attachment: screenshot, testcase with patch vs. Safari

Comment 5

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

Comment on attachment 312681 [details] [diff] [review]
patch, v.0.1, disable antialiasing for text rendering based on pref setting

Hmm, I just realized that SetShouldSmoothFonts is public, so we probably want to rework that function somewhat to do better checks for which function pointers are available.

Comment 6

[John Daggett (:jtd)]

The one question I have here is what is the advantage of using the private API call CGContextSetShouldAntialiasFonts, vs. using the public CGContextSetShouldAntialias.  Within the code here we are already doing a context save/restore so it's not quite clear to me what added efficiency this private API call affords unless we are somehow able to avoid the save/restore by manually saving and restoring just the antialias setting state for fonts.

Down the road I think using this setting is going to get hazy in situations where more complex graphics are involved, not just plain vanilla text (e.g. rotated text should probably have antialiasing always enabled).  But I think we can ignore those issues for now...

Comment 7

[John Daggett (:jtd)]

Vlad, comments?

Comment 8

[Alexander Kellett]

Thanks for the work on this John!

Comment 9

[John Daggett (:jtd)]

Attachment: patch, v.0.2, without cairo fixes

Since the cairo portions should be fixed by 419715, this includes just the gfx/thebes changes to make things work.

Comment 10

[John Daggett (:jtd)]

Comment on attachment 313541 [details] [diff] [review]
patch, v.0.2, without cairo fixes

Tested with latest cairo changes.  Note that the pref is only read at startup, probably not a big deal.

Comment 11

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

Comment on attachment 313541 [details] [diff] [review]
patch, v.0.2, without cairo fixes

Looks good!

Comment 12

[John Daggett (:jtd)]

Attachment: testcase, assorted fonts with and without anti-aliasing enabled

Comment 13

[John Daggett (:jtd)]

Attachment: screenshot, showing assorted fonts with and without anti-aliasing enabled

After mulling over this bug, I don't think this is the right behavior for general users, I don't think it will make things more readable in general for smaller text font sizes.  So I'm going to add in a pref setting that will be false by default, this will allow users who prefer the "turn off anti-aliasing at low sizes" to enable it manually.

Comment 14

[John Daggett (:jtd)]

Attachment: patch, v.0.3, add pref setting, disabled by default

One more iteration, this time using a pref setting disabled by default.

Steps to enable:

1. Open about:config
2. Enter gfx. in the filter textbox
3. Toggle the gfx.use_text_smoothing_setting flag to true

Result: after restart, browser will use the Appearance panel setting to disable font smoothing below a certain font size.

Comment 15

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

Comment on attachment 318114 [details] [diff] [review]
patch, v.0.3, add pref setting, disabled by default

Looks fine, but I wonder if we should bother taking it for 1.9 this late given that it'll be turned off by default?

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I think it makes a noticeable reduction in blurriness in the smaller font sizes *that anyone might actually use* (and is certainly more pleasant to my eyes).  

I agree that the very smallest fonts are more legible with smoothing on (where "more legible" = "hurts my eyes, but I can make out the glyph shapes"), but I don't know anyone who's insane enough to try to use (or read) 6px text (other than the Acid2 test).  If you have your minimum font size set to a reasonable value, you don't see those tiny sizes anyway.

Moreover, respecting the platform behavior is the right thing to do, both as a citizen and from the user perspective.  Landing some version of this (either for 1.9, or 1.9.0.1 if you're trying to cut out small landings before 1.9) allows users who want non-blurry small fonts to get that behavior and (the pref version) also allows various Gecko consumers to make the choice to respect the platform behavior by default if their users desire it.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thinking more about this, it occurred to me that prefing the behavior like this implies that users will want AA at small sizes only in their browser and not the rest of the OS, which doesn't make sense to me.  

That is, if you want small fonts AAed, you would set your pref in Appearance very low (or presumably all the way off with a third party utility); you wouldn't depend on each app to disable AA (or AA below a certain size) individually.

Comment 18

[John Daggett (:jtd)]

Attachment: screenshot, testcase with and without subpixel AA

Normally I would completely agree that we should follow platform conventions but I think this particular convention is a bit out of date.  With most Mac users using LCD screens for which text will be rendered with subpixel antialiasing (i.e. text rendering is biased using the LCD RGB subpixel geometries), I don't see a clear win for using the antialiasing setting.  Most of the fonts in the assorted fonts testcase are unreadable below 9px in size in the non-antialiased case.  Above that size there's only marginal difference between the subpixel AA case and the non-antialiased case.

Obviously, some users and other Gecko-based projects (e.g. Camino) may prefer to follow strict rules of platform conformance and have this support enabled by default.  But keep in mind that we now use ATSUI to layout *all* text on the Mac, so *all* text will be subpixel positioned with or without AA on.  To really make text more readable in the non-antialiased case, we'd really need to play around with the advances to push them to pixel boundaries and that's a much bigger effort than what this patch implements.

Comment 19

[John Daggett (:jtd)]

Comment on attachment 318114 [details] [diff] [review]
patch, v.0.3, add pref setting, disabled by default

To allow users to adjust this setting and other Gecko-based projects to enable this by default, I think we should take this patch.  It is low risk without any huge impact.

Comment 20

[Damon Sicore (:damons)]

Comment on attachment 318114 [details] [diff] [review]
patch, v.0.3, add pref setting, disabled by default

a1.9+=damons

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

John checked this in yesterday morning: http://tinyurl.com/4n7rns

Comment 22

[Alexander Kellett]

Thank you!!!!!!!