389074 - implement Core Text backend to render text

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[u49640]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 

Apple has a new Text rendering in 10.5 (Leopard) that should be better than ATSUI that was implemented in Bug 121540.

The only page I've found so far about this, is the WWDC page that said:

"Adopt the new Core Text APIs that replace the deprecated QuickDraw Text APIs. Learn how to replace your handling of Pascal and C strings, text encodings, fonts, styles, layouts, and more with Core Text and other Leopard APIs. If you're currently using ATSUI to achieve simple Unicode text rendering, learn how you can switch to Core Text for faster rendering."

Mozilla should evaluate, if a switch to core text is feasible.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I remember hearing Vlad and roc discuss this on IRC.

Comment 2

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

Yup, I have a patch for this; it is much better than ATSUI and should give us some wins.  But we'd only be able to use it on 10.5, but that's ok.

I'll attach the patch-in-progress after I get back from OSCON, but I was waiting for a new Leopard seed because I think I'm running into some OS bugs...

Comment 3

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

Is it worth doing this for 1.9?

Comment 4

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

I think so, though not as a requirement.  It's not a huge amount of work, and has a potential for a significant improvement.  I'll poke at my patch next weekend and see if I can figure out whether the problem I was running into is an OS issue or not.

Comment 5

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

Attachment: patch-in-progress, may be outdated, has bugs

Attaching this here so it doesn't get lost; John Daggett might be taking this over.

Comment 6

[Alexander Kellett]

What's the progress on this issue? I'm just testing the latest firefox 3 nightlies with the strangecairo/atsui/cg mix used at the moment on the Mac. It seems that even 8pt fonts are antialiased at the moment.

thanks

Comment 7

[Alexander Kellett]

To temporarily solve the font size antialias issue I have the following local patch:

Index: cairo/cairo/src/cairo-quartz-surface.c
===================================================================
RCS file: /cvsroot/mozilla/gfx/cairo/cairo/src/cairo-quartz-surface.c,v
retrieving revision 1.42
diff -u -r1.42 cairo-quartz-surface.c
--- cairo/cairo/src/cairo-quartz-surface.c      6 Feb 2008 06:48:47 -0000       1.42
+++ cairo/cairo/src/cairo-quartz-surface.c      17 Feb 2008 15:53:55 -0000
@@ -1547,10 +1547,16 @@
     }
 #endif
 
+    if (5.0 < scaled_font->font_matrix.xx && scaled_font->font_matrix.xx < 10.0)
+       CGContextSetShouldAntialias(surface->cgContext, FALSE);
+
     CGContextShowGlyphsWithAdvances (surface->cgContext,
                                     cg_glyphs,
                                     cg_advances,
                                     num_glyphs);
+                           
+    if (5.0 < scaled_font->font_matrix.xx && scaled_font->font_matrix.xx < 10.0)
+       CGContextSetShouldAntialias(surface->cgContext, TRUE);
 
     if (action == DO_IMAGE || action == DO_TILED_IMAGE) {
        CGContextConcatCTM (surface->cgContext, surface->sourceImageTransform);

Comment 8

[Alexander Kellett]

Attachment: hack for current cvs to disable aa for fontsizes from 5 to 10

Comment 9

[John Daggett (:jtd)]

(In reply to comment #6)
> What's the progress on this issue? I'm just testing the latest firefox 3
> nightlies with the strangecairo/atsui/cg mix used at the moment on the Mac. It
> seems that even 8pt fonts are antialiased at the moment.

Alexander, thanks very much for the patch.  But you should really write up a separate bug, since the issue you're concerned about is very specific and support for CoreText is a much larger and independent issue.  The bug here is marked as an enhancement and I'm slated to work on it once FF3 ships.  Even then, it'll only be available on 10.5, since CoreText is not officially supported on 10.4 at this time.

As for small fonts being anti-aliased, that sounds like something we should be checking for at runtime and it sounds like something that should happen for FF3.  That's where a separate bug comes in!

Comment 10

[Alexander Kellett]

Thank you John. I've created issue https://bugzilla.mozilla.org/show_bug.cgi?id=418479.

Comment 11

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

I'm warming up to the idea of doing this soon. It will help prepare us for 64bit, maybe win some performance, and maybe fix some of the yucky ATSUI code. (I'm thinking of the code for extracting glyphs and matching glyphs to characters.) We'll probably need to do it for AAT support even if we end up being able to use Harfbuzz for Opentype.

Comment 12

[Jonathan Kew [:jfkthame]]

Yes, it would be good to do this, although we'll need to keep the ATSUI code path around and check at runtime for the availability of Core Text (until we're prepared to break compatibility with pre-Leopard systems).

I'd be happy to look into this, but if Vlad or John (or anyone) is already onto it, I don't want to get in the way. :-)

Comment 13

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

There's a thread about Mac OS requirements on dev.planning: http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/1613c3fe1ac4144c/fd76844c2cec1012

You're the most likely candidate for this, yeah :-). Vlad is pretty stretched already, and John has SVG fonts to keep him busy.

Comment 14

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

We would definitely want this in parallel with ATSUI for a while. It could be a run-time or build-time switch; run-time's obviously better if it's not much extra work.

Comment 15

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

It's hard to find bugs about current code when they are filed in components for dead code and never moved out ;)

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: updated patch to provide CoreText support (work in progress)

I'm attaching my current work-in-progress on this issue; not quite ready for proper review yet, but it does seem to be pretty functional and stable.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: updated patch (work in progress)

Currently still needs some work on clustering; overall performance on page-load test is 2% ahead of the ATSUI path. :)

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: updated work-in-progress

updated again, currently passes all except 5 layout reftests; added preliminary configure support

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: patch to implement CoreText support on 10.5+

Comment 20

[Jonathan Kew [:jfkthame]]

On my system running OS X 10.5.5, the CoreText path currently fails 4 reftests that pass under ATSUI (there are several others that fail similarly for both text engines):

  layout/reftests/line-breaking/ja-1.html
  layout/reftests/line-breaking/ja-2.html
  layout/reftests/line-breaking/ja-3.html

All 3 of these show small (sub-pixel?) differences in glyph positioning between the wrapped lines and those with explicit <br>. I'm not yet sure why this happens but will try to investigate.

  layout/reftests/text/zwnj-01.html

In this test, the presence of ZWNJ in RTL text disturbs the position of an adjacent glyph, even though overall line metrics are unaffected. This appears to be an Apple bug; filed as rdar://6427865.

Comment 21

[John Daggett (:jtd)]

Jonathan, I was looking over the patch this week but unfortunately it no longer applies cleanly.  Could you update the patch?  The implementation of LookupLocalFont has changed.  I'm not quite sure what the ATSFontRef version of this method is for, I don't see any callers:

+    gfxFontEntry* LookupLocalFont(ATSFontRef aFontRef);

Also, I think we probably also need to switch calls to ATSUI name table routines to use the CoreText equivalents instead in gfxQuartzFontCache.mm:

http://hg.mozilla.org/mozilla-central/file/71e9be2a304c/gfx/thebes/src/gfxQuartzFontCache.mm#l554

It might make sense to do this in a separate patch, since this would probably help us with performance on 10.5 independent of the larger issues of ATSUI vs. CoreText layout, we might take this patch without taking the larger CoreText patch.

Not quite sure how we would deploy this.  We don't build separate 10.4/10.5 binaries, so if we're going to use the same binary for both we need to do runtime switching.  If we're going to use Harfbuzz, this may just be an option to keep on the back burner.

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: updated to current trunk

Refreshed the patch to apply to current trunk (as of this afternoon); builds and runs ok. In browsing a variety of international sites, it consistently works at least as well as ATSUI, or in some cases better. Currently has a couple of new reftest failures (they're probably recently-added tests) in addition to those mentioned in comment #20, but they don't look too alarming.

Re ATSUI usage in gfxQuartzFontCache.mm: this will be addressed in a separate patch, as that's independent of the text layout aspect.

Further changes will be coming, but comments on the existing patch are still welcome.

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: updated to current trunk, added #ifdefs to eliminate ATSUI code on 64-bit

Comment 24

[John Daggett (:jtd)]

Overall, it looks fine.  There's a lot of code duplication and that's unfortunate, it creates a maintenance headache and there's a natural tendency for things to get out of sync.  We could introduce a superclass for all MacOSX fonts (gfxOSXFont/gfxOSXFontGroup?) and put shared code there but I realize that has it's own problems.  This depends on how long we expect to have to deal with 10.4/ATSUI.  If we're going to support both for 1.9.2, I would push to reduce some of the code duplication.

An example of how this bites us is in InitMetrics:

gfxCoreTextFonts.cpp:

>     ATSFontMetrics atsMetrics;
>     ATSFontGetHorizontalMetrics(mATSFont, kATSOptionFlagsDefault, &atsMetrics);

This is missing a top crasher fix put in recently in gfxAtsuiFonts.cpp:

>     ATSFontMetrics atsMetrics;
>     OSStatus err;
>     
>     err = ATSFontGetHorizontalMetrics(aFontRef, kATSOptionFlagsDefault,
>                                 &atsMetrics);
>                                 
>     if (err != noErr) {
>         mIsValid = PR_FALSE;
>         
> #ifdef DEBUG        
>         char warnBuf[1024];
>         sprintf(warnBuf, "Bad font metrics for: %s err: %8.8x", NS_ConvertUTF16toUTF8(GetName()).get(), PRUint32(err));
>         NS_WARNING(warnBuf);
> #endif
>         return;
>     }
  
Please move over that fix.

> +  AC_MSG_RESULT([$ac_cv_have_core_text])
> +  if test "$ac_cv_have_core_text" = "yes"; then
> +    AC_DEFINE(HAVE_CORE_TEXT, 1)
> +  fi

Small nit, I think we should be using MOZ_CORETEXT or something like that instead of HAVE_CORE_TEXT as build conditional.

Also, I think we should conditionalize the inclusion/compilation of gfxCoreTextFonts files in the makefiles rather than wrapping files in conditionals:

>  ifneq (,$(filter $(MOZ_WIDGET_TOOLKIT),mac cocoa))
>  EXPORTS +=	gfxPlatformMac.h \
>  		gfxQuartzSurface.h \
>  		gfxQuartzImageSurface.h \
>  		gfxQuartzPDFSurface.h \
>  		gfxAtsuiFonts.h \
> +		gfxCoreTextFonts.h \
>  		gfxQuartzNativeDrawing.h \
>  		$(NULL)
>  
>  endif

Use instead:

  ifdef MOZ_CORETEXT 
  EXPORTS += gfxCoreTextFonts.h
  endif

>  ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
>  CPPSRCS	+= \
>  	gfxQuartzSurface.cpp \
>  	gfxQuartzImageSurface.cpp \
>  	gfxQuartzPDFSurface.cpp \
>  	gfxPlatformMac.cpp \
>  	gfxAtsuiFonts.cpp \
> +	gfxCoreTextFonts.cpp \
>  	$(NULL)

Use instead:

  ifdef MOZ_CORETEXT
  CPPSRCS += gfxCoreTextFonts.cpp
  endif

Trim out conditionals at the start of gfxCoreTextFonts.h and gfxCoreTextFonts.cpp.

> +#define DUMP_TEXT_RUNS

This should be commented out by default.

> +    printf("Using %s for font & glyph shaping support\n",
> +           mUseCoreText ? "CoreText" : "ATSUI");

Remove.

>  nsresult
>  gfxPlatformMac::GetFontList(const nsACString& aLangGroup,
>                              const nsACString& aGenericFamily,
>                              nsTArray<nsString>& aListOfFonts)
>  {
> -    gfxQuartzFontCache::SharedFontCache()->
> -        GetFontList(aLangGroup, aGenericFamily, aListOfFonts);
> +#ifdef HAVE_CORE_TEXT
> +#ifndef __LP64__
> +    if (mUseCoreText) {
> +#endif
> +        CTFontCollectionRef allFontsCollection =
> +            CTFontCollectionCreateFromAvailableFonts(NULL);
> +        CFArrayRef allFonts =
> +            CTFontCollectionCreateMatchingFontDescriptors(allFontsCollection);
> +
> +        nsString nameStr;
> +        PRUint32 numFonts = CFArrayGetCount(allFonts);
> +        for (PRUint32 i = 0; i < numFonts; i++) {
> +            CTFontDescriptorRef desc = (CTFontDescriptorRef) CFArrayGetValueAtIndex(allFonts, i);
> +            CFStringRef name = (CFStringRef) CTFontDescriptorCopyAttribute(desc, kCTFontDisplayNameAttribute);
> +
> +            CFIndex nameLen = CFStringGetLength(name);
> +            nameStr.SetLength(nameLen);
> +            CFStringGetCharacters(name, CFRangeMake(0, nameLen), nameStr.BeginWriting());
> +            aListOfFonts.AppendElement(nameStr);
> +            CFRelease(name);
> +        }
> +
> +        CFRelease(allFonts);
> +        CFRelease(allFontsCollection);
> +        return NS_OK;
> +#ifndef __LP64__
> +    }
> +#endif
> +#endif /* HAVE_CORE_TEXT */
> +
> +#ifndef __LP64__
> +    // can't reach here on 64-bit because we always have CoreText
> +    gfxQuartzFontCache::SharedFontCache()->GetFontList(aLangGroup, aGenericFamily, aListOfFonts);
> +
>      return NS_OK;
> +#endif
>  }

This code belongs in gfxQuartzFontCache::GetFontList not here.  And I don't think this change is completely right.  GetFontList returns a list of localized family names based on the list of families we maintain.  Assuming CTFontCollectionCreateFromAvailableFonts is returning a list of font families (not font faces) which is the same as Cocoa, it's still going to be slightly different from our list because of things like Osaka Mono (see InitSingleFaceList).  I think we need to consistently use a single family list, otherwise weird anomalies will crop up.

It looks like you didn't make any changes to gfxQuartzFontCache.cpp for this patch.  This means we're still dependent on the ATSUI API ATSUGetIndFontName, used in ReadOtherFamilyNamesForFace, as noted in comment 21.  The latest patch attached to bug 468387 adds name table reading routines to gfxFontUtils.  So we can switch to those in the future and eliminate this dependency.

> +    ATSFontRef GetATSFont() {
> +        return mATSFont;
> +    }
> +
> +    CTFontRef GetCTFont() {
> +        return mCTFont;
> +    }
> +

> +    mATSFont = FMGetATSFontRefFromFont(aFontEntry->GetFontID());

> +    if (fe && !fontGroup->HasFont(FMGetATSFontRefFromFont(fe->GetFontID()))) 

Since in the future we want to be using ATSFontRef's instead of ATSUFontID's, I think we should change the code in MacOSFontEntry to use ATSFontRef id's and fix up the code in gfxAtsuiFonts.h/cpp to cast these to ATSUFontID's and eliminate all the FMGetATSFontRefFromFont calls in code we'll be using going forward.

I'm going to do some testing with this next week, I'll post more comments then.

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: updated based on jdaggett comments

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to comment #24)
> There's a lot of code duplication and that's
> unfortunate, it creates a maintenance headache and there's a natural tendency
> for things to get out of sync.  We could introduce a superclass for all MacOSX
> fonts (gfxOSXFont/gfxOSXFontGroup?) and put shared code there but I realize
> that has it's own problems.

Yes, I wondered about this, too. One reason I didn't go that way at first was to avoid any risk of disrupting the existing code by refactoring, but it's a question we should consider.

> > +    printf("Using %s for font & glyph shaping support\n",
> > +           mUseCoreText ? "CoreText" : "ATSUI");
> 
> Remove.

I've put this inside #ifdef DEBUG; if you don't mind, I'd like to leave it there for now, as I find it handy to confirm which path is being used (e.g., after capturing all console output to a log file).


> This code belongs in gfxQuartzFontCache::GetFontList not here.

Agreed. This was left from the original patch I started from, but I have now removed it entirely, as it isn't part of Core Text rendering. Updating gfxQuartzFontCache will happen in other bugs anyway.


In the new version, I've marked several reftests as "random" because currently Core Text gives us failures due to minor glyph positioning and/or antialiasing issues. In at least one case, there's clearly an Apple bug involved. The problems are minor enough that they wouldn't prevent us shipping with Core Text enabled, IMO.

There's one remaining reftest failure, layout/reftests/bugs/474417-1.html, that I have not marked as known-fail or random, pending further investigation. (It seems that the presence of U+FEFF is causing a problem here.)

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to comment #26)

> > This code belongs in gfxQuartzFontCache::GetFontList not here.
> 
> Agreed. This was left from the original patch I started from, but I have now
> removed it entirely

Just noticed a stray comment that got left here by mistake; it will disappear in the next iteration!

Comment 28

[John Daggett (:jtd)]

> > > +    printf("Using %s for font & glyph shaping support\n",
> > > +           mUseCoreText ? "CoreText" : "ATSUI");
> > 
> > Remove.
> 
> I've put this inside #ifdef DEBUG; if you don't mind, I'd like to leave
> it there for now, as I find it handy to confirm which path is being used
> (e.g., after capturing all console output to a log file).

This means that every developer will see this console output.  Better to
use the DEBUG_<xxx> where <xxx> is your username, if you look at the
build system automatically enables this for just this purpose, that's
while you see DEBUG_roc or DEBUG_tor in various places.

Comment 29

[John Daggett (:jtd)]

Attachment: testcase, sample text for 10.5 fonts (some Vista)

Generates sample text for all font faces under 10.5, for use in comparing metrics.

Comment 30

[John Daggett (:jtd)]

Attachment: screenshot, 474417 reftest

screenshot comparing 474417 reftest with (1) FF 3.0.7 (2) build with coretext patch (3) latest trunk build, with pixie enlargement of coretext rendering.  The metrics seem to be off and there's some sort of funky glyph rendering going on.

Comment 31

[John Daggett (:jtd)]

Comments so far:

gfxAtsuiFonts.cpp:
>  mMetrics.strikeoutOffset = mMetrics.xHeight / 2.0;

gfxCoreTextFonts.cpp:
>  mMetrics.strikeoutOffset = mMetrics.xHeight;

Is there are reason for changing this calculation? Or is this just an
oversight?  Other than this, the metrics calcs seem to produce the same
results ATSUI vs. CoreText.

The use of mSyntheticBoldOffset in InitMetrics also seems a bit
different.  Not sure it matters, just curious why.

gfxAtsuiFonts.cpp:
>  mMetrics.maxAdvance = atsMetrics.maxAdvanceWidth * size + mSyntheticBoldOffset;
>  mMetrics.aveCharWidth += mSyntheticBoldOffset;

gfxCoreTextFonts.cpp:
>  mMetrics.maxAdvance = atsMetrics.maxAdvanceWidth * size + mSyntheticBoldOffset;

+PRBool
+gfxCoreTextFont::TestCharacterMap(PRUint32 aCh)
+{
+    return mIsValid && GetFontEntry()->TestCharacterMap(aCh) ? PR_TRUE : PR_FALSE;
+}

This reduces to:

  return mIsValid && GetFontEntry()->TestCharacterMap(aCh);

In gfxCoreTextFontGroup::GetGlyphRunsFromLine:

+        nsAutoArrayPtr<CGGlyph> glyphsArray;
+        nsAutoArrayPtr<CGPoint> positionsArray;
+        nsAutoArrayPtr<CFIndex> glyphToCharArray;
+        const CGGlyph* glyphs = NULL;
+        const CGPoint* positions = NULL;
+        const CFIndex* glyphToChar = NULL;
+
+        glyphs = CTRunGetGlyphsPtr(run);
+        if (glyphs == NULL) {
+            glyphsArray = new CGGlyph[numGlyphs];
+            if (!glyphsArray)
+                break;
+            CTRunGetGlyphs(run, CFRangeMake(0, 0), glyphsArray.get());
+            glyphs = glyphsArray.get();
+        }

Seems like you should be able to eliminate the heap allocation here; use
a nsAutoTArray<CGGlyph,1000> for glyphsArray, do
AppendElements(numGlyphs), then pass glyphsArray.Elements() into
CTRunGetGlyphs.  Same for positionsArray and glyphToCharArray.  That
will allocate the default elements on the stack (e.g. 1000) and only use
heap-allocated memory when needed.  Maybe 512 is a better default size,
not sure what average text run is typically.

I'm going to finish up this review tomorrow, I need to go over
GetGlyphRunsFromLine a little more.

Comment 32

[John Daggett (:jtd)]

BTW, have you run through the Talos pagesets with CoreText enabled?  Any idea what the speedup/slowdown is like?

Comment 33

[Jonathan Kew [:jfkthame]]

(In reply to comment #30)
> Created an attachment (id=368860) [details]
> screenshot, 474417 reftest
> 
> screenshot comparing 474417 reftest with (1) FF 3.0.7 (2) build with coretext
> patch (3) latest trunk build, with pixie enlargement of coretext rendering. 
> The metrics seem to be off and there's some sort of funky glyph rendering going
> on.

Yes, it seems that U+FEFF causes some weird glyph rendering problem; I've seen this locally on that test (see comment #26), and also on the tryserver (which I discovered is running builds and unit tests on 10.5), though the exact kind of distortion was a bit different (probably dependent on the particular fonts installed).

I'm going to try to reproduce this in a small test app and report it to Apple, but meanwhile we should be able to work around it in our code.

Comment 34

[Jonathan Kew [:jfkthame]]

(In reply to comment #32)
> BTW, have you run through the Talos pagesets with CoreText enabled?  Any idea
> what the speedup/slowdown is like?

Not sure if this is the same pageset as Talos, but I ran roc's large page-load test (~500 top sites or whatever) back in December, and it was showing an overall speedup of about 1% .... slight, but it was reasonably consistent over multiple runs.

Comment 35

[Jonathan Kew [:jfkthame]]

(In reply to comment #31)

> In gfxCoreTextFontGroup::GetGlyphRunsFromLine:
> 
> +        nsAutoArrayPtr<CGGlyph> glyphsArray;
> +        nsAutoArrayPtr<CGPoint> positionsArray;
> +        nsAutoArrayPtr<CFIndex> glyphToCharArray;
> +        const CGGlyph* glyphs = NULL;
> +        const CGPoint* positions = NULL;
> +        const CFIndex* glyphToChar = NULL;
> +
> +        glyphs = CTRunGetGlyphsPtr(run);
> +        if (glyphs == NULL) {
> +            glyphsArray = new CGGlyph[numGlyphs];
> +            if (!glyphsArray)
> +                break;
> +            CTRunGetGlyphs(run, CFRangeMake(0, 0), glyphsArray.get());
> +            glyphs = glyphsArray.get();
> +        }
> 
> Seems like you should be able to eliminate the heap allocation here; ...

I've run an instrumented build for a while, including the page_load_test, and have not yet seen a case where CTRunGetGlyphsPtr fails to get a direct pointer to the glyphs within the run. So it seems that in practice, the array is used very rarely if at all.

(I also checked the length of the glyph runs, and was getting 99% of glyph runs with fewer than 100 glyphs. So if we do use an nsAutoTArray, a moderate size such as 128 would seem sensible.)

I'm not sure this is worth doing at all, though, as:
a) it seems that getting the direct pointer normally succeeds, so we don't need our own array at all;
b) at a guess (I have no data to support this), the direct access may be most likely to fail on extremely large runs, if CoreText does something different internally in this case - but then we'd probably have to do an allocation anyway;
c) defining the three nsAutoTArrays, even though we don't normally need them, could have a negative impact because the larger stack frame will have an effect on CPU cache usage, as memory accesses will be spread further apart.

WDYT? On balance, I'm inclined to leave this code as is. The real win is in using the direct pointer accessors, which avoid the need to copy the data, as well as avoiding any allocation of a place to copy it to.

Comment 36

[John Daggett (:jtd)]

Comment on attachment 368498 [details] [diff] [review]
updated based on jdaggett comments

> I've run an instrumented build for a while, including the page_load_test, and
> have not yet seen a case where CTRunGetGlyphsPtr fails to get a direct pointer
> to the glyphs within the run. So it seems that in practice, the array is used
> very rarely if at all.

OK, I looked at the API docs a little closer today and that seems fine. 
What a peculiar API, whatever...

> gfxCoreTextFontGroup::GetGlyphRunsFromLine(gfxTextRun *aTextRun,
>                                            CTLineRef aLine,
>                                            const PRPackedBool *aUnmatched,
>                                            PRInt32 aLayoutStart,
>                                            PRInt32 aLayoutLength)
> {
>     // Here, the textRun has been bidi-wrapped (if necessary);
>     // aLayoutStart and aLayoutLength define the range of characters
>     // within the textRun that are "real" data we need to handle
> 
>     static const PRInt32 DIR_LTR = 1, DIR_RTL = -1;
>     PRInt32 dir = aTextRun->IsRightToLeft() ? DIR_RTL : DIR_LTR;
>     CFArrayRef glyphRuns = CTLineGetGlyphRuns(aLine);
>     PRUint32 numRuns = CFArrayGetCount(glyphRuns);
> 
>     // Iterate through the glyph runs. Note that they may extend into the wrapper
>     // area, so we have to be careful not to include the extra glyphs from there
>     for (PRUint32 runIndex = 0; runIndex < numRuns; runIndex++) {
>         CTRunRef run = (CTRunRef)CFArrayGetValueAtIndex(glyphRuns, runIndex);
>         PRInt32 numGlyphs = CTRunGetGlyphCount(run);
>         if (numGlyphs == 0)
>             continue;
> 
>         // skip the run if it is entirely outside the real text range
>         CFRange stringRange = CTRunGetStringRange(run);
>         if (stringRange.location >= aLayoutStart + aLayoutLength ||
>             stringRange.location + stringRange.length <= aLayoutStart)
>             continue;

So this method basically consists of a giant loop, one pass for each
run.  I think it would be better to move the loop into InitTextRun and
call the bulk of this function once per run (call it SetGlyphsFromRun?).

Several nits:

+        static const PRInt32 NO_GLYPH = -1;
+        nsAutoTArray<PRInt32,128> charToGlyphArray;
+        if (!charToGlyphArray.SetLength(stringRange.length))
+            break;

So 128 is our guess for the text run length that covers 90% of the
cases?  Let's put this in a #define and give a name.

+    nsAutoArrayPtr<PRPackedBool> unmatchedArray;

+            if (unmatched == NULL) {
+                unmatchedArray = new PRPackedBool[aTotalLength];
+                if (unmatchedArray != NULL) {
+                    unmatched = unmatchedArray.get();
+                    memset(unmatched, PR_FALSE, aTotalLength*sizeof(PRPackedBool));
+                }
+            }

Use nsAutoTArray<PRPackedBool, xxx> instead.  Use constant defined above.

+    static const PRInt32 DIR_LTR = 1, DIR_RTL = -1;
+    PRInt32 dir = aTextRun->IsRightToLeft() ? DIR_RTL : DIR_LTR;

Create a boolean isLTR and use that instead of always comparing with dir (e.g. dir == DIR_RTL).

+        PRInt32 *charToGlyph = charToGlyphArray.Elements();
+        for (PRInt32 offset = 0; offset < stringRange.length; ++offset)
+            charToGlyph[offset] = NO_GLYPH;

Use memset here instead.

+            while (1) {
+                // at the top of this loop, charEnd points to (NOT beyond) the current end of the clump;
   .
   .
+                if (firstGlyphFound) {
+                    // When we have found the starting glyph, it's a valid clump.
+                    break;
+                }
+            }

This is the same as do { } while (!firstGlyphFound) which seems clearer.

+            // for missing glyphs, we already recorded the info in the textRun
+            if (aUnmatched && aUnmatched[baseCharIndex] == PR_TRUE) {
+                glyphStart = glyphEnd;
+                charStart = charEnd;
+                continue;
+            }

+            {
+                if (aUnmatched == NULL || aUnmatched[baseCharIndex] == PR_FALSE)
+                    aTextRun->SetSimpleGlyph(baseCharIndex - aLayoutStart,
+                                             g.SetSimpleGlyph(advance, glyphs[glyphStart]));
+            } else if (aUnmatched == NULL || aUnmatched[baseCharIndex] == PR_FALSE) {
+                // collect all glyphs in a list to be assigned to the first char;

The second check for unmatched characters seems redundant, the code is
already checking for unmatched characters and continuing to the next
iteration.  The value of baseCharIndex doesn't change between these two
blocks of code.  Also, the '== PR_TRUE' is not needed.

+PRBool
+gfxCoreTextFont::TestCharacterMap(PRUint32 aCh)
+{
+    return mIsValid && GetFontEntry()->TestCharacterMap(aCh) ? PR_TRUE : PR_FALSE;
+}

Don't need the conditional expression here.

Do you have testcases for assuring that we hit all the various branches
within the loop in GetGlyphRunsFromLine?  Have you tried writing a quick
fuzzer-like JS program to flush out possible problems?  If not, I slap
together something quick and dirty tomorrow.

Within GetGlyphRunsFromLine it seems to me there's a 90% fast-path case,
where there's one glyph-per-character and everything is a simple glyph. 
Seems like detecting that isn't too hard.  I'll let you decide but it
appears we're running through a rather large loop per glyph when the
simple case would be just a few lines.  But I guess correctness should
really be first.

> Not sure if this is the same pageset as Talos, but I ran roc's large page-load
> test (~500 top sites or whatever) back in December, and it was showing an
> overall speedup of about 1% .... slight, but it was reasonably consistent over
> multiple runs.

OK, good.  Bummed that we're not seeing a speedup but I guess no
regressions are good too.  ;)

Comment 37

[Jonathan Kew [:jfkthame]]

(In reply to comment #36)

Thanks for the feedback; will work through these points and update.

Just a couple of things where I wondered if you'd reconsider?

> +    static const PRInt32 DIR_LTR = 1, DIR_RTL = -1;
> +    PRInt32 dir = aTextRun->IsRightToLeft() ? DIR_RTL : DIR_LTR;
> 
> Create a boolean isLTR and use that instead of always comparing with dir (e.g.
> dir == DIR_RTL).

The reason for creating dir with values of +/- 1 was so that in the inner loop finding ligature clumps, we can do

+                charEnd += dir;

without needing a test at that point. (A bit like other code in gfxFont.cpp, for example, creates a gfxFloat with value +/- 1.0 and then multiplies advance values by this.) But I guess it's not a big deal.


> +        PRInt32 *charToGlyph = charToGlyphArray.Elements();
> +        for (PRInt32 offset = 0; offset < stringRange.length; ++offset)
> +            charToGlyph[offset] = NO_GLYPH;
> 
> Use memset here instead.

Surely using memset() to fill an array of PRInt32 is not strictly correct? We could get away with it, given that the NO_GLYPH value we're using happens to be -1, but that feels hacky to me.


> Bummed that we're not seeing a speedup but I guess no
> regressions are good too.  ;)

Yeah. :)  Actually, considering that text rendering performance is only one part of what gets tested by page-load, even a 1% gain overall (some runs gave up to 2%, but I can't claim that consistently) isn't so bad.

Note that we get some definite wins on actual rendering behavior: for a good example, look at http://alanwood.net/unicode/combining_diacritical_marks.html and compare the ATSUI vs CoreText results.

Comment 38

[John Daggett (:jtd)]

> > +    static const PRInt32 DIR_LTR = 1, DIR_RTL = -1;
> > +    PRInt32 dir = aTextRun->IsRightToLeft() ? DIR_RTL : DIR_LTR;
> > 
> > Create a boolean isLTR and use that instead of always comparing with dir (e.g.
> > dir == DIR_RTL).
> 
> The reason for creating dir with values of +/- 1 was so that in the inner loop
> finding ligature clumps, we can do
> 
> +                charEnd += dir;
> 
> without needing a test at that point. (A bit like other code in gfxFont.cpp,
> for example, creates a gfxFloat with value +/- 1.0 and then multiplies advance
> values by this.) But I guess it's not a big deal.

Sorry for not being more clear.  Using dir as +1/-l is fine but all the
dir == DIR_LTR comparisons seems awkward, a simple boolean set based on
IsRightToLeft() seems better.

> Surely using memset() to fill an array of PRInt32 is not strictly
> correct? We could get away with it, given that the NO_GLYPH value we're
> using happens to be -1, but that feels hacky to me.

Hmmm, set the constant to 0xFFFFFFFF then?  Not that big a deal either
way, but memset is easier to read than an extra loop.

Comment 39

[John Daggett (:jtd)]

Noticed one more this morning:

+   // the rest of the chars in the group are ligature continuations, no associated glyphs
+   while (++baseCharIndex != endCharIndex) {
+       if (baseCharIndex - aLayoutStart >= aLayoutLength)
+           break;

This is equivalent to

    while (++baseCharIndex != endCharIndex && (baseCharIndex - aLayoutStart) < aLayoutLength) {

Comment 40

[John Daggett (:jtd)]

I did a quick build with some code coverage logging in GetGlyphRunsFromLine:

  http://pastebin.mozilla.org/636423

Running all the reftests hits these points (look for COVERAGE macro in
code snippet above), with RTL or LTR rendering:

  4 rtl
  5 ltr
  6 rtl
  7 ltr
  8 rtl
  9 ltr
  11 rtl
  13 rtl
  
Points 1, 2, 3 are the cases where the CoreText API's don't return
arrays and require explicit array allocation.  This is a failure case,
probably don't need to worry about tests to invoke these paths.  Points
5, 6 are only hit in either the LTR or RTL case.

So I think it would be good to have tests that cover these additional
situations:

  4, ltr
  7, rtl
  8, ltr
  9, rtl (is this possible?)
  10, ltr
  10, rtl
  11, ltr
  12, ltr
  12, rtl
  13, ltr

Comment 41

[John Daggett (:jtd)]

Attachment: testcase, text snippets to try and exercise all codepaths

Note the incorrectly rendered combining diacritic, this is a regression from existing behavior.

With this testcase + reftests, missing coverage is

  8, rtl
  12, ltr
  12, rtl

Comment 42

[Jonathan Kew [:jfkthame]]

(In reply to comment #41)

> Note the incorrectly rendered combining diacritic, this is a regression from
> existing behavior.

I assume you're referring to the rendering of "figuŗ̃e", which should have a tilde over the 'r' and a cedilla beneath. Yes, I see an incorrect result here: the 'r' is missing, the tilde is over the 'u', and the cedilla is above that!

This appears to be a bug in CoreText, not in our code; it does not handle the diacritics properly when they are in a non-canonical order, and the later diacritic can combine with the base character. Inspecting the glyph and position arrays returned from the CTRun shows that the 'r' is missing. The glyph IDs returned for the word are {219, 96, 110, 375, 383, 94}, which are {fi, g, u, tildecmb, cedillacmb, e} in Futura; there is no sign of the 'r'.

Changing the test from

    figuŗ̃e

to

    figuŗ̃e

(i.e. sorting the diacritics according to their canonical combining class values) gives correct rendering; the run returns glyph IDs {219, 96, 110, 364, 375, 94}, which correspond to {fi, g, u, rcedilla, tildecmb, e}. Note that the combination <r, cedilla> has been combined into a precomposed glyph. This is evidently failing in the non-canonically-ordered case. Note also that if you replace the 'r' with a character that doesn't have a precomposed glyph with cedilla, the problem does not occur.

I'll file a bug report with Apple about this (the same problem can easily be demonstrated in TextEdit.app, for example). The failure depends on three conditions, AFAICT: (1) text uses multiple combining diacritics rather than precomposed accented letters where possible; (2) diacritics are not in canonical order; and (3) the font has a precomposed glyph for base + a non-initial diacritic.

Meanwhile, we could work around this by ensuring the text is normalized before passing it to CoreText. But I suspect that the particular situation that fails will be vanishingly rare in real-life web pages. Note that if the text were normalized (either as NFC or NFD) the problem would not arise. The only common cases of non-normalized text are in some non-Latin scripts like Arabic (ordering of shadda and vowel marks, for example), where there are no precomposed forms available in normal fonts.

In view of this, I think we should not burden our code with a normalization check at this point, unless we find that (a) Apple doesn't fix the CoreText bug, and (b) we see evidence that it affects real pages.

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to comment #42)

> I'll file a bug report with Apple about this (the same problem can easily be
> demonstrated in TextEdit.app, for example).

Filed in Radar as issue 6720723.

Comment 44

[John Daggett (:jtd)]

(In reply to comment #42)
> In view of this, I think we should not burden our code with a normalization
> check at this point, unless we find that (a) Apple doesn't fix the CoreText
> bug, and (b) we see evidence that it affects real pages.

Ok, that's fine but we should log a bug on this since it will appear to be a regression in our code (renders ok in 1.9.1, doesn't render in 1.9.2).

Comment 45

[Jonathan Kew [:jfkthame]]

Attachment: updated based on review comments; resolve rendering problem with U+FEFF

Comment 46

[Jonathan Kew [:jfkthame]]

The problem with U+FEFF in reftest 474417-1 (see comments 26, 30) arises because of the following sequence of events:

The text contains "A<U+FEFF>B", where U+FEFF is ZERO-WIDTH NO-BREAK SPACE (though now deprecated in favor of U+2060 WORD JOINER), and is being drawn in the default font, which resolves to Times on my system.

Our font-matching determines that Times does not support U+FEFF, and searches for a font that does include this character. It finds Thonburi (a Thai font), at least on my fairly "plain" Leopard system. So we call AddGlyphRun three times, for the initial Times run, the Thonburi run, and the following Times run. So far so good.

Then we pass the whole attributed string to CoreText, ask it to lay out the line, and retrieve the resulting glyphs. With ATSUI, this gave us the expected three glyph runs (although the Thonburi one, which contains only ZWNBSP, is invisible).

CoreText, however, decides to eliminate the ZWNBSP glyph altogether, and returns only two glyph runs, both in the Times font; the ZWNBSP glyph is entirely missing from the returned array.

This means that when we store the glyph data, it looks exactly as if "A<zwnbsp>" has formed a single-glyph ligature, with both characters being associated with the single "A" glyph. However, the two characters were allocated to separate glyph runs because they were assigned different fonts during the font-matching process. This means that the "ligature" will end up being drawn in two parts, and the second part is drawn using the Thonburi font (but using the glyph ID of "A" in Times).

To solve this, I have added a step to "sanitize" the glyph runs after retrieving the glyph data from CoreText; basically, any glyph run that begins at a character that is a trailing part of a ligature will have its starting offset advanced beyond the ligature (as we cannot possibly switch fonts in the middle of a real ligature!) If this results in the run becoming zero-length, it is eliminated completely. With this change, the test with ZWNBSP renders correctly.

An alternative approach would be to defer calling AddGlyphRun until we have retrieved the glyphs from CoreText and can see whether it really uses all the runs we thought we needed. However, this gets cumbersome for two reasons: (a) when we get the glyph run data back from CoreText, it doesn't directly tell us which gfxFont to use, so we would have to retrieve that from the CTFontRef, which is extra work I don't want to do in the inner loop here; (b) we'd have to store missing-glyph info somewhere until after calling CoreText, instead of calling SetMissingGlyph as soon as we find an unsupported character, as we can't call that until the glyph runs have been set. So on balance, I think it's simpler to call AddGlyphRun up front, and then tidy them up afterwards (which will very rarely be necessary in practice).

I have added a reftest specifically checking that ZWNBSP does not disrupt rendering. I'm also modifying 474417-1 to remove the need for ZWNBSP there, as that was not really the point of the test. It turns out, however, that <wbr> (the subject of that test) has another issue that I will file separately; the use of ZWNBSP there was masking the fact that <wbr> disrupts kerning.

Comment 47

[Jonathan Kew [:jfkthame]]

(In reply to comment #41)

> With this testcase + reftests, missing coverage is
> 
>   8, rtl

To hit this, you need a RTL character for which you have no font coverage. N'Ko (e.g, U+07D0) works for me on this machine, but you might have to uninstall extra fonts in order to ensure you hit it.

Alternatively, you could put directional overrides around your "missing" character:
  [‮﬈‬]

>   12, ltr
>   12, rtl

I'm not sure this situation could actually arise, but the condition was included because otherwise the loop there looks as though it might have the potential to be infinite, perhaps if CoreText gives us bad glyph/index data somehow. (Having merged the loop termination conditions (as per comment #39) there is no longer a separate branch anyway.)

Comment 48

[Jonathan Kew [:jfkthame]]

(In reply to comment #44)

> Ok, that's fine but we should log a bug on this since it will appear to be a
> regression in our code (renders ok in 1.9.1, doesn't render in 1.9.2).

Filed bug 485244.

Comment 49

[John Daggett (:jtd)]

Comment on attachment 369334 [details] [diff] [review]
updated based on review comments; resolve rendering problem with U+FEFF

Looks good.  Plus'ing this with one condition:

+    // InitMetrics will create the mCTFont (possibly taking account of sizeAdjust)
+    InitMetrics();

Need to add the validity check after this, same as the gfxAtsuiFont code:

    if (!mIsValid) {
        return;
    }

It would be nice to have a single reftest that tried to exercise as many
subsections of SetGlyphsFromRun as possible, so that when changes are
made in this code we can hopefully catch off-by-one errors that only
occur in non-typical text runs.  We obviously can't catch all of these
but at least we can try to catch some of these.  A simple != against
about:blank would be fine.  This can be set up in a separate patch.

Comment 50

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

Great! Jonathan, if you prepare a final patch for checkin, we can get this landed.

Comment 51

[Jonathan Kew [:jfkthame]]

Attachment: updated patch ready for checkin

Added the mIsValid check (comment 49); also verified that the patch still applies cleanly (and builds successfully!) with updated trunk.

Comment 52

[Josh Aas]

pushed to mozilla-central, thanks Jonathan and John!

http://hg.mozilla.org/mozilla-central/rev/e413694940b3

We should probably modify the title of this bug to refer to an implementation, close it out, and open a new bug on enabling Core Text for 32-bit builds.

Comment 53

[Jim Jeffery not reading bug-mail 1/2/11]

This was backed out with changeset: 
http://hg.mozilla.org/mozilla-central/rev/4374b8db1a6e
http://hg.mozilla.org/mozilla-central/rev/1913e22b2562

Should this be re-opened ?

Comment 54

[Markus Stange [:mstange]]

Yes.

The 10.4 Talos machines were crashing. I can reproduce the crash on my 10.4 machine when I load http://www.whatwg.org/specs/web-apps/current-work/.
Stack trace is here: http://pastebin.mozilla.org/637216
It's a "pure virtual method call" crash at NS_ADDREF(oldfm);.

Comment 55

[Jonathan Kew [:jfkthame]]

I managed to borrow a PPC machine running 10.4, and built trunk + coretext patch on that, but have not been able to reproduce the crash as yet.

Markus, is your build that shows the failure PPC or Intel? If it's PPC, could you send me a copy to run locally? I don't have access to 10.4 on Intel... :(

Comment 56

[Markus Stange [:mstange]]

It's on Intel.

Comment 57

[Jonathan Kew [:jfkthame]]

Attachment: updated EliminateDuplicateFaces

Turns out EliminateDuplicateFaces was not working correctly with the updated ATSFontRef-based font handling; this update fixes that. I don't know if this may have been a factor in the failures on 10.4, as I have not yet been able to reproduce this locally.

Comment 58

[Markus Stange [:mstange]]

That seems to have fixed it. I haven't been able to crash with the new patch.

Comment 59

[Jonathan Kew [:jfkthame]]

Awesome - thanks for testing!

I'll mark this as ready for re-landing, then, and expect this to fix the Talos bustage we saw.

Comment 60

[Josh Aas]

updated patch pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/588938aa8808