Closed Bug 418479 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect, P4)

PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: a, Assigned: jtd)

References

Details

(Keywords: regression, testcase)

Attachments

(6 files, 2 obsolete files)

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.
Assignee: joshmoz → jdaggett
Component: GFX: Mac → GFX: Thebes
Priority: -- → P4
Version: unspecified → Trunk
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
Keywords: regression, testcase
QA Contact: mac → thebes
Flags: wanted1.9.0.x+
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
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.
Attachment #312681 - Flags: superreview?(vladimir)
Attachment #312681 - Flags: review?(vladimir)
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.
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...
Vlad, comments?
Depends on: 419715
Thanks for the work on this John!
Since the cairo portions should be fixed by 419715, this includes just the gfx/thebes changes to make things work.
Attachment #312681 - Attachment is obsolete: true
Attachment #312681 - Flags: superreview?(vladimir)
Attachment #312681 - Flags: review?(vladimir)
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.
Attachment #313541 - Flags: superreview?(vladimir)
Attachment #313541 - Flags: review?(vladimir)
Comment on attachment 313541 [details] [diff] [review]
patch, v.0.2, without cairo fixes

Looks good!
Attachment #313541 - Flags: superreview?(vladimir)
Attachment #313541 - Flags: superreview+
Attachment #313541 - Flags: review?(vladimir)
Attachment #313541 - Flags: review+
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.
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.
Attachment #313541 - Attachment is obsolete: true
Attachment #318114 - Flags: superreview?(vladimir)
Attachment #318114 - Flags: review?(vladimir)
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?
Attachment #318114 - Flags: superreview?(vladimir)
Attachment #318114 - Flags: superreview+
Attachment #318114 - Flags: review?(vladimir)
Attachment #318114 - Flags: review+
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.
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.
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 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.
Attachment #318114 - Flags: approval1.9?
Comment on attachment 318114 [details] [diff] [review]
patch, v.0.3, add pref setting, disabled by default

a1.9+=damons
Attachment #318114 - Flags: approval1.9? → approval1.9+
John checked this in yesterday morning: http://tinyurl.com/4n7rns
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Thank you!!!!!!!
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.