502906 - refactor font classes to support new types of fonts and shapers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: Part 1: eliminate gfxAtsuiFontGroup and gfxCoreTextFontGroup classes

This was described on dev-tech-gfx, see the thread URL above. It is a foundation for introducing HarfBuzz as a new text layout back-end, and will also allow us to unify some of the code that is currently reimplemented in the separate back-ends.

An initial step is to eliminate the platform/backend-specific subclasses of gfxFontGroup, moving their functionality either up into the platform-independent fontgroup class (e.g., matching available fonts to the characters in a text run) or down into the gfxFont classes (e.g., getting glyphs and metrics for a text run). The first patch does this for the Mac OS X font back-ends; this will make it possible for a single fontgroup to manage both CoreText and HarfBuzz-rendered fonts, for example.

The other platforms will be handled similarly in separate patches.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 1: (updated) remove gfxAtsuiFontGroup and gfxCoreTextFontGroup

WIP, first of a series of patches to restructure the font backends.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 2: replace gfxAtsuiFont and gfxCoreTextFont with new gfxFontAccessor, gfxFontLayoutEngine subclasses

This restructures the Mac font classes to a point where it is possible to introduce a new HarfBuzz shaper implementation without massive code duplication across the different shaping paths (see bug 449292). Currently Mac-only, parallel patches will refactor the other platform backends to follow the same model.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Part 1, v2: eliminate separate gfx{Atsui,CoreText}FontGroup classes

First step towards font class refactoring: eliminate separate gfxFontGroup subclasses for different Mac shaping back-ends, move textRun creation from the fontGroup down to the font (as different fonts within the group may be using different engines in future).

This patch only really affects the Mac back-ends; there are minor adjustments to other platforms just to keep the build happy as we begin to move towards a more unified structure.

Passes on tryserver, applied on top of patches at bugs 519445 and 493280.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: part 1, v3: refreshed and cleaned-up patch to eliminate Atsui and CoreText subclasses of gfxFontGroup

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suggested to Jonathan on IRC that gfxFontShaper would be a better name than gfxFontLayoutEngine.

Comment 7

[John Daggett (:jtd)]

Need to fix regressions from 493280 before landing.

Comment 8

[John Daggett (:jtd)]

Comment on attachment 420625 [details] [diff] [review]
part 1, v3: refreshed and cleaned-up patch to eliminate Atsui and CoreText subclasses of gfxFontGroup

Overall, looks good.

Within gfxPlatform::SetupClusterBoundaries:

+    PRUint32 i, length = aTextRun->GetLength();
+    for (i = 0; i < length; ++i) {
+        PRBool surrogatePair = PR_FALSE;
+        PRUint32 ch = aString[i];
+        if (ch >= 0xD800 && ch <= 0xDBFF &&
+            i < length - 1 &&
+            aString[i+1] >= 0xDC00 && aString[i+1] < 0xDFFF) {
+            ch = (ch - 0xD800) * 0x400 + (aString[i+1] - 0xDC00) + 0x10000;
+            surrogatePair = PR_TRUE;
+        }
+        if (i > 0 && gc->Get(aString[i]) == nsIUGenCategory::kMark) {
+            gfxTextRun::CompressedGlyph g;
+            aTextRun->SetGlyphs(i, g.SetComplex(PR_FALSE, PR_TRUE, 0), nsnull);
+        }
+        if (surrogatePair) {
+            ++i;
+            gfxTextRun::CompressedGlyph g;
+            aTextRun->SetGlyphs(i, g.SetComplex(PR_FALSE, PR_TRUE, 0), nsnull);
+        }
+    }

Why all the surrogate pair handling with constants?  Why not use the macros NS_IS_HIGH_SURROGATE, NS_IS_LOW_SURROGATE and SURROGATE_TO_UCS4 instead?  It would really make this code more readable.

So this generic code for other platforms (since Mac, Windows, and Pango platforms all seem to implement this)?

+            if ((block >= 0x05 && block <= 0x08) // hebrew, arabic, syriac, thaana, n'ko, reserved R-L
+                || (block == 0xfb && block <= 0xfe) // hebrew & arabic presentation forms
+                || block == 0xd8 // high surrogate that might form bidi char
+                || ch == 0x200f || ch == 0x202b || ch == 0x202e) // bidi controls

Named constants please (enums, #defines, whatever).

+        if (isRTL && !HasMirroringInfo()) {
+// 361695 - ATSUI only does glyph mirroring when the font contains a 'prop' table
+// with glyph mirroring info, the character mirroring has to be done manually in the 
+// fallback case.  Only used for RTL text runs.
+            PRUint32 i;
+            for (i = 0; i < aRunLength; ++i) {
+                textBuffer[startOffset + i] = SymmSwap(aString[aRunStart + i]);
+            }
 
Comment indentation.

+void
+gfxAtsuiFont::InitTextRun(gfxTextRun *aTextRun,
+                          const PRUnichar *aString,
+                          PRUint32 aRunStart,
+                          PRUint32 aRunLength)
+{
+    // TODO: limit length of text we pass to ATSUI
+    // But is it worth doing, as ATSUI will be going away?
+    // This code is doomed, as soon as we officially pull the plug
+    // on 10.4 support on trunk.

We've worked around ATSUI crashing problems by limiting the length in the past.  Does this only affect us on 10.4?  I seem to recall problems in 10.5 also.  I don't think it's a great idea to have code that allows users to be exposed to problems like this if they enable a pref.  If this code isn't safe and we think it's "dead", shouldn't you just trim out the code here?

I am concerned about one thing, this patch pushes the Mac code ahead but it leaves Windows and Linux code in separate states, making it harder for others to make changes easily across platforms.  Also how are you planning to work the Uniscribe code to do shaping per-font?

I'd really like to see other platforms kept in sync so that other cross-platform font system changes can be made concurrently to Harfbuzz work.  Having different code structure across platforms makes things very confusing, especially in the case of Linux.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to comment #8)
> Within gfxPlatform::SetupClusterBoundaries:
> 
> Why all the surrogate pair handling with constants?  Why not use the macros
> NS_IS_HIGH_SURROGATE, NS_IS_LOW_SURROGATE and SURROGATE_TO_UCS4 instead?  It
> would really make this code more readable.

Fixed.

> So this generic code for other platforms (since Mac, Windows, and Pango
> platforms all seem to implement this)?

Yes, it's a generic implementation that's intended for use everywhere, but I have not yet removed the various platform-specific versions. (And testing might reveal that in some cases, using native platform APIs is preferable, though I hope we can eliminate those paths in due course.)

> +            if ((block >= 0x05 && block <= 0x08) // hebrew, arabic, syriac,
> thaana, n'ko, reserved R-L
> +                || (block == 0xfb && block <= 0xfe) // hebrew & arabic
> presentation forms
> +                || block == 0xd8 // high surrogate that might form bidi char
> +                || ch == 0x200f || ch == 0x202b || ch == 0x202e) // bidi
> controls
> 
> Named constants please (enums, #defines, whatever).

I've moved this to a (static inline) function in gfxFontUtils, using named constants and allowing it to be shared between back-ends.

> +    // TODO: limit length of text we pass to ATSUI
> +    // But is it worth doing, as ATSUI will be going away?
> +    // This code is doomed, as soon as we officially pull the plug
> +    // on 10.4 support on trunk.
> 
> We've worked around ATSUI crashing problems by limiting the length in the past.
>  Does this only affect us on 10.4?  I seem to recall problems in 10.5 also.  I
> don't think it's a great idea to have code that allows users to be exposed to
> problems like this if they enable a pref.  If this code isn't safe and we think
> it's "dead", shouldn't you just trim out the code here?

Yes, I expect 10.5 could be affected; but my current assumption is that as soon as we officially abandon 10.4 support on trunk (which I expect to happen before we ship a browser based on this code), we'll be removing the ATSUI code. We're keeping it around for now because AFAIK that support decision has not formally been made. Also, I like to keep both the Core Text and ATSUI back-ends available on trunk for testing purposes for the time being, but I'm not expecting this to make it into a shipping product.

> I am concerned about one thing, this patch pushes the Mac code ahead but it
> leaves Windows and Linux code in separate states, making it harder for others
> to make changes easily across platforms.  Also how are you planning to work the
> Uniscribe code to do shaping per-font?

The "Part 2" patch here restructures the Mac code further, into the "font accessor" and "font shaper" classes, and also does the equivalent with the Windows code, so at that point they come much more into sync. I'm just refreshing Part 2 and will post it for review shortly.

After that, it'll just be the Pango code that's painfully different. I don't think it's realistic or worthwhile to try and harmonize the Pango path with the Windows and Mac ones. Rather, I intend to leave it as untouched as possible while Harfbuzz gets added alongside it (based on the new Windows and Mac structure); then in due course we can drop the entire chunk of Pango font code.

Comment 10

[Jonathan Kew [:jfkthame]]

Checked in Part 1:
http://hg.mozilla.org/mozilla-central/rev/4e25e2e82747

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 2a: refactor Mac and Windows font code to use separate accessor and shaper objects

With this patch, the Mac and Windows backends both use the generic gfxFont class, with subsidiary accessor and shaper objects providing its interface to the platform. This is the structure that allows us to add in the harfbuzz shaper in a cross-platform way.

This patch alone will break the Linux build; part 2b (attached separately) fixes that so that it continues to work, but does not attempt a full refactoring of the Linux/Pango/Fontconfig implementation.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 2b: patch up the Linux code to accommodate the changes in gfxFont

See above; this is an interim stage just to keep the Linux build working, until we're ready to do a full replacement of the Pango code with the Harfbuzz shaper (which will be based closely on the cross-platform gfxFont/accessor/shaper model).

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+// forward-declare the Cairo font references that gfxFontAccessor needs
+typedef struct _cairo_font_face cairo_font_face_t;
+typedef struct _cairo_scaled_font cairo_scaled_font_t;
+
+class gfxFontAccessor {

Need some documentation here to explain what this class is for. I guess it's to provide access to font data and metrics?

+    PRBool                 mNeedsBold;
+
+    PRBool                 mIsValid;
+
+    PRBool                 mHasMetrics;

PRPackedBool

+
+class gfxFontShaper {

This needs documentation.

Looks nice.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(you should blog about this)

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: part 2a: refactor Mac and Windows font code, remove ATSUI support

This is updated in several ways:
* the ATSUI code is removed
* made font accessor and shaper objects refcounted to simplify management
* font table caching is pushed down to the gfxFontEntry by default, for better sharing
* fixed a metrics issue that was affecting mathml on windows, so all tests now pass on tryserver on both Mac and Windows

There are further enhancements I have in mind - e.g. making the font table cache use reference counting so as to release tables that are no longer being used - but I'd prefer to get this into the tree soon and then do further work in followups.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: part 2b: patch up the mobile & Linux code to keep it building ok with the new gfxFont

Attaching the current state of the Linux compatibility patch (needed before we can actually land part 2a, as that alone breaks the mobile and linux builds).

Still running some tests on this to determine if a few mochitest failures are real issues with the patch or just unreliable tests...

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 424554 [details] [diff] [review]
part 2b: patch up the mobile & Linux code to keep it building ok with the new gfxFont

This now appears to be OK on tryserver; and the mochitest failures I get locally also occur without the patch, so they seem to be related to system configuration, etc, rather than to this code.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: part 2a: refactor Mac and Windows font code to use accessor and shaper objects

Refreshed for current trunk + patches in bug 548177 and bug 524107, which are expected to land shortly.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: part 2b: patch up the mobile & Linux code to keep it building ok with the new gfxFont

Comment 20

[John Daggett (:jtd)]

I did a quick pass on this today, I'll do another pass once you've integrated changes needed to the DWrite code.  

I think it's somewhat unfortunate to have three classes for holding font info, a basic gfxFont, and a gfxFontAccessor and gfxFontEntry for each font flavor.  I don't see what pulling out gfxFontAccessor buys us, seems like you could simply separate out the shaping code into shapers and leave code for accessing font info in subclasses of gfxFont.  There would be a lot less code moving around and a lower risk of regressions creeping in between edits.  Is there some behavior that requires gfxFont to be distinct from gfxFontAccessor?  The distinction seems artificial, lots of gfxFont methods are simply pass-through's to the same method in gfxFontAccessor.

The gfxFontShaper is almost completely made up of static methods, only the InitTextRun is non-static.

+    // Get a pointer to a font table (or nsnull if not available);
+    // the pointer is guaranteed to remain valid as long as the font entry exists.
+    // The font entry may hold its own cached copy of the font data if necessary,
+    // or it may simply get a table pointer from the platform if that is supported.
+    // Returns nsnull if the requested table does not exist.
+    // Default implementation uses GetFontTable to load data, and caches it in
+    // the font entry; platforms should override if they have a way to directly
+    // get a pointer to font data from the OS without copying it (e.g., by mmap()ing
+    // the font file).
+    virtual void* GetFontTablePtr(PRUint32 aTableTag,
+                                  PRUint32 *aTableLen = nsnull);
+
+    // Query whether a table is available in the font, without necessarily
+    // loading the actual data.
+    // Default implementation checks the font table cache, and if not there,
+    // uses GetFontTable (so it actually does load the data); override if the
+    // platform has a cheaper way to check for presence of the table.
+    virtual PRBool HasTable(PRUint32 aTableTag);

+
+    nsTHashtable<FontTableHashEntry> mFontTables;

+float
+gfxFontAccessor::FUnitsToDevUnits(PRInt32 aValue)
+{
+    PRUint32 headLen;
+    void *headData = GetFontTablePtr(TRUETYPE_TAG('h','e','a','d'), &headLen);
+    if (!headData || headLen != sizeof(HeadTable)) {
+        return aValue;
+    }
+
+    PRUint16 upem = ((HeadTable*)headData)->unitsPerEm;
+    return aValue * GetAdjustedSize() / upem;
+}

+PRInt32
+gfxFontAccessor::GetUnscaledGlyphAdvance(PRUint32 aGlyphID)
+{
+    PRUint32 hheaLen;
+    void *hheaData = GetFontTablePtr(TRUETYPE_TAG('h','h','e','a'), &hheaLen);
+    if (!hheaData || hheaLen != sizeof(HMetricsHeader)) {
+        return 0;
+    }
+
+    PRUint32 hmtxLen;
+    void *hmtxData = GetFontTablePtr(TRUETYPE_TAG('h','m','t','x'), &hmtxLen);
+    if (!hmtxData) {
+        return 0;
+    }
+
+    PRUint32 numLongMetrics = ((HMetricsHeader*)hheaData)->numberOfHMetrics;
+    if (aGlyphID >= numLongMetrics) {
+        aGlyphID = numLongMetrics - 1;
+    }
+
+    PRUint16 adv = ((HMetrics*)hmtxData)->metrics[aGlyphID].advanceWidth;
+
+    return adv;
+}

+    // Glyph advance in device units (scaled to the font size)
+    float GetScaledGlyphAdvance(PRUint32 aGlyphID) {
+        return FUnitsToDevUnits(GetUnscaledGlyphAdvance(aGlyphID));
+    }

+// ??? jfk - to check
+//                glyphX = x - mFontAccessor->GetScaledGlyphAdvance(glyph->index) * appUnitsPerDevUnit;

+// ??? jfk - to check
+//                        glyphX -= mFontAccessor->GetScaledGlyphAdvance(glyph->index) * appUnitsPerDevUnit;

There's a cache of font tables per font entry.  Font entries exist until teardown so effectively for platform fonts that means until the app is torn down.  But the only code that uses this in your patch is commented out.

I'm guessing you intend this for use with the harfbuzz shaper.  I don't think it makes sense for use with platform shapers and that makes it hard to evaluate the utility of this code.  I'm not sure a general font table cache is warranted.  The code in FUnitsToDevUnits is a bad use of it, even with a font table cache, units-per-em should kept in a field and not looked up each time.  I also think you shouldn't be looking up tables per glyph, advance values should be looked up with an array of glyphs.

But for this patch, I don't think any of this code belongs here, it's really code to be evaluated in the context of the harfbuzz shaper since it's not needed for platform shapers.

+    // get pointer to a specific table within the font data
+    static const void*
+    GetSFNTTablePtr(const PRUint8* aFontData,
+                    PRUint32 aFontDataLength,
+                    PRUint32 aTableTag,
+                    PRUint32 *aTableSize = nsnull);
+

This function isn't used anywhere.  If this is for harfbuzz use, save it for that patch.

+class gfxQuartzFontAccessor : public gfxFontAccessor {
+public:
+    gfxQuartzFontAccessor(MacOSFontEntry *aFontEntry,
+                          const gfxFontStyle *aFontStyle,
+                          PRBool aNeedsBold,
+                          AntialiasOption anAntialiasOption = kAntialiasDefault);

Rather than gfxQuartzFontAccessor, this should really be gfxMacFontAccessor (or gfxCoreTextFontAccessor?) I think, for the same reason gfxQuartzFontCache was changed.  Maybe this was just leftoever from the CoreText + ATSUI version?  It's using ATS + CoreText API's.

+    nsCOMPtr<nsIPrefBranch> textLayoutBranch;
+    prefService->GetBranch("gfx.text_layout.", getter_AddRefs(textLayoutBranch));
+    mTextLayoutPrefBranch = do_QueryInterface(textLayoutBranch);
+    if (mTextLayoutPrefBranch)
+      mTextLayoutPrefBranch->AddObserver("", this, PR_TRUE);

Hmm, what's this?  Looks like it leftover or unused.

> +#if 0 // FIXME
>          if (mForceGDIPlace)
>              return PlaceGDI();
>  
>          PRBool allCJK = PR_TRUE;

> +            /* Uniscribe doesn't like this font, use GDI instead */
> +//            item->GetShaper()->GetFontAccessor()->GetFontEntry()->mForceGDI = PR_TRUE;
 
So GDI placement does work in this patch?  I think this needs to work before landing.

+/*
         // jtdfix - push this into the pref handling code??
         mItemLangGroup = nsnull;
 
         const SCRIPT_PROPERTIES *sp = item->ScriptProperties();
         if (!sp->fAmbiguousCharSet) {
             WORD primaryId = PRIMARYLANGID(sp->langid);
             mItemLangGroup = gScriptToText[primaryId].langCode;
         }
+*/

If this isn't needed remove it.

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - split out gfxCoreTextFont into gfxMacFont and gfxCoreTextShaper

This introduces a gfxFontShaper interface, and splits gfxCoreTextFont into gfxMacFont (which will have table accessor methods added to support the Harfbuzz shaper as well) and gfxCoreTextShaper. So it gives no change in functionality, but provides the possibility of plugging in a different shaping engine.

(Similar changes for the Windows GDI/Uniscribe and DirectWrite code will follow after further testing.)

Comment 22

[John Daggett (:jtd)]

Comment on attachment 431043 [details] [diff] [review]
part 2 - split out gfxCoreTextFont into gfxMacFont and gfxCoreTextShaper


>  gfxFont::gfxFont(gfxFontEntry *aFontEntry, const gfxFontStyle *aFontStyle) :
> -    mFontEntry(aFontEntry), mIsValid(PR_TRUE), mStyle(*aFontStyle), mSyntheticBoldOffset(0)
> +    mFontEntry(aFontEntry), mIsValid(PR_TRUE),
> +    mStyle(*aFontStyle), mSyntheticBoldOffset(0),
> +    mShaper(nsnull)
> 
> +gfxCoreTextShaper::gfxCoreTextShaper(gfxMacFont *aFont)
> +    : gfxFontShaper(aFont)

From this it appears that the font owns the shaper but it's never deleted.  Delete in the gfxFont destructor?

-gfxCoreTextFont::InitMetrics()
+gfxMacFont::InitMetrics(const gfxFontStyle *aFontStyle)

Why the need to pass in the style?  This is only called with mStyle.

-    CGFontRef cgFont = ::CGFontCreateWithPlatformFont(&mATSFont);
-    mFontFace = cairo_quartz_font_face_create_for_cgfont(cgFont);
-    ::CGFontRelease(cgFont);
-
-    cairo_status_t cairoerr = cairo_font_face_status(mFontFace);
-    if (cairoerr != CAIRO_STATUS_SUCCESS) {
-        mIsValid = PR_FALSE;
-#ifdef DEBUG
-        char warnBuf[1024];
-        sprintf(warnBuf, "Failed to create Cairo font face: %s status: %d",
-                NS_ConvertUTF16toUTF8(GetName()).get(), cairoerr);
-        NS_WARNING(warnBuf);
-#endif
-        return;
-    }

+    CGFontRef cgFont = ::CGFontCreateWithPlatformFont(&mATSFont);
+    mFontFace = cairo_quartz_font_face_create_for_cgfont(cgFont);
+    ::CGFontRelease(cgFont);
+
+    cairo_matrix_t sizeMatrix, ctm;
+    cairo_matrix_init_identity(&ctm);
+    cairo_matrix_init_scale(&sizeMatrix, mAdjustedSize, mAdjustedSize);

The error handling code has been trimmed out, not sure that's such a great idea.  The code in cairo_scaled_font_create for Quartz fonts looks like it makes all sorts of calls that would fail/crash if the font is a mess.  Unless there's a really good reason to omit this, I think it makes more sense to leave in the extra error checking.

+#ifdef DEBUG
+/*
+        char warnBuf[1024];
+        sprintf(warnBuf, "Failed to create scaled font: %s status: %d", NS_ConvertUTF16toUTF8(GetName()).get(), cairoerr);
+        NS_WARNING(warnBuf);
+*/
+#endif

Why comment out the warning here?

+    mMetrics.aveCharWidth += mSyntheticBoldOffset; // FIXME

Fix or include a better comment noting what the problem is.

+    float GetCharWidth(PRUnichar aUniChar, PRUint32 *aGlyphID);
+    float GetCharHeight(PRUnichar aUniChar);

Should these take a CTFont ref as a parameter, since in both GetCharWidth and GetCharHeight you create a CTFont on the fly?  These are called several times within InitMetrics, seems like a local CTFont would eliminate the need to do this multiple times.

-        nsresult rv = CallGetService(NS_UNICHARUTIL_CONTRACTID, &gGenCategory);
+        nsresult rv = CallGetService(NS_UNICHARCATEGORY_CONTRACTID, &gGenCategory);

Just curious, what's this change about?

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Mac font restructure - cleaned up as per comments

(In reply to comment #22)
> From this it appears that the font owns the shaper but it's never deleted. 
> Delete in the gfxFont destructor?

Oops, I thought I'd done that. Must have ended up in the Windows patch by mistake. :(

I've made mShaper an nsAutoPtr so that deletion is automatic.

> -gfxCoreTextFont::InitMetrics()
> +gfxMacFont::InitMetrics(const gfxFontStyle *aFontStyle)
> 
> Why the need to pass in the style?  This is only called with mStyle.

Uhh, yes. I was misled by older code where the subclass had its own mFontStyle as well (wonder why?), and I was removing that as it's only needed during initialization here.

> The error handling code has been trimmed out, not sure that's such a great
> idea.

Indeed. Restored it.

> Why comment out the warning here?

Sorry, leftover from an earlier iteration where this code didn't have access to GetName().

> +    mMetrics.aveCharWidth += mSyntheticBoldOffset; // FIXME
> 
> Fix or include a better comment noting what the problem is.

The comment is obsolete, this is now back where it belongs.

> Should these take a CTFont ref as a parameter

Makes sense.

> -        nsresult rv = CallGetService(NS_UNICHARUTIL_CONTRACTID,
> &gGenCategory);
> +        nsresult rv = CallGetService(NS_UNICHARCATEGORY_CONTRACTID,
> &gGenCategory);
> 
> Just curious, what's this change about?

Correcting a dumb error, that was the wrong service ID and this has never worked (but the function was not in use, so we hadn't noticed). With the right ID, it actually succeeds. :)

Comment 24

[John Daggett (:jtd)]

Comment on attachment 431335 [details] [diff] [review]
part 2 - Mac font restructure - cleaned up as per comments

Looks good.

Comment 25

[Jonathan Kew [:jfkthame]]

pushed part 2:
http://hg.mozilla.org/mozilla-central/rev/4954c1bf098d

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - factor out Uniscribe and GDI shapers from Windows font code

I think this is much tidier now, and it seems to pass unit and performance tests well. (Some tryserver Tp4 runs even suggest a noticeable perf win, although it seems to depend on the version of Windows we're on.)

To allow fallback from Uniscribe to "dumb" GDI layout for bad fonts, I've made gfxFontShaper::InitTextRun return a boolean indicating success; if it fails, the GDI font will replace its Uniscribe shaper with a GDI one, and mark the font entry as "needs GDI". (I don't actually know a specific testcase that will trigger this fallback, but I have tested the GDI layout path by choosing it manually.)

Comment 27

[John Daggett (:jtd)]

(In reply to comment #26)
> Created an attachment (id=431664) [details]
> part 3 - factor out Uniscribe and GDI shapers from Windows font code

>      if (mUnknownCMAP) {
> +#if 0 // FIXME
>          if (aCh > 0xFFFF)
>              return PR_FALSE;
> .
> .
> .
>          if (hasGlyph) {
>              mCharacterMap.set(aCh);
>              return PR_TRUE;
>          }
> +#endif

This needs to be reworked.  Have you tested your patch with Type1 fonts?

> +    // options to specify the kind of AA to be used when creating a font
> +    typedef enum {
> +        kAntialiasDefault,
> +        kAntialiasNone,
> +        kAntialiasGrayscale,
> +        kAntialiasSubpixel
> +    } AntialiasOption;
> 
> +    // the AA setting requested for this font - may affect glyph bounds
> +    AntialiasOption            mAntialiasOption;
> +
> 
> +static inline cairo_antialias_t
> +GetCairoAntialiasOption(gfxFont::AntialiasOption anAntialiasOption)
> +{
> +    switch (anAntialiasOption) {
> +    default:
> +    case gfxFont::kAntialiasDefault:
> +        return CAIRO_ANTIALIAS_DEFAULT;
> +    case gfxFont::kAntialiasNone:
> +        return CAIRO_ANTIALIAS_NONE;
> +    case gfxFont::kAntialiasGrayscale:
> +        return CAIRO_ANTIALIAS_GRAY;
> +    case gfxFont::kAntialiasSubpixel:
> +        return CAIRO_ANTIALIAS_SUBPIXEL;
> +    }
> +}

This type appears to completely reflect cairo_antialias_t, seems simpler to use the cairo type if it's a 1-for-1 match.

+    operator HDC () {
+        return dc;
+    }

I think it would be better to avoid using this syntactic sugar, it doesn't really save much in the way of typing and doesn't document the code as well as something like 'GetDC' does.

> -void
> -gfxWindowsFont::FillLogFont(gfxFloat aSize)
> -{
> 
> -    isItalic = (GetStyle()->style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE));
> 
> -    // if user font, disable italics/bold if defined to be italics/bold face
> -    // this avoids unwanted synthetic italics/bold
> -    if (fe->mIsUserFont) {
> -        if (fe->IsItalic())
> -            isItalic = PR_FALSE; // avoid synthetic italic
> -        if (fe->IsBold()) {
> -            weight = 400; // avoid synthetic bold
> -        }
> -    }
> -
> -    fe->FillLogFont(&mLogFont, isItalic, weight, aSize);
> 
> +void
> +gfxGDIFont::InitMetrics()
> +{
> +    PRBool isItalic = (mFontStyle.style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE));
> 
> +            FillLogFont(isItalic, mAdjustedSize);
> 
> +void
> +gfxGDIFont::FillLogFont(PRBool aNeedsItalic, gfxFloat aSize)
> +{
> 
> +    // if user font, disable italics/bold if defined to be italics/bold face
> +    // this avoids unwanted synthetic italics/bold
> +    if (fe->mIsUserFont) {
> +        if (fe->IsItalic())
> +            aNeedsItalic = PR_FALSE; // avoid synthetic italic
> +        if (fe->IsBold()) {
> +            weight = 400; // avoid synthetic bold
> +        }
> +    }
> +
> +    fe->FillLogFont(&mLogFont, aNeedsItalic, weight, aSize);
> +}

You're moving the definition and use of isItalic farther away from it's use with this change, I don't think that makes much sense.  I don't see any reason to make aNeedsItalic a parameter here.

> gfxFont.h:
> 
> class THEBES_API gfxFont {
> 
>     gfxFontStyle               mStyle;
> 
> }
> 
> +class gfxGDIFont : public gfxFont
> +{
> +
> +    const gfxFontStyle    mFontStyle;
> +
> +}

Redundant data field added?

+gfxGDIFont::InitMetrics()
+{
+    PRBool isItalic = (mFontStyle.style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE));
+    if (mAdjustedSize == 0.0) {
+        mAdjustedSize = mFontStyle.size;
+        if (mFontStyle.sizeAdjust != 0.0) {
+            // to implement font-size-adjust, we first create the "unadjusted" font
+            FillLogFont(isItalic, mAdjustedSize);
+            mFont = ::CreateFontIndirectW(&mLogFont);
+
+            // initialize its metrics, then delete the font
+            InitMetrics();

Seems like you've got a potential infinite recursion here, if mFontStyle.size is 0 and mFontStyle.sizeAdjust != 0.0, stack overflow will occur.  Not that I don't enjoy stack overflow as much as the next guy...

> -    HRESULT PlaceUniscribe() {
> +    HRESULT Place() {
>          HRESULT rv;
>          HDC placeDC = nsnull;
>  
> +        mOffsets.SetLength(mNumGlyphs);
> +        mAdvances.SetLength(mNumGlyphs);

SetLength returns false if setting the length failed but the code below it ignores a failure and passes the arrays into Uniscribe.  This could potentially result in a buffer overrun.  Check the return result and fail appropriately.

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to comment #27)
> >      if (mUnknownCMAP) {
> > +#if 0 // FIXME

Aarghh - how in the world did I forget to do that? Now fixed.

> > +    // options to specify the kind of AA to be used when creating a font
> This type appears to completely reflect cairo_antialias_t, seems simpler to use
> the cairo type if it's a 1-for-1 match.

The problem with that is visibility - gfxFont.h is an exported header, but the cairo headers aren't, and so we can't #include them here and we don't have cairo_antialias_t available within the declaration of gfxFont.

> You're moving the definition and use of isItalic farther away from it's use
> with this change, I don't think that makes much sense.  I don't see any reason
> to make aNeedsItalic a parameter here.

True. It made more sense when I was thinking we didn't have or need access to the actual style record within FillLogFont. Reverted that.

> +gfxGDIFont::InitMetrics()
...
> +            InitMetrics();
> 
> Seems like you've got a potential infinite recursion here, if mFontStyle.size
> is 0 and mFontStyle.sizeAdjust != 0.0, stack overflow will occur.  

Hmm. Actually, font-size-adjust seems to be broken altogether. :( Just looking at that now, then I'll post an update.

> > +        mOffsets.SetLength(mNumGlyphs);
> > +        mAdvances.SetLength(mNumGlyphs);
> 
> SetLength returns false if setting the length failed but the code below it
> ignores a failure and passes the arrays into Uniscribe.  This could potentially
> result in a buffer overrun.  Check the return result and fail appropriately.

Good catch. Our existing code also suffers from this; we might want to fix it even on branches:
http://mxr.mozilla.org/mozilla1.9.2/source/gfx/thebes/src/gfxWindowsFonts.cpp#2012

Comment 29

[John Daggett (:jtd)]

> The problem with that is visibility - gfxFont.h is an exported header, but the
> cairo headers aren't, and so we can't #include them here and we don't have
> cairo_antialias_t available within the declaration of gfxFont.

Ah, ok, guess we have no choice then.

> Good catch. Our existing code also suffers from this; we might want to fix it
> even on branches:
> http://mxr.mozilla.org/mozilla1.9.2/source/gfx/thebes/src/gfxWindowsFonts.cpp#2012

Ouch.

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - factor out Uniscribe and GDI shapers - fix issues from comment #27

Comment 31

[Jonathan Kew [:jfkthame]]

(In reply to comment #29)
> > Good catch. Our existing code also suffers from this; we might want to fix it
> > even on branches:
> > http://mxr.mozilla.org/mozilla1.9.2/source/gfx/thebes/src/gfxWindowsFonts.cpp#2012
> 
> Ouch.

There are actually quite a number of these unchecked SetLength() calls. I'll post an updated patch with error checking added shortly, and have filed bug 551661 to fix them on 1.9.2.

Comment 32

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - factor out Uniscribe and GDI shapers - add error checking on SetLength calls

Comment 33

[John Daggett (:jtd)]

Comment on attachment 431834 [details] [diff] [review]
part 3 - factor out Uniscribe and GDI shapers - add error checking on SetLength calls

Looks good.

Comment 34

[Jonathan Kew [:jfkthame]]

Pushed part 3:
http://hg.mozilla.org/mozilla-central/rev/8ff6056308bd

Comment 35

[[e-mail]]

Comment on attachment 431834 [details] [diff] [review]
part 3 - factor out Uniscribe and GDI shapers - add error checking on SetLength calls

> #ifdef MOZ_FT2_FONTS
> #include "gfxFT2Fonts.h"
> #else
>-#include "gfxWindowsFonts.h"
> #ifdef CAIRO_HAS_DWRITE_FONT
> #include "gfxDWriteFonts.h"
> #endif
> #endif
This change unfortunately means that if you have neither FT2 nor DWRITE then gfkWindowsPlatform.h fails to compile due to a lack of font headers.

+#include "gfxGDIShaper.h"
...
+                                           INT_MAX,
This file doesn't seem to include limits.h which is required to define this.

Comment 36

[Philip Chee]

I'm getting this build error.

Building deps for /c/t1/hg/comm-central/mozilla/gfx/thebes/src/gfxRect.cpp
cl -FogfxRect.obj -c  -DIMPL_THEBES -DWOFF_MOZILLA_CLIENT -DMOZILLA_INTERNAL_API -DMOZ_SUITE=1 -DOSTYPE=\"WINNT5.1\" -DOSARCH=WINNT  -I/c/t1/hg/comm-central/mozilla/gfx/thebes/src -I. -I../../../dist/
include -I../../../dist/include/nsprpub  -Ic:/t1/hg/objdir-sm/mozilla/dist/include/nspr -Ic:/t1/hg/objdir-sm/mozilla/dist/include/nss         -GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb  -DNDEBUG -DTRIMMED -O1 -Ic:/t1/hg/objdir-sm/mozilla/dist/include/cairo  -MD            -FI ../../../dist/include/mozilla-config.h -D
MOZILLA_CLIENT /c/t1/hg/comm-central/mozilla/gfx/thebes/src/gfxRect.cpp
../../../dist/include\gfxWindowsPlatform.h(185) : error C2143: syntax error : missing ';' before '*'
../../../dist/include\gfxWindowsPlatform.h(185) : error C4430: missing type specifier - int assumed.
 Note: C++ does not support default-int
../../../dist/include\gfxWindowsPlatform.h(185) : error C4430: missing type specifier - int assumed.
 Note: C++ does not support default-int
../../../dist/include\gfxWindowsPlatform.h(185) : warning C4183: 'FindFontFamily': missing return type; assumed to be a member function returning 'int'
make[7]: *** [gfxPlatform.obj] Error 2

Comment 37

[Justin Wood (:Callek)]

(In reply to comment #36)
> I'm getting this build error.
> 
... same here

I am trying a clobber right now to see if that fixes things, though in this particular case I doubt it.

Comment 38

[Justin Wood (:Callek)]

...files Bug 552619 for the failure

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to comment #34)
> Pushed part 3:
> http://hg.mozilla.org/mozilla-central/rev/8ff6056308bd

This checkin appears to have caused a Tp4 regression on Windows:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/550e214057c0e13f

Is that expected and/or workaroundable?

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to comment #39)
> (In reply to comment #34)
> > Pushed part 3:
> > http://hg.mozilla.org/mozilla-central/rev/8ff6056308bd
> 
> This checkin appears to have caused a Tp4 regression on Windows:
> http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/550e214057c0e13f

Yes, I noticed that - all the memory metrics seem to have taken a hit. On the other hand, there's a noticeable (5-6%) performance _win_ on WINNT5.1, at least.

> Is that expected and/or workaroundable?

I'm doing some investigation here; will report back later.

Comment 41

[Jonathan Kew [:jfkthame]]

Attachment: part 3.1 - reduce memory footprint of GDI font code

This patch aims to reduce RAM footprint by doing two things: (1) allocating the Metrics structure separately from the gfxGDIFont, and initializing only on demand rather than at font creation time, because we create a surprising number of fonts that we never use; and (2) removing the LOGFONTW record from the gfxGDIFont, as it is only needed transiently during initialization.

Waiting on tryserver results to see what impact this has. An earlier test of (1) alone showed a 1-2% reduction in Tp4 memory metrics from not allocating Metrics records until the font is actually used.

Comment 42

[Jonathan Kew [:jfkthame]]

Comment on attachment 433032 [details] [diff] [review]
part 3.1 - reduce memory footprint of GDI font code

The WINNT5.1 tryserver results suggest this should give us back around 5% or so. It'd be nice to land it and see what the "real" Talos results show, as tryserver is pretty noisy.

Comment 43

[Dimitry Andric]

This checkin seems to result in some font rendering issues, at least on Windows XP.  Some pages and fonts seem to get rendered with much more or less space between characters, but not in all situations.  See http://forums.mozillazine.org/viewtopic.php?f=23&t=1808685&start=0 for an example.

Comment 44

[Jonathan Kew [:jfkthame]]

(In reply to comment #43)
> This checkin seems to result in some font rendering issues, at least on Windows
> XP.  Some pages and fonts seem to get rendered with much more or less space
> between characters, but not in all situations.  See
> http://forums.mozillazine.org/viewtopic.php?f=23&t=1808685&start=0 for an
> example.

This has been filed as bug 553136; please follow up there with any additional detail on this issue. Thanks!

Comment 45

[Jonathan Kew [:jfkthame]]

Attachment: part 4: restructure dwrite font code to match mac and gdi-based paths

This removes the dwrite subclass of gfxFontGroup, which is intended to be backend-agnostic, and moves the textrun initialization to a new gfxDWriteShaper class instead. There should be no change in behavior, but matching the new architecture for other backends will simplify maintenance of areas such as font-matching/fallback, and the use of the gfxFontShaper model will allow us to experiment with harfbuzz shaping of dwrite fonts as well as gdi fonts.

Comment 46

[Bas Schouten (:bas.schouten)]

Comment on attachment 437273 [details] [diff] [review]
part 4: restructure dwrite font code to match mac and gdi-based paths

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -167,17 +167,17 @@ pref("media.autoplay.enabled", true);
> pref("gfx.color_management.mode", 2);
> pref("gfx.color_management.display_profile", "");
> pref("gfx.color_management.rendering_intent", 0);
> 
> pref("gfx.downloadable_fonts.enabled", true);
> 
> #ifdef XP_WIN
> #ifndef WINCE
>-pref("gfx.font_rendering.directwrite.enabled", false);
>+pref("gfx.font_rendering.directwrite.enabled", true);
> #endif
> #endif
> 
> pref("accessibility.browsewithcaret", false);
> pref("accessibility.warn_on_browsewithcaret", true);
> 
> pref("accessibility.browsewithcaret_shortcut.enabled", true);
> 

Really? Are you sure?

Comment 47

[Jonathan Kew [:jfkthame]]

Oops - no, that's not supposed to be there, it was temporary! I'll go remove it right now......

Comment 48

[Bas Schouten (:bas.schouten)]

Comment on attachment 437273 [details] [diff] [review]
part 4: restructure dwrite font code to match mac and gdi-based paths

Provided we don't make DWrite the default. :-)

Comment 49

[Jonathan Kew [:jfkthame]]

pushed part 4 - without turning on dwrite by default!
http://hg.mozilla.org/mozilla-central/rev/1cac391aec8c

Comment 50

[Jonathan Kew [:jfkthame]]

Attachment: part 3.1 - reduce memory footprint of GDI font code - refreshed patch

Updated patch for current trunk.

John, if you're not going to have a chance to review this, is there someone else you'd suggest?

Comment 51

[John Daggett (:jtd)]

+ NS_WARNING("invalid font! expect missing text");

How about "expect incorrect text rendering"?

+    mMetrics = new gfxFont::Metrics;
+    ::memset(mMetrics, 0, sizeof(*mMetrics));

Check for null, set mIsValid to false and bail.

There's still a problem that if the new gfxFont::Metrics fails there's lots of code that will fail, since the Metrics are passed back by reference.  This isn't something new, the same problem existed in Windows 1.9.1 code.  Kinda gross, but maybe a single dummy metrics object for these situations?

+    gfxUniscribeShaper(gfxGDIFont *aFont)
+        : gfxFontShaper(aFont)
+        , mScriptCache(NULL)
+    {
+        MOZ_COUNT_CTOR(gfxUniscribeShaper);
+    }
 
-    virtual ~gfxUniscribeShaper();
+    virtual ~gfxUniscribeShaper()
+    {
+        MOZ_COUNT_DTOR(gfxUniscribeShaper);
+    }

Hmmm, what's this change for?  Debug code perhaps?

Comment 52

[Jonathan Kew [:jfkthame]]

(In reply to comment #51)
> + NS_WARNING("invalid font! expect missing text");
> 
> How about "expect incorrect text rendering"?

Fine, will do.

> 
> +    mMetrics = new gfxFont::Metrics;
> +    ::memset(mMetrics, 0, sizeof(*mMetrics));
> 
> Check for null, set mIsValid to false and bail.

operator new can't return null, it's infallible. Failure to allocate will simply terminate the application.

(This was true even before the "infallible malloc" code landed a while ago, as the windows runtime would throw an exception that killed the app.)

> +    gfxUniscribeShaper(gfxGDIFont *aFont)
> +        : gfxFontShaper(aFont)
> +        , mScriptCache(NULL)
> +    {
> +        MOZ_COUNT_CTOR(gfxUniscribeShaper);
> +    }
> 
> -    virtual ~gfxUniscribeShaper();
> +    virtual ~gfxUniscribeShaper()
> +    {
> +        MOZ_COUNT_DTOR(gfxUniscribeShaper);
> +    }
> 
> Hmmm, what's this change for?  Debug code perhaps?

It means our refcount-trace code will notice if we leak shaper objects, rather than silently leaking them. Seems like a good practice in general. (They'll be empty macros in release builds anyway.)

Comment 53

[John Daggett (:jtd)]

Comment on attachment 442055 [details] [diff] [review]
part 3.1 - reduce memory footprint of GDI font code - refreshed patch

> > +    mMetrics = new gfxFont::Metrics;
> > +    ::memset(mMetrics, 0, sizeof(*mMetrics));
> > 
> > Check for null, set mIsValid to false and bail.
> 
> operator new can't return null, it's infallible. Failure to allocate will
> simply terminate the application.
> 
> (This was true even before the "infallible malloc" code landed a while ago, as
> the windows runtime would throw an exception that killed the app.)

Ok, fine.

> > +    gfxUniscribeShaper(gfxGDIFont *aFont)
> > +        : gfxFontShaper(aFont)
> > +        , mScriptCache(NULL)
> > +    {
> > +        MOZ_COUNT_CTOR(gfxUniscribeShaper);
> > +    }
> > 
> > -    virtual ~gfxUniscribeShaper();
> > +    virtual ~gfxUniscribeShaper()
> > +    {
> > +        MOZ_COUNT_DTOR(gfxUniscribeShaper);
> > +    }
> > 
> > Hmmm, what's this change for?  Debug code perhaps?
> 
> It means our refcount-trace code will notice if we leak shaper objects, rather
> than silently leaking them. Seems like a good practice in general. (They'll be
> empty macros in release builds anyway.)

I realize what they do, I was just wondering why you're only doing this
for these shapers and not others as well.  A follow-on patch is fine.

Comment 54

[Jonathan Kew [:jfkthame]]

pushed http://hg.mozilla.org/mozilla-central/rev/23d5bcb2c5d2

I added MOZ_COUNT_[CD]TOR to the DWrite shaper as well, for consistency; I didn't touch the non-Windows implementations in this patch, though I think it'd be worth doing that as part of ongoing font work.