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).
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.
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.
(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).
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.
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.
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.
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.