Closed
Bug 661471
Opened 12 years ago
Closed 12 years ago
Create preference(s) to allow specific font families to be forced to use GDI Classic rendering
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
Tracking | Status | |
---|---|---|
firefox5 | - | --- |
People
(Reporter: roc, Assigned: roc)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
Attachments
(12 files, 13 obsolete files)
29.13 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
9.81 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
18.10 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
3.98 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
5.77 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
13.13 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
4.25 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
65.84 KB,
patch
|
jfkthame
:
review+
asa
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
5.86 KB,
patch
|
asa
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
20.67 KB,
image/png
|
Details |
We seem to have some consensus that forcing the classic "core Web fonts" to use GDI Classic rendering with DirectWrite would be better than the status quo.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: nobody → roc
Attachment #536823 -
Flags: review?(jfkthame)
Assignee | ||
Updated•12 years ago
|
Attachment #536823 -
Flags: review?(jdaggett)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #536824 -
Flags: review?(jfkthame)
Assignee | ||
Updated•12 years ago
|
Attachment #536824 -
Flags: review?(jdaggett)
Comment 3•12 years ago
|
||
Comment on attachment 536824 [details] [diff] [review] force GDI Classic for the "core Web fonts" >+pref("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families", >+ "Arial,Andale Mono,Comic Sans MS,Courier New,Impact,Georgia,Trebuchet MS,Verdana"); What about Times New Roman?
Assignee | ||
Comment 4•12 years ago
|
||
Oops, yeah, I missed that one.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #536824 -
Attachment is obsolete: true
Attachment #536824 -
Flags: review?(jfkthame)
Attachment #536824 -
Flags: review?(jdaggett)
Attachment #536836 -
Flags: review?(jfkthame)
Assignee | ||
Updated•12 years ago
|
Attachment #536836 -
Flags: review?(jdaggett)
Comment 6•12 years ago
|
||
And Arial Black isn't covered by Arial, is it? Intentionally left out for other reasons?
Assignee | ||
Comment 7•12 years ago
|
||
I believe that with DirectWrite, Arial Black is part of the Arial family.
Comment 8•12 years ago
|
||
Comment on attachment 536823 [details] [diff] [review] fix Review of attachment 536823 [details] [diff] [review]: ----------------------------------------------------------------- Looks like it should be fine, provided tryserver is happy. I think my only substantive question is the one about unconditionally forcing GDI Classic for DW fonts on the GDI backend - is that really desirable? ::: gfx/cairo/cairo/src/cairo-d2d-private.h @@ +77,5 @@ > typedef struct _cairo_d2d_device cairo_d2d_device_t; > > struct _cairo_d2d_surface { > _cairo_d2d_surface() : d2d_clip(NULL), clipping(false), isDrawing(false), > + textRenderingState(TEXT_RENDERING_UNINITIALIZED) Should use a real tab rather than 8 spaces here, for consistency with the rest of the cairo code. (There are some other instances of this in the other cairo files, too.) ::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp @@ +1108,5 @@ > IDWriteGdiInterop *gdiInterop; > DWriteFactory::Instance()->GetGdiInterop(&gdiInterop); > IDWriteBitmapRenderTarget *rt; > > + IDWriteRenderingParams *params = DWriteFactory::RenderingParams(TRUE); Do we really want to force GDI Classic mode unconditionally here? That seems like it'd prevent non-d2d users ever trying other dwrite modes, which (if they've explicitly turned on dwrite) may be what they actually want. ::: gfx/thebes/gfxDWriteFontList.h @@ +199,5 @@ > nsRefPtr<IDWriteFont> mFont; > nsRefPtr<IDWriteFontFile> mFontFile; > DWRITE_FONT_FACE_TYPE mFaceType; > > + int mIsCJK; Maybe use "short" or even "signed char" here, to reduce the size of the structure slightly? It only needs three values - true (1), false (0), and uninitialized (-1).
Comment 9•12 years ago
|
||
Comment on attachment 536836 [details] [diff] [review] force GDI Classic for the "core Web fonts" You should also include Webdings, I think; it's one of the Core Web Fonts, and shows pretty dramatic differences in rendering between GDI Classic and DW Natural modes.
Attachment #536836 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to comment #8) > ::: gfx/cairo/cairo/src/cairo-d2d-private.h > @@ +77,5 @@ > > typedef struct _cairo_d2d_device cairo_d2d_device_t; > > > > struct _cairo_d2d_surface { > > _cairo_d2d_surface() : d2d_clip(NULL), clipping(false), isDrawing(false), > > + textRenderingState(TEXT_RENDERING_UNINITIALIZED) > > Should use a real tab rather than 8 spaces here, for consistency with the > rest of the cairo code. (There are some other instances of this in the other > cairo files, too.) I don't think this is actually required. I discussed it with Carl Worth a long time ago :-). > ::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp > @@ +1108,5 @@ > > IDWriteGdiInterop *gdiInterop; > > DWriteFactory::Instance()->GetGdiInterop(&gdiInterop); > > IDWriteBitmapRenderTarget *rt; > > > > + IDWriteRenderingParams *params = DWriteFactory::RenderingParams(TRUE); > > Do we really want to force GDI Classic mode unconditionally here? That seems > like it'd prevent non-d2d users ever trying other dwrite modes, which (if > they've explicitly turned on dwrite) may be what they actually want. Good point, will fix. The _dwrite_draw_glyphs_to_gdi_surface_d2d path fails to set RenderingParams at all. I won't fix that in this bug, but I've added an XXX comment pointing out the issue. > ::: gfx/thebes/gfxDWriteFontList.h > @@ +199,5 @@ > > nsRefPtr<IDWriteFont> mFont; > > nsRefPtr<IDWriteFontFile> mFontFile; > > DWRITE_FONT_FACE_TYPE mFaceType; > > > > + int mIsCJK; > > Maybe use "short" or even "signed char" here, to reduce the size of the > structure slightly? It only needs three values - true (1), false (0), and > uninitialized (-1). Made it a PRInt8.
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #536823 -
Attachment is obsolete: true
Attachment #536823 -
Flags: review?(jfkthame)
Attachment #536823 -
Flags: review?(jdaggett)
Attachment #536865 -
Flags: review?(jfkthame)
Assignee | ||
Updated•12 years ago
|
Attachment #536865 -
Flags: review?(jdaggett)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #536866 -
Flags: review?(jdaggett)
Assignee | ||
Comment 13•12 years ago
|
||
I've pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=24a3a50f5d85 Jonathan expects there to be some minor reftest failures (and passes) to paper over.
Updated•12 years ago
|
Attachment #536865 -
Flags: review?(jfkthame) → review+
Comment 14•12 years ago
|
||
(In reply to comment #13) > I've pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=24a3a50f5d85 > > Jonathan expects there to be some minor reftest failures (and passes) to > paper over. For which usable wallpaper can be found in attachment 536575 [details] [diff] [review] at bug 652141 (I expect the issues to be mostly the same).
Assignee | ||
Comment 15•12 years ago
|
||
I'll probably just land this on trunk tomorrow morning.
Whiteboard: [needs landing]
Comment 16•12 years ago
|
||
(In reply to comment #15) > I'll probably just land this on trunk tomorrow morning. John is at the CSS font working group F2F in Japan, can we wait a couple days for him to come back to review the patch first?
Comment 17•12 years ago
|
||
If we want to fix this for Fx5 - which I think we should! - then we need to get it landed pretty soon so as to have a chance of seeking approval for beta and getting into a beta build (as I understand those only happen weekly) before the train is completely out of sight. Another possible reviewer might be Bas, if John's not available.
Comment 18•12 years ago
|
||
Comment on attachment 536865 [details] [diff] [review] fix v2 Review of attachment 536865 [details] [diff] [review]: ----------------------------------------------------------------- + gfxDWriteFontEntry *fe = + static_cast<gfxDWriteFontEntry*>(mFontEntry.get()); + cairo_dwrite_scaled_font_set_force_GDI_classic(mCairoScaledFont, + fe->GetForceGDIClassic()); +DWRITE_MEASURING_MODE +gfxDWriteFont::GetMeasuringMode() +{ + return static_cast<gfxDWriteFontEntry*>(mFontEntry.get())->GetForceGDIClassic() + ? DWRITE_MEASURING_MODE_GDI_CLASSIC + : gfxWindowsPlatform::GetPlatform()->DWriteMeasuringMode(); +} The logic in both these cases should be based on size, there's absolutely no reason to use GDI classic rendering at larger sizes, it only makes the text look jaggy. Something like: if (size < 16px) { use classic } else { use default } Testpage here: http://people.mozilla.org/~jdaggett/tests/cleartype-waterfall.html
Comment 19•12 years ago
|
||
Comment on attachment 536866 [details] [diff] [review] force GDI Classic for the "core Web fonts", v2 +// Currently we apply this setting to all the classic Microsoft "core Web fonts". +pref("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families", + "Andale Mono,Arial,Comic Sans MS,Courier New,Georgia,Impact,Times New Roman,Trebuchet MS,Verdana,Webdings"); I would prefer to limit this to the sans-serif faces that are at the heart of the complaints. The serif faces in this list are actually more readable with subpixel antialiasing at small sizes. I would limit this to: Arial,Courier New,Trebuchet MS,Verdana
Comment 20•12 years ago
|
||
> I would prefer to limit this to the sans-serif faces that are at the
> heart of the complaints. The serif faces in this list are actually more
> readable with subpixel antialiasing at small sizes.
At small sizes, yes. So ideally, shouldn't this be font-size dependent as well? If I remember correctly, DW also improves sans-serif text at small sizes, so I'm not sure why you're drawing a line between serif and sans-serif.
Comment 21•12 years ago
|
||
(In reply to comment #20) > > I would prefer to limit this to the sans-serif faces that are at the > > heart of the complaints. The serif faces in this list are actually more > > readable with subpixel antialiasing at small sizes. > > At small sizes, yes. So ideally, shouldn't this be font-size dependent as > well? If I remember correctly, DW also improves sans-serif text at small > sizes, so I'm not sure why you're drawing a line between serif and > sans-serif. Not exactly, it's the sans-serif fonts that look "crisp" in GDI classic mode because they are placed on pixel boundaries. With serif faces placement on pixel boundaries doesn't assure "crispness" nearly as much. Compare FF4 to Chrome with this testcase: http://people.mozilla.org/~jdaggett/tests/cleartype-waterfall.html
Comment 22•12 years ago
|
||
(In reply to comment #18) > Something like: > > if (size < 16px) { > use classic > } else { > use default > } It's also been suggested that at very small sizes, subpixel positioning will start to look better again as glyph line widths become significantly smaller than a pixel. Given the simplicity of the above, is that something that could be done here or would that take more research?
Comment 23•12 years ago
|
||
(In reply to comment #22) > (In reply to comment #18) > > Something like: > > > > if (size < 16px) { > > use classic > > } else { > > use default > > } > > It's also been suggested that at very small sizes, subpixel positioning will > start to look better again as glyph line widths become significantly smaller > than a pixel. Given the simplicity of the above, is that something that > could be done here or would that take more research? By very small you mean <9px? It's hard to have optimal settings for this range as they will be *very* dependent on the font as to whether it's usable at all at these sizes.
Comment 24•12 years ago
|
||
(In reply to comment #19) > I would limit this to: > > Arial,Courier New,Trebuchet MS,Verdana Sorry, I meant to write: Arial,Courier New,Tahoma,Trebuchet MS,Verdana
Comment 25•12 years ago
|
||
(In reply to comment #23) > By very small you mean <9px? It's hard to have optimal settings for this > range as they will be *very* dependent on the font as to whether it's usable > at all at these sizes. Something like that - I don't remember if an exact size range was suggested, or if it was based on anything substantial. It does stand to reason though that at some point, snapping glyphs to the pixel grid is going to leave them unacceptably deformed - subpixel positioning is no guarantee of readability, but it's better than nothing.
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #536865 -
Attachment is obsolete: true
Attachment #536865 -
Flags: review?(jdaggett)
Attachment #537036 -
Flags: review+
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #537038 -
Flags: review?(jdaggett)
Assignee | ||
Comment 28•12 years ago
|
||
Comment on attachment 537038 [details] [diff] [review] Part 2: Add a pref to limit the forcing of 'GDI Classic' to a maximum font size Whoever gets to it first...
Attachment #537038 -
Flags: review?(jfkthame)
Assignee | ||
Comment 29•12 years ago
|
||
There was a bug in part 1 which caused reftest failures. We weren't forcing "GDI Classic" for @font-face local() fonts.
Attachment #537039 -
Flags: review?(jdaggett)
Assignee | ||
Comment 30•12 years ago
|
||
Comment on attachment 537039 [details] [diff] [review] Part 3: Apply 'GDI Classic' prefs to @font-face local() Whoever gets to it first
Attachment #537039 -
Flags: review?(jfkthame)
Assignee | ||
Comment 31•12 years ago
|
||
This is exactly what John asked for.
Attachment #537040 -
Flags: review+
Assignee | ||
Comment 32•12 years ago
|
||
This is actually Jonathan's patch that I stole and reviewed :-)
Attachment #537041 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #536836 -
Attachment is obsolete: true
Attachment #536836 -
Flags: review?(jdaggett)
Assignee | ||
Updated•12 years ago
|
Attachment #536866 -
Attachment is obsolete: true
Attachment #536866 -
Flags: review?(jdaggett)
Assignee | ||
Comment 33•12 years ago
|
||
New tryserver push: http://tbpl.mozilla.org/?tree=Try&rev=28df502c24ee
Whiteboard: [needs landing]
Comment 34•12 years ago
|
||
Comment on attachment 537040 [details] [diff] [review] Part 4: Force DirectWrite to use 'GDI Classic' rendering for sans-serif 'core Web fonts' of size < 16 pixels +// Currently we apply this setting to the sans-serif Microsoft "core Web fonts". +pref("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families", + "Arial,Courier New,Trebuchet MS,Verdana"); Need Tahoma in the list here.
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #537040 -
Attachment is obsolete: true
Attachment #537064 -
Flags: review?(jdaggett)
Updated•12 years ago
|
Attachment #537039 -
Flags: review?(jdaggett) → review+
Updated•12 years ago
|
Attachment #537064 -
Flags: review?(jdaggett) → review+
Comment 36•12 years ago
|
||
Comment on attachment 537038 [details] [diff] [review] Part 2: Add a pref to limit the forcing of 'GDI Classic' to a maximum font size review+ with changes in part 4v2
Attachment #537038 -
Flags: review?(jdaggett) → review+
Assignee | ||
Comment 37•12 years ago
|
||
Thanks! I'll land this once my tryserver results come in.
Assignee | ||
Comment 38•12 years ago
|
||
Tryserver got a green reftest run, so I checked this in. http://hg.mozilla.org/mozilla-central/rev/79f718b57c49 http://hg.mozilla.org/mozilla-central/rev/c79148810af9 http://hg.mozilla.org/mozilla-central/rev/3ad038dfdd82 http://hg.mozilla.org/mozilla-central/rev/077b3514036d http://hg.mozilla.org/mozilla-central/rev/051b1bd21ae7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 39•12 years ago
|
||
Very shortly we should apply for aurora and beta approval for this bug and for bug 624589 (which this depends on).
Assignee | ||
Comment 40•12 years ago
|
||
Er, bug 642589.
Comment 41•12 years ago
|
||
Perhaps Segoe UI could be added to the list as well, so that text in chrome would be covered? Should a separate bug be filed for that?
Updated•12 years ago
|
tracking-firefox5:
--- → ?
Comment 42•12 years ago
|
||
Do the font sizes in pixel here refer to what the designer specifies or to how it is in fact displayed, ie. after zooming and such?
Comment 43•12 years ago
|
||
(In reply to comment #42) > Do the font sizes in pixel here refer to what the designer specifies or to > how it is in fact displayed, ie. after zooming and such? The font sizes here are at the graphics layer level so they're all in real pixels (i.e. after zoom). So zoom = 200% + font-size: 8px == 16px at the graphics layer.
Comment 44•12 years ago
|
||
OK, that's good.
Comment 45•12 years ago
|
||
(In reply to comment #41) > Perhaps Segoe UI could be added to the list as well, so that text in chrome > would be covered? Should a separate bug be filed for that? Filed bug 661869.
Updated•12 years ago
|
Attachment #537038 -
Flags: review?(jfkthame)
Updated•12 years ago
|
Attachment #537039 -
Flags: review?(jfkthame)
Comment 46•12 years ago
|
||
I believe it's due to this bug so I will say it here, tell me if I need to file a new bug: This new pref seems to break about everything when Cleartype is turned off: http://tinyurl.com/6gfkgu7 And even more when we use font implied before for the UI (Tahoma used here instead of the default SegoeUI): http://tinyurl.com/6xqc99g Turning off hardware acceleration of course resolve the problem.
Comment 47•12 years ago
|
||
(In reply to comment #46) > This new pref seems to break about everything when Cleartype is turned off: Please see bug 662115. The patches in this bug did not create the problem that you describe, since that problem pre-dates this bug. It merely exposed a problem that already existed.
Assignee | ||
Comment 48•12 years ago
|
||
The problem is that the changes in this bug make things break with default prefs and Cleartype disabled. This patch fixes that. It doesn't perfectly handle disabling of Cleartype without restarting the browser, but we already don't handle that perfectly, and I think that's OK.
Attachment #537479 -
Flags: review?(jfkthame)
Comment 49•12 years ago
|
||
Comment on attachment 537479 [details] [diff] [review] Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides Review of attachment 537479 [details] [diff] [review]: ----------------------------------------------------------------- If ClearType is disabled, we should probably use DirectWrite's default rendering parameters - i.e. from IDWriteFactory::CreateRenderingParams - rather than our mRenderingParams, which will still fail if the cleartype_params prefs are set inappropriately. I don't have time to test it tonight, but will try this out tomorrow (if you don't get to it and post an update before then).
Assignee | ||
Comment 50•12 years ago
|
||
is this what you want?
Attachment #537492 -
Flags: review?(jfkthame)
Comment 51•12 years ago
|
||
I was hoping we could do something along these lines, and actually handle the case where font smoothing is disabled (or enabled) while the browser is running; I don't like the fact that it's currently possible to end up in a state where basically no text gets drawn. (E.g., with your "alternative" patch, go to the system control panel and disable smoothing, then zoom the browser window to enlarge text: virtually all of it disappears.) However, this version still doesn't handle all situations correctly, so some further followup is needed if we're going to handle on-the-fly changes.
Attachment #537525 -
Flags: feedback?(roc)
Assignee | ||
Comment 52•12 years ago
|
||
I wrote a patch like this, but it still doesn't handle dynamic changes properly because RenderingParams() doesn't get called very often (which is a good thing!). Surfaces initialized while Cleartype is enabled continue to use the old settings after Cleartype is disabled. Personally I don't see that handling dynamic Cleartype changes cleanly without restarting should be a high priority compared to the big issues we're grappling with here.
Assignee | ||
Comment 53•12 years ago
|
||
Comment on attachment 537525 [details] [diff] [review] part 6- another alternative approach Review of attachment 537525 [details] [diff] [review]: ----------------------------------------------------------------- So I think this as acceptable as the others.
Attachment #537525 -
Flags: feedback?(roc) → feedback+
Comment 54•12 years ago
|
||
Comment on attachment 537492 [details] [diff] [review] Part 6 alternative OK, I suggest we take this patch for now, as the simplest/cleanest solution that should fix the basic issue for users with ClearType disabled. I'd really like to handle dynamic changes without getting into states with missing text - after all, the rest of the Windows environment responds immediately to changes in font-smoothing settings - but we could do that as a followup bug; it's more urgent to fix the 'static' issue here.
Attachment #537492 -
Flags: review?(jfkthame) → review+
Updated•12 years ago
|
Assignee | ||
Comment 55•12 years ago
|
||
Attachment #537716 -
Flags: review?(jfkthame)
Assignee | ||
Comment 56•12 years ago
|
||
I believe this is what you ordered :-). With this patch, changes to the Cleartype setting are reflected immediately in all browser windows. The patch ensures that all gfxFonts are recreated when the Cleartype setting changes. So when Cleartype is disabled, the antialias_mode in every cairo_dwrite_scaled_font_t defaults to something other than CAIRO_ANTIALIAS_SUBPIXEL. This patch ignores attempts to set "GDI classic" mode for fonts that don't have CAIRO_ANTIALIAS_SUBPIXEL.
Attachment #537479 -
Attachment is obsolete: true
Attachment #537492 -
Attachment is obsolete: true
Attachment #537525 -
Attachment is obsolete: true
Attachment #537479 -
Flags: review?(jfkthame)
Attachment #537717 -
Flags: review?
Assignee | ||
Comment 57•12 years ago
|
||
Actually, here's just the part that handles dynamic changes to the Cleartype setting.
Attachment #537718 -
Flags: review?(jfkthame)
Assignee | ||
Comment 58•12 years ago
|
||
... and with a crash fix if you happen to change the Cleartype setting just as a page is being torn down.
Attachment #537717 -
Attachment is obsolete: true
Attachment #537718 -
Attachment is obsolete: true
Attachment #537717 -
Flags: review?
Attachment #537718 -
Flags: review?(jfkthame)
Attachment #537722 -
Flags: review?(jfkthame)
Assignee | ||
Comment 59•12 years ago
|
||
Attachment #537724 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #537724 -
Flags: review? → review?(jfkthame)
Assignee | ||
Comment 60•12 years ago
|
||
I think this fixes the issue John raised.
Attachment #537725 -
Flags: review?(jfkthame)
Comment 61•12 years ago
|
||
Parts 6.1 - 6.3 definitely make things better, but we still have the issue that if you turn off font smoothing at the system level while the rendering_mode is explicitly set, most text disappears.
Comment 62•12 years ago
|
||
Here's an alternative implementation of part 6.3 (based on the earlier patch that created distinct "custom", "gdi classic" and "default" rendering params); I think this fixes the disappearing text issues, by ensuring we use DW's default parameters instead of our custom ClearType params if CT is disabled.
Attachment #537873 -
Flags: review?(roc)
Comment 63•12 years ago
|
||
Comment on attachment 537716 [details] [diff] [review] Part 6.1: Expose cairo_win32_get_system_text_quality Review of attachment 537716 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #537716 -
Flags: review?(jfkthame) → review+
Comment 64•12 years ago
|
||
Comment on attachment 537722 [details] [diff] [review] Part 6.2 v3 Review of attachment 537722 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresContext.cpp @@ +776,3 @@ > { > + if (!mShell) > + return; style guide calls for {braces} ::: widget/src/windows/nsWindow.cpp @@ +4470,5 @@ > + Preferences::GetBool(kPrefName, PR_FALSE); > + Preferences::SetBool(kPrefName, !fontInternalChange); > +} > + > +static PRBool CleartypeSettingChanged() ClearType normally gets an uppercase T, I think. @@ +4836,5 @@ > + ForceFontUpdate(); > + gfxFontCache *fc = gfxFontCache::GetCache(); > + if (fc) { > + fc->Flush(); > + } Do we need to explicitly flush the textrun cache as well, to make sure textruns will be created afresh (with possibly different metrics)?
Assignee | ||
Comment 65•12 years ago
|
||
(In reply to comment #61) > Parts 6.1 - 6.3 definitely make things better, but we still have the issue > that if you turn off font smoothing at the system level while the > rendering_mode is explicitly set, most text disappears. That's what part 7 is about.
Assignee | ||
Comment 66•12 years ago
|
||
Oh, I see what you mean. You want the rendering mode that forces Cleartype to still render something when Cleartype is disabled. I don't care *too* much about that myself...
Assignee | ||
Comment 67•12 years ago
|
||
(In reply to comment #64) > ::: layout/base/nsPresContext.cpp > @@ +776,3 @@ > > { > > + if (!mShell) > > + return; > > style guide calls for {braces} I don't think it does around "return", "break" and "continue". > ::: widget/src/windows/nsWindow.cpp > @@ +4470,5 @@ > > + Preferences::GetBool(kPrefName, PR_FALSE); > > + Preferences::SetBool(kPrefName, !fontInternalChange); > > +} > > + > > +static PRBool CleartypeSettingChanged() > > ClearType normally gets an uppercase T, I think. OK. > @@ +4836,5 @@ > > + ForceFontUpdate(); > > + gfxFontCache *fc = gfxFontCache::GetCache(); > > + if (fc) { > > + fc->Flush(); > > + } > > Do we need to explicitly flush the textrun cache as well, to make sure > textruns will be created afresh (with possibly different metrics)? gfxTextRunWordCache observes pref changes to "font.*". gfxTextRunCache does need to be flushed, but currently nothing flushes it even for font changes for which it definitely should be flushed already! I will fix that in a separate patch.
Assignee | ||
Comment 68•12 years ago
|
||
Comment on attachment 537873 [details] [diff] [review] part 6.3 alternative - handle rendering modes and cleartype setting more carefully Review of attachment 537873 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #537873 -
Flags: review?(roc) → review+
Assignee | ||
Comment 69•12 years ago
|
||
Comment on attachment 537724 [details] [diff] [review] Part 6.3: Allow force_gdi_classic only if Cleartype is enabled Obsoleted by Jonathan's part 6.3
Attachment #537724 -
Attachment is obsolete: true
Attachment #537724 -
Flags: review?(jfkthame)
Assignee | ||
Comment 70•12 years ago
|
||
Comment on attachment 537725 [details] [diff] [review] Part 7: Only use force_gdi_classic_for_families when rendering_mode parameter is -1 Obsoleted by Jonathan's part 6.3
Attachment #537725 -
Attachment is obsolete: true
Attachment #537725 -
Flags: review?(jfkthame)
Comment 71•12 years ago
|
||
Comment on attachment 537873 [details] [diff] [review] part 6.3 alternative - handle rendering modes and cleartype setting more carefully > DWRITE_RENDERING_MODE renderingMode = > mRenderingMode >= DWRITE_RENDERING_MODE_DEFAULT && mRenderingMode <= DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC ? >- (DWRITE_RENDERING_MODE)mRenderingMode : defaultParams->GetRenderingMode(); >+ (DWRITE_RENDERING_MODE)mRenderingMode : DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL; I think that should be mDefaultRenderingParams->GetRenderingMode() instead of DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL. A rendering mode initializes to a default of DWRITE_RENDERING_MODE_DEFAULT, which happens to render like DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL under most circumstances, but the two are not technically equivalent.
Comment 72•12 years ago
|
||
(In reply to comment #71) > Comment on attachment 537873 [details] [diff] [review] [review] > part 6.3 alternative - handle rendering modes and cleartype setting more > carefully > > > DWRITE_RENDERING_MODE renderingMode = > > mRenderingMode >= DWRITE_RENDERING_MODE_DEFAULT && mRenderingMode <= DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC ? > >- (DWRITE_RENDERING_MODE)mRenderingMode : defaultParams->GetRenderingMode(); > >+ (DWRITE_RENDERING_MODE)mRenderingMode : DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL; > > I think that should be mDefaultRenderingParams->GetRenderingMode() instead > of DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL. A rendering mode initializes to > a default of DWRITE_RENDERING_MODE_DEFAULT, which happens to render like > DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL under most circumstances, but the > two are not technically equivalent. Actually, DWRITE_RENDERING_MODE_DEFAULT maps to a mode based on the size used: http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-d2d-surface.cpp#3630 But I agree with your point, setting DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL for out-of-bounds parameters doesn't make sense here.
Comment 73•12 years ago
|
||
There's really a lot of patch here. My recommendation is to let the full set of patches bake on mozilla-central for a little while (a week?) before pushing the whole lot to Aurora.
Comment 74•12 years ago
|
||
(In reply to comment #70) > Comment on attachment 537725 [details] [diff] [review] [review] > Part 7: Only use force_gdi_classic_for_families when rendering_mode > parameter is -1 > > Obsoleted by Jonathan's part 6.3 I don't think part 7 is obsoleted by that patch; at least it wasn't expected to be. We still need an additional change to make sure that an explicitly-set mode overrides the GDI Classic setting for fonts in the force_classic list. But AFAICT the part 7 patch didn't fully work; if I set the render_mode pref to 5, it looked like it was correctly using Natural glyphs for these fonts, but laying out with GDI metrics, resulting in irregular spacing. So I think we still want a version of Part 7, but it needs further work...
Comment 75•12 years ago
|
||
(In reply to comment #67) > > style guide calls for {braces} > > I don't think it does around "return", "break" and "continue". AFAICS, there's no such exception mentioned, though I know lots of existing code omits them. I'll leave it to your judgment... there are better things to spend our time on! > gfxTextRunWordCache observes pref changes to "font.*". gfxTextRunCache does > need to be flushed, but currently nothing flushes it even for font changes > for which it definitely should be flushed already! I will fix that in a > separate patch. OK, thanks.
Updated•12 years ago
|
Attachment #537722 -
Flags: review?(jfkthame) → review+
Comment 76•12 years ago
|
||
Updated version of part 7 - I think this handles the interaction now, so that the force-GDI list only applies if the user has not explicitly forced a rendering mode in the prefs.
Attachment #537969 -
Flags: review?(roc)
Assignee | ||
Comment 77•12 years ago
|
||
Comment on attachment 537969 [details] [diff] [review] part 7: only use force_gdi_classic if an explicit rendering mode has not been set Review of attachment 537969 [details] [diff] [review]: ----------------------------------------------------------------- Can you land this lot? :-)
Attachment #537969 -
Flags: review?(roc) → review+
Assignee | ||
Comment 78•12 years ago
|
||
Comment on attachment 537969 [details] [diff] [review] part 7: only use force_gdi_classic if an explicit rendering mode has not been set Review of attachment 537969 [details] [diff] [review]: ----------------------------------------------------------------- Can you land this lot? :-)
Comment 79•12 years ago
|
||
(In reply to comment #78) > Can you land this lot? :-) OK - to be on the safe side, I'll push them to try this evening, then I can land them tomorrow morning my time.
Assignee | ||
Comment 80•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/63c8f645cc1c http://hg.mozilla.org/integration/mozilla-inbound/rev/3eb99af46b93 http://hg.mozilla.org/integration/mozilla-inbound/rev/1ccc676a3a2c http://hg.mozilla.org/integration/mozilla-inbound/rev/5607cca3f7bf
Whiteboard: [inbound]
Comment 81•12 years ago
|
||
Argh - I just pushed these to mozilla-central without realizing you'd landed them on mozilla-inbound .... somehow missed the relevant bugmail. Apologies to whoever handles the m-i -> m-c merge. :( There will be a couple of minor conflicts, as I tweaked the comments in all.js and also addressed comments #71-72 here. http://hg.mozilla.org/mozilla-central/rev/3329c9d9a4ae (pt 6.1) http://hg.mozilla.org/mozilla-central/rev/8524ae3117e4 (pt 6.2) http://hg.mozilla.org/mozilla-central/rev/91c6afbd9046 (pt 6.3) http://hg.mozilla.org/mozilla-central/rev/5795ed49996d (pt 7)
Assignee | ||
Comment 82•12 years ago
|
||
You should probably back them out of mozilla-inbound.
Comment 83•12 years ago
|
||
OK, backed out all 4 changesets in comment #80 from mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/f0472d3bf28a AFAICS this should allow the inbound->central merge to proceed harmlessly.
Whiteboard: [inbound]
Comment 84•12 years ago
|
||
So after landing of pt. 7 it's not possible to use 'natural symmetric' (gfx.font_rendering.cleartype_params.rendering_mode = 5) and retain the ability to force GDI classic rendering for some font families?
Comment 85•12 years ago
|
||
(In reply to comment #84) > So after landing of pt. 7 it's not possible to use 'natural symmetric' > (gfx.font_rendering.cleartype_params.rendering_mode = 5) and retain the > ability to force GDI classic rendering for some font families? That's true. I think (IIRC) that the default behavior (with gfx.font_rendering.cleartype_params.rendering_mode = -1) will be to use natural symmetric for text above a certain size, in accordance with MS recommendations. So it doesn't seem like there should be much need to explicitly select it.
Comment 86•12 years ago
|
||
AFAIK it's only for fonts sizes over 16ppem, and i prefer natural symmetric for all sizes because at smaller sizes 'natural' rendering is either too light or, if turn up the contrast, too jagged (especially obvious on letters like S). Should i file a new bug asking for ability to change rendering mode AND to force GDI classic rendering for some font families or the chances of it getting fixed are close to none so it's pointless?
Comment 87•12 years ago
|
||
I don't know what the chances would be..... might depend how complex it makes things. How would you suggest that should be specified in prefs?
Comment 88•12 years ago
|
||
Revert it to not care about whether gfx.font_rendering.cleartype_params.rendering_mode is set to -1 and introduce an separate setting for turning it on/off?
Comment 89•12 years ago
|
||
(In reply to comment #88) > introduce an separate setting for turning it on/off? You can just set force_gdi_classic_max_size to 0 (or clear out the font list) to turn off the GDI classic fallback.
Comment 90•12 years ago
|
||
I agree with John Bez that some fonts are rendered better in natural_symmetric even in small size. That is why some of us explicitly set gfx.font_rendering.cleartype_params.rendering_mode to 5 instead of leaving it to -1 because we want to have natural_symmetric at all font sizes for those fonts, and set force_gdi_classic_for_families to use GDI_classic for fonts that suit. Part 7 of this patch is breaking this practice. Bug 652141 is discussing changing the default setting for gfx.font_rendering.cleartype_params.rendering_mode to GDI classic. So either bug 652141 is a WONTFIX, or part 7 of this patch is redundant.
Comment 91•12 years ago
|
||
(In reply to comment #87) > I don't know what the chances would be..... might depend how complex it > makes things. How would you suggest that should be specified in prefs? Baboo and Kai Liu already said it all. The easiest solution would be to backout pt. 7, because anyone who knows how to change rendering mode will also know how to clear out gfx.font_rendering.cleartype_params.force_gdi_classic_for_families if he or she wants all fonts rendered the same. A little harder solution would be to add pref for forcing forcing GDI classic for certain font families, something like boolean gfx.font_rendering.cleartype_params.force_force_gdi_classic_for_families :)
Assignee | ||
Comment 92•12 years ago
|
||
I'll leave it up to John Daggett to decide whether we want patch 7 or not.
Comment 93•12 years ago
|
||
Two more fonts to consider for the fallback list: 1) Microsoft Sans Serif -- This is the default font used for form controls (it's what MS Shell Dlg maps to). Since it is an adaptation of the old MS Sans Serif raster font, a GDI-style rendering is arguably closest to its "intended" appearance. (And it looks better that way, too.) 2) Consolas -- It is somewhat widely used as a code font (developer.mozilla.org's fancy code formatting thing uses it, and so does the MSDN library). The fuzzier appearance of the natural rendering of this font at 10pt (esp. with color) was one of the reasons Microsoft backed down on using natural in VS2010. Also, as a monospace font, the positioning fidelity of natural seems less relevant. (I could file a new bug if that's preferred, but I got the impression from the recent round of patches is that the preference is to keep all the related stuff consolidated together in this bug.)
Assignee | ||
Comment 94•12 years ago
|
||
Let's not add anything more to this bug. It was my mistake to do that in the first place.
Comment 95•12 years ago
|
||
(In reply to comment #94) > Let's not add anything more to this bug. Okay, I filed that request as bug 664055.
Comment 96•12 years ago
|
||
Any news from John Daggett on patch 7?
Comment 97•12 years ago
|
||
This is a roll-up of parts 1 to 6.3, merged to the current tip of mozilla-aurora ready for landing there. (Carrying forward r+ from the m-c patches.) I'm keeping part 7 separate as there's a pending question of whether we really want to keep this, or revert to the behavior we had without it.
Attachment #539812 -
Flags: review+
Attachment #539812 -
Flags: approval-mozilla-aurora?
Comment 98•12 years ago
|
||
Part 7 for aurora, if we decide to keep it (see comments #84 and following). The alternative is to back it out on central - we shouldn't leave the two with different behavior.
Attachment #539814 -
Flags: approval-mozilla-aurora?
Comment 99•12 years ago
|
||
Jonathan, have you done a crash-stats and input check to make sure that these patches haven't caused any regressions?
Comment 100•12 years ago
|
||
I don't see any evidence in crash-stats (looking at the topcrashes for the past week on 7.0a1) that these have had any effect (positive or negative) there. I did notice someone on input.mozilla.com remarked that fonts are better on nightlies, which is what we'd expect; otherwise, just the usual complaints about blurriness (referring to the DW rendering in release versions, I assume). So in short, I didn't see any indication of regressions. (My searching may not be particularly expert, however!)
Comment 101•12 years ago
|
||
Asa, can you weigh in on this form a product standpoint? This seems like too risky a change to take 1/2 way through aurora but if product concerns trump the risk we can look to take it.
Comment 102•12 years ago
|
||
(In reply to comment #92) > I'll leave it up to John Daggett to decide whether we want patch 7 or not. One problem I see now is that changing the two force_xxx prefs still don't appear to be live while the rendering mode pref is. This is confusing and it makes it hard to test differences in rendering without restarting. So I guess I would be fine with allowing the force_gdi prefs to override the rendering mode pref as long as the force_xxx prefs were live. But that should also probably happen in a separate bug.
Comment 103•12 years ago
|
||
(In reply to comment #101) > Asa, can you weigh in on this form a product standpoint? This seems like too > risky a change to take 1/2 way through aurora but if product concerns trump > the risk we can look to take it. In Firefox 4, we introduced what is seen as a major regression in font rendering by many (in the form of DirectWrite's natural (symmetric) rendering mode); specifically, they think it looks blurry and/or lighter. We know this has been a major issue for our users because it's come up a lot on SUMO, especially when Firefox 4 was first released. This series of patches fixes the major cause of that regression by using DirectWrite's "GDI classic" rendering mode for the common web fonts at small sizes, where DirectWrite's "blurriness" is most apparent. If we wait for Firefox 7 for these patches, we're worried that we're going to bleed lots of users to alternate browsers. This is already an issue, obviously, but the longer it remains unresolved, the more users we'll lose. I hope this is an adequate explanation of the issue!
Comment 104•12 years ago
|
||
Sorry I didn't get to this sooner. I think this is a pretty serious regression for quite a few Windows users. I'd hoped when we shipped 4 that people would be more accepting of the change because IE and other Windows software sees some of the same problems. Unfortunately that wasn't the case and there's been quite a bit of unrest. The only question I have, given how late in the game we are here, is whether it would be safer to take a "flip all fonts" pref rather than introducing code to only change it for certain fonts at certain sizes. I think we should certainly do something, either theses patches for selective falling back to GDI or a patch to flip all fonts back.
Comment 105•12 years ago
|
||
(In reply to comment #104) > Sorry I didn't get to this sooner. I think this is a pretty serious > regression for quite a few Windows users. I'd hoped when we shipped 4 that > people would be more accepting of the change because IE and other Windows > software sees some of the same problems. Unfortunately that wasn't the case > and there's been quite a bit of unrest. > > The only question I have, given how late in the game we are here, is whether > it would be safer to take a "flip all fonts" pref rather than introducing > code to only change it for certain fonts at certain sizes. I think we should > certainly do something, either theses patches for selective falling back to > GDI or a patch to flip all fonts back. The latter option was filed as bug 652141. I posted a trivial patch there to flip the default value of the rendering-mode pref, but it was not accepted (see comments 28 and following).
Comment 106•12 years ago
|
||
And no reason was given why this patch can't land. Only some misleading facts like dev team fight and some devs pushing new font rendering... Also I see no reason, why we still keeping default rendering as Natural, because 99,9% of web is using fonts in size about 8-14 and in this sizes the best results (sharp and clear) provide only GDI Classic. Even internal interface look better, look on screen shots posted in this bug #652141
Comment 107•12 years ago
|
||
(In reply to comment #106) Virtual_ManPL, I appreciate your passion about this issue but your commentary is not helpful here. The Development and Product teams are fully aware of the issues and the various solutions. Continued commentary is just noise that makes it harder for us to do the work that needs to be done to improve Firefox. If you're not adding new data, please don't comment. Thanks.
Comment 108•12 years ago
|
||
(In reply to comment #104) > The only question I have, given how late in the game we are here, is whether > it would be safer to take a "flip all fonts" pref rather than introducing > code to only change it for certain fonts at certain sizes. No, it would not be safer. A wholesale switch away from DWRITE_RENDERING_MODE_DEFAULT *must* be accompanied by the patches in this bug (in particular, the part 6 patches) or else we will run into bug 662115. So there really isn't an option here: you have to take this bug. (Either that, or spin a new patch to fix bug 662115, but that doesn't make sense from a risk perspective.)
Assignee | ||
Comment 109•12 years ago
|
||
Kai is correct.
Comment 110•12 years ago
|
||
Comment on attachment 539812 [details] [diff] [review] parts 1-6 rebased to mozilla-aurora tip After considerable discussion, I've concluded that this is not an appropriate change for Aurora. No one wants to see this fixed more than me, but given the size of the changes and the fact that no more than 10-15% of our users are going to be affected by the regression, I can't see how it warrants putting the Firefox 6 release at risk to get it a fix to users six weeks sooner.
Attachment #539812 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•12 years ago
|
Attachment #539814 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•12 years ago
|
Target Milestone: --- → mozilla7
Comment 111•12 years ago
|
||
Please notice that Microsoft has now released an update for Arial, Tahoma and Verdana apparently for improving the rendering with DirectWrite at small sizes: http://support.microsoft.com/kb/2545698/en-us?fr=1
Comment 112•12 years ago
|
||
(In reply to comment #111) > Please notice that Microsoft has now released an update for Arial, Tahoma > and Verdana apparently for improving the rendering with DirectWrite at small > sizes: > http://support.microsoft.com/kb/2545698/en-us?fr=1 The three fonts from KB2545698, at the sizes mentioned in the article, at 96DPI. left=before, right=after, top=Firefox (GDI fallback disabled), bottom=IE9
Comment 113•12 years ago
|
||
(In reply to comment #112) > Created attachment 542624 [details] > KB2545698.png, before and after > > The three fonts from KB2545698, at the sizes mentioned in the article, at > 96DPI. > > left=before, right=after, top=Firefox (GDI fallback disabled), bottom=IE9 Rendered with what gamma/enhanced contrast settings? See about:support for this info.
Comment 114•12 years ago
|
||
(In reply to comment #113) > Rendered with what gamma/enhanced contrast settings? Default. (-1)
Comment 115•12 years ago
|
||
(In reply to comment #114) > (In reply to comment #113) > > Rendered with what gamma/enhanced contrast settings? > > Default. (-1) What shows up under Cleartype parameter settings, as listed in about:support in a nightly build? "Default" is dependent upon whether you've run the Cleartype tuner or not.
Comment 116•12 years ago
|
||
(In reply to comment #115) > What shows up under Cleartype parameter settings, as listed in about:support > in a nightly build? "Default" is dependent upon whether you've run the > Cleartype tuner or not. Parameters not found; I never ran the system's CT tuner on this particular machine.
Comment 117•12 years ago
|
||
(In reply to comment #111) > Please notice that Microsoft has now released an update for Arial, Tahoma > and Verdana apparently for improving the rendering with DirectWrite at small > sizes: > http://support.microsoft.com/kb/2545698/en-us?fr=1 Rendering these fonts with GDI Classic is still necessary? IMHO it is not.
Comment 118•12 years ago
|
||
Please clarify: Has this been implemented only for Windows 7? I ask this because core fonts for a given operating system and platform are not necessarily the core fonts for other operating systems and platforms. See <http://www.codestyle.org/css/font-family/sampler-CombinedResultsFull.shtml>.
Comment 119•12 years ago
|
||
DirectWrite rendering is only available for Windows Vista and newer.
You need to log in
before you can comment on or make changes to this bug.
Description
•