Bug 5166 – Remaining cases where the ATSUI and CG code paths measure and render the same text differently

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

This is a follow up to bug 4940. The fix for that bug did not address the following cases where the 
same problem occurs:
1) ATSU applies kerning as specified by the font
2) ATSU uses ligatures (such as "fi") specified by the font
3) A fallback font is used whose rendering mode is different from that of the WebTextRenderer's 
original font

The attached testcase demonstrates all 3 problems. The first two lines demonstrate kerning. Notice the 
different width and appearance. Double-click  words in the second line to select them and notice how 
the selection highlight is in the wrong place. The next couple of lines show this in a different font. The 
next couple of lines demonstrate ligatures. The last couple of lines demonstrate the fallback font 
problem. Hebrew is rendered in Lucida Grande in both, but the metrics are different since Times and 
Lucida Grande don't use the same rendering mode.

Case (3) is probably the easiest to correct. For cases (1) and (2), dumbing down the ATSU code path to 
behave just like the CG code path may defeat that path's sole purpose, so if done it may have to be 
done carefully and selectively.

Finally, after the synthesized bold/oblique patch is landed, it might add another problematic case.

Created attachment 4077 [details]
testcase

Created attachment 4240 [details]
better testcase

The last part of the previous testcase was rubbish

Created attachment 4241 [details]
match rendering mode for overrides, disable kerning and ligatures in ATSU

Created attachment 4242 [details]
match rendering mode for overrides, disable kerning and ligatures in ATSU

Also use ATSU for highlighting and drawing if any of the characters before to
run.from require it.

Created attachment 4247 [details]
fix more ATSUI/CG differences

Comment on attachment 4247 [details]
fix more ATSUI/CG differences

Since small caps affect metrics, this patch implements them in the ATSU code
path. Synthetic bold is also taken into account to ensure consistent metrics,
but ATSU still doesn't "double-strike". The patch also implements synthetic
oblique (since it practically came for free with the synthetic bold). (I should
add a testcase for these features).

[Darin Adler]

Comment on attachment 4247 [details]
fix more ATSUI/CG differences

I think that turning off kerning and ligatures in the ATSU path is actually the
wrong direction -- in the long run I'd like to support kerning and ligatures in
our CG code path, but still keep the code path very fast. That doesn't mean I
this patch is unacceptable, but I would like to go the other direction as much
as possible in the future.

In disposeATSULayoutParameters, there's no need for the if before free; free is
defined to do nothing for 0.

Generally we only allocate with calloc when we need the "initialized with 0"
behvior, otherwise, we don't want to spend the extra time to zero the entire
buffer. So perhaps some of those calloc calls should just be malloc with
multiplication.

There is one place with two spaces after an =, a call to ATSUMatchFontsToText.
Should remove the extra space.

An question unrelated to this patch: can we use ATSUMatchFontsToText in
substituteFontForString in the future? I think ATSU's subsitute fonts might be
better than the ones we get from AppKit calls, and perhaps could wean us from
AppKit SPI.

I'm concerned about the performance impliciations of checking more characters
in the calls to shouldUseATSU. I understand the check is necessary, but we'll
need to test that this doesn't slow down key benchmarks.

Also, perhaps the rule of checking from the beginning of the run could be
inside shouldUseATSU instead of having to repeat it in two places.

Perhaps the attribute arrays should have size declared explicitly as 4; I think
that makes it slighly less likely the sizes will get ouf of sync with the size
parameter to ATSUSetAttributes.

I'm concerned that the heuristic about when to turn off ligatures is not good.
If there's some great Arabic font that also happens to include Roman characters
in it (perhaps it's a universal font that covers many characters), then I still
want the ligatures to work properly. We should probably contemplate another
heuristic.

Perhaps for a later patch, not this one: As long as we're copying the buffer
for the small caps case, we should also copy the buffer for runs that have
'\n', '\t', NBSP, or other control characters in them. Then '\n', '\t', and
NBSP can just be replaced with spaces; for other control characters perhaps we
can use zero-width spaces as the CG code path does. Or we could adjust offsets
and lengths and remove those other control characters.

Why does this patch omit the "render bold twice" part? It seems we could do
that relatively easily by calling ATSUDrawText twice; do we really need to
stage this in two separate patches?

I think these things should be done separately to make it easier to catch
performance regressions. So ideally one patch should add the multiple renderers
and small caps. A separate one could add synthetic oblique and bold. Still
another could turn off kerning and ligatures. And a final one could turn on the
more-correct check for when to use ATSU. This will make it much easier for the
people who test performance to notice effects of the individual changes.

I'd like to be more clear on which of these suggestions are mandatory and which
are optional, but my thinking on this is not so clear at the moment; instead I
want to get my feedback to you ASAP.

(In reply to comment #7)

> I think that turning off kerning and ligatures in the ATSU path is actually the
> wrong direction

Once the CG code path supports kerning and/or ligatures, turning them back on in the ATSU
code path should be very easy. Right now this patch is necessary because when the two
code paths don't match each other in all aspects that affect how text is measured,
it leads to buggy behavior. I also think that supporting features inconsistently (e.g.
one line has ligatures and the next one does not, for no apparent reason) is worse
than not supporting them at all (yet I don't expect this logic to be applied when it
limits the CG code path, line/word-spacing and justification being an example).

> An question unrelated to this patch: can we use ATSUMatchFontsToText in
> substituteFontForString in the future?

Probably.

> I'm concerned about the performance impliciations of checking more characters
> in the calls to shouldUseATSU. I understand the check is necessary, but we'll
> need to test that this doesn't slow down key benchmarks.

In my experience, run->from != 0 only in two cases: drawing the highlight for
a partially-selected run and drawing the drag image for a partially-selected run. I'll
be surprised if these code paths are even exercised in the benchmarks.

> Also, perhaps the rule of checking from the beginning of the run could be
> inside shouldUseATSU instead of having to repeat it in two places.

Agreed.

> I'm concerned that the heuristic about when to turn off ligatures is not good.
> If there's some great Arabic font that also happens to include Roman characters
> in it (perhaps it's a universal font that covers many characters), then I still
> want the ligatures to work properly. We should probably contemplate another
> heuristic.

I don't suppose overriding in the character morphing operation
(kATSULayoutOperationMorph) would do any good, since the override proc doesn't
seem to have any say on the actual morphing process.

> As long as we're copying the buffer
> for the small caps case

Oh, it's probably a good idea to merge addDirectionalOverride into
createATSULayoutParameters to avoid copying twice.

Another idea I had was to merge createATSUTextLayoutForRun into
createATSULayoutParameters.

> Why does this patch omit the "render bold twice" part? It seems we could do
> that relatively easily by calling ATSUDrawText twice; do we really need to
> stage this in two separate patches?

Since the ATSUTextLayout may point to several renderers, with only some of
them being synthetic bold, the override should be told when it's the second pass
and should substitute glyphs coming from a non-synthetic renderer with the space
glyph. It can be done in this patch, then again...

> I think these things should be done separately to make it easier to catch
> performance regressions. So ideally one patch should add the multiple renderers
> and small caps. A separate one could add synthetic oblique and bold. Still
> another could turn off kerning and ligatures. And a final one could turn on the
> more-correct check for when to use ATSU. This will make it much easier for the
> people who test performance to notice effects of the individual changes.

Only one of the above ("the more-correct check for when to use ATSU") affects
performance on the CG code path (then again, as I said, not really). The rest
affect only ATSU, which I thought goes unnoticed by the benchmark. How about this
order:
1) correct check for when to use ATSU (to fix highlighting bug in lines containing
combining marks)
2) multiple renderers and small caps (to fix more highlighting bugs and a very
noticeable rendering inconsistency)
3) turning off kerning and ligatures (for consistency and to fix more highlighting
bugs; might regress some Arabic)
4) adding synthetic oblique and bold (for consistency)

Optionally, do 2-4 in only one or two patch since they only affect ATSU performance.

You made a few other comments on the code, which I'll address when I update the patch.

[Darin Adler]

OK, all your comments make sense to me, and I think your plan is good.

The only part that still makes me uncomfortable is the heuristic about when to turn off ligatures. But if you 
think it will cause no actual problems in practice, then I think it's worth the risk.

Maybe the right thing to do is to investigate fast ways to render ligatures in the CG code path. Then we 
can just turn everything back on and be happy.

[Darin Adler]

(Right thing slightly longer term I mean.)

Created attachment 4965 [details]
1. correct check for when to use ATSU

Created attachment 4966 [details]
2. multiple renderers, small caps and correct metrics for synthetic bold

Created attachment 4967 [details]
3. disable kerning and some ligatures in the ATSUI code path

Created attachment 4968 [details]
4. render synthetic bold and oblique

General comment about the patches: the 4 patches are meant to apply to TOT after the patches for bug 
579 and bug 5878 are landed. Each patch is a diff from that with previous patches applied, but will also 
merge independently, except for patch 4 which needs patch 2.

Comment on attachment 4965 [details]
1. correct check for when to use ATSU

(see also comment #15)

Comment on attachment 4966 [details]
2. multiple renderers, small caps and correct metrics for synthetic bold

(see also comment #15). Merged createATSUTextLayout with
createATSULayoutParameters. Also eliminated applyMirroringToRun.

Comment on attachment 4967 [details]
3. disable kerning and some ligatures in the ATSUI code path

(see also comment #15)

Comment on attachment 4968 [details]
4. render synthetic bold and oblique

(see also comment #15). Perhaps this should check for the special case where a
single renderer is used for the entire run and that renderer uses synthetic
bold (then a simple "double-strike" will do).

[Darin Adler]

Comment on attachment 4968 [details]
4. render synthetic bold and oblique

I think we'll get uninitialized variable warnings for syntheticBoldOffset and
spaceGlyph with this patch as-is. Otherwise looks fine to me.

Comment on attachment 4968 [details]
4. render synthetic bold and oblique

I verified that the compiler (gcc 4.0) doesn't warn about those variables. If
it's still a concern, r- again and I'll add initializers.

[Darin Adler]

Comment on attachment 4967 [details]
3. disable kerning and some ligatures in the ATSUI code path

I'm still skeptical about this rule about when to turn off ligatures. I wish we
could figure out a super-fast way of doing ligatures in CG instead.

The "covers the letter a" heuristic seems weak to me. If the makers of the
Geeza fonts chose to revise them to include more characters, then the heuristic
would break.

But I don't have a better suggestion.

[Darin Adler]

Comment on attachment 4966 [details]
2. multiple renderers, small caps and correct metrics for synthetic bold

It seems strange to mix "bool" and "NO". "BOOL" goes with "NO" and "YES", and
"bool" goes with "true" and "false", and "Boolean" goes with "TRUE" and
"FALSE".

r=me

(In reply to comment #15)
> bug 579

I meant to write bug 5479.

Once that fix is landed, I should split this bug into 3 or 4 separate bugs, each with a testcase and the 
relevant patch in commit-ready form.

[Eric Seidel (no email)]

Comment on attachment 4967 [details]
3. disable kerning and some ligatures in the ATSUI code path

It would be nice to have a radar/bugzilla number with your comment.  Also,
having the test case as a patch is most helpful.

Comment on attachment 4965 [details]
1. correct check for when to use ATSU

Now part of attachment 5134 [details] in bug 6132.

Comment on attachment 4967 [details]
3. disable kerning and some ligatures in the ATSUI code path

Now part of attachment 5139 [details] in bug 6137.

Comment on attachment 4966 [details]
2. multiple renderers, small caps and correct metrics for synthetic bold

Now part of attachment 5140 [details] in bug 6139.

Comment on attachment 4968 [details]
4. render synthetic bold and oblique

Now part of attachment 5140 [details] in bug 6139.

Comment on attachment 4968 [details]
4. render synthetic bold and oblique

Cleared review flags to remove this bug from the commit queue.