481948 - Indic scripts: consonant clusters with REPHA have glyphs disappearing

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Shreevatsa R]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.7) Gecko/2009021906 Firefox/3.0.7
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.7) Gecko/2009021906 Firefox/3.0.7

In several Indic scripts including Devanagari, consonant clusters that start with a RA must be displayed with a "repha" over the last consonant in the cluster. On Firefox on Mac OS X (which uses ATSUI), intermediate consonants disappear when this happens. This is handled correctly by Safari, and by Firefox on Windows and Linux.

A Devanagari example ("irkkkkkkkkkka"): इर्क्क्क्क्क्क्क्क्क्क ("RA VIRAMA KA VIRAMA KA VIRAMA...")
This contains 10 "k"s, but only 2 are displayed. (Copy-paste it into Textedit to see that all 10 are present.)

I'm not actually sure how corresponding text must be displayed for other Indic scripts; e.g. it appears to me that for the Kannada "irkkkkkkkkkka": ಇರ್ಕ್ಕ್ಕ್ಕ್ಕ್ಕ್ಕ್ಕ್ಕ್ಕ , Safari's rendering is wrong as well. In fact, in Kannada it seems acceptable to avoid the repha entirely (see "Issue 2" at http://www.baraha.com/help/kb/unicode_issues.htm) but for Devanagari it is true that the repha must follow the syllable (all consonants, and possibly a vowel sign: http://rishida.net/scripts/indic-overview/ )

Reproducible: Always

Steps to Reproduce:
1. Look at the text इर्क्क्क्क्क्क्क्क्क्क in Firefox
Actual Results:  
Two 'k's are visible

Expected Results:  
Ten 'k's must be visible

Generalization of Bug 428317 and Bug 429064

Comment 1

[Jonathan Kew [:jfkthame]]

Note that this bug is specific to the ATSUI font backend. The CoreText backend for OS X 10.5 (see bug 389074) displays the Devanagari example (and similar testcases) correctly, with all 10 'k' glyphs visible.

However, we still need to fix this for the ATSUI backend for 1.9.1 (and 1.9.2 if we don't drop 10.4 support), as it is a pretty serious case of incorrect rendering.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: rewrite of char/glyph clustering algorithm for ATSUI backend

This bug arises because of how PostLayoutCallback in the ATSUI backend tries to find character and glyph "clumps"; it does not handle Indic-rearrangement cases where a glyph crosses over an arbitrarily long run of others. When the REPHA glyph is eventually encountered, it gets clumped with the visually-adjacent consonant, but the underlying character range is much larger and should include all the logically-intermediate characters (visually preceding glyphs).

This patch resolves the problem by reimplementing the character/glyph clumping algorithm; it is essentially a copy of gfxCoreTextFontGroup::SetGlyphsFromRun, adapted to work with the ATSUI glyph information records. With this patch, arbitrarily long Indic syllables with glyph rearrangement are rendered properly without dropping glyphs.

If this works well in practice (it reftests fine; just sent to tryserver for further testing), it would strengthen the case for refactoring/merging the ATSUI and CoreText backends, as it harmonizes a major piece of the code that was formerly quite different.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 370172 [details] [diff] [review]
rewrite of char/glyph clustering algorithm for ATSUI backend

John, requesting review on this (you'll recognize the code!) as it fixes a serious issue for Indic text; I think we should consider whether we can take it on 1.9.1. It passes reftests locally and also the various try-server tests.

For 1.9.2, we should either drop the ATSUI path (if we decide to drop 10.4 support) or merge the ATSUI and CoreText backends; I'd be happy to do that in a followup patch for trunk.

Comment 4

[Jonathan Kew [:jfkthame]]

Shreevatsa, if you'd like to try this, you can download a build from

https://build.mozilla.org/tryserver-builds/2009-03-31_03:[e-mail]-atsui-clusters/

I'd appreciate any testing we can get.

Comment 5

[Shreevatsa R]

Yes, it appears to be fixed in that build. (I tried the Mac build.)

Thanks,

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 370172 [details] [diff] [review]
rewrite of char/glyph clustering algorithm for ATSUI backend

Roc, could you review this, as I guess John hasn't had time to look at it yet? Would be nice to be able to consider taking this on 1.9.1 if possible.

Comment 7

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

I'm too zonked to grapple with this now, but I'll do it ASAP.

Comment 8

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

+            if (charEnd == (PRInt32)glyphRecords[glyphStart].originalOffset/2)
+                firstGlyphFound = PR_TRUE;

Instead of doing this, can't we just find the correct end character by starting at glyphRecords[glyphStart].originalOffset/2 and finding the next character that has a charToGlyph entry? And then do another loop over those characters to compute glyphEnd? (In fact, maybe that can be fused with the following code "// We also need to check if there are any following glyphs"...) It seems to me that would be a little clearer.

+            g.SetComplex(glyphsInClump > 1 ?
+                             PR_FALSE : aTextRun->IsClusterStart(baseCharIndex),

glyphsInClump == 1 && aTextRun->IsClusterStart(baseCharIndex)
?

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: updated patch with revised char/glyph-clumping algorithm

Revised the code in the light of your comment; I think this is now clearer, and it also works better (the previous version did not always avoid "holes" in the glyph clump, which leads to an inferior selection behavior when drag-selecting across Indic text, for example).

This is looking good on reftests (and general browsing of int'l sites); was also going well on mochitest until it crashed somewhere deep in a JavaScript test, which I think is unrelated to this patch. Awaiting tryserver results to check that performance is still ok there.

Comment 10

[Jonathan Kew [:jfkthame]]

New tryserver build is available at

  https://build.mozilla.org/tryserver-builds/2009-04-13_14:[e-mail]-atsui-rev/

This passed tests ok, and seems to work well for me locally.

Roc, WDYT about considering this for 1.9.1? Given that it fixes an issue where text display is not just a bit sub-optimal, but actually misses out glyphs entirely, I think it's serious.

Comment 11

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

I think we should probably take it, but Vlad should be the one to approve it.

Comment 12

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

Let's let this bake on the trunk for a bit, and then let's get it into 1.9.1 -- I'm a little worried due to the area of code the patch is touching, but I agree that the bug is bad enough that we should try to fix it.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: further update to the clustering behavior [Checkin: Comment 18]

After further testing (in particular, see bug 488364), here is a further revision to the clustering and character/glyph association behavior. This now reproduces the prior ATSUI-based behavior for character clustering in the presence of Indic reordering, and provides the same partial-ligature behavior. There are now no known deviations from the previous behavior even in the bizarre colored-span/reordered-glyphs cases; the only user-visible change is that the disappearing glyph problem is resolved.

It is possible to make this code behave more like the Pango backend, rendering Indic clusters in sections just like ligatures, by simply disabling the test of /inOrder/ near the end of PostLayoutCallback. This is a behavior change we might like to consider in the course of re-examining shaping backends.

As the previous patch hadn't actually landed on trunk, we should take this version instead.

Comment 14

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

Comment on attachment 372976 [details] [diff] [review]
further update to the clustering behavior
[Checkin: Comment 18]

Have you got some tests we should add here?

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: add reftests for Indic cluster and ligature display issues [Checkin: Comment 18]

The first reftest tests the specific Indic display bug here; the subsequent tests check other cluster-related display issues that showed up in the course of working on this (see also bug 488364).

Comment 16

[Jonathan Kew [:jfkthame]]

BTW, the test 481948-3 will currently fail on the CoreText path, so I guess it should be marked random on OS X. I'm working on a fix for this and will post that on this bug as well, as it is also an Indic clustering-related issue.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: update CoreText clustering to behave similarly to ATSUI [Checkin: Comment 18]

This patch updates the CoreText backend based on the improvements in the new ATSUI algorithm. With this patch, all the reftests pass under CoreText as well.

Comment 18

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

ATSUI patch: http://hg.mozilla.org/mozilla-central/rev/a8713095a52b
Coretext patch: http://hg.mozilla.org/mozilla-central/rev/f44d624f624c
Reftests: http://hg.mozilla.org/mozilla-central/rev/135d06ed1540

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 372976 [details] [diff] [review]
further update to the clustering behavior
[Checkin: Comment 18]

a191=beltzner

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 373073 [details] [diff] [review]
add reftests for Indic cluster and ligature display issues
[Checkin: Comment 18]

a191=beltzner

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: combined ATSUI patch for 1.9.1, including tests and regression fix from bug 490777

The ATSUI clustering update turned out to have a regression, see bug 490777. So we need to take that fix on 1.9.1 as well, to avoid regressing there.

This version of the patch is updated for the 1.9.1 tree and incorporates the tests and the fix from 490777. Requesting review on the combined patch just to double-check that it's all in order; this should now apply cleanly and be ready for check-in on 1.9.1.

Comment 22

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

Comment on attachment 376170 [details] [diff] [review]
combined ATSUI patch for 1.9.1, including tests and regression fix from bug 490777

This is fine but unless you've changed something here I think it's probably better to just check in the patch for this bug and the patch for the regression as separate changesets (but in the same push). That way I think it's more clear how what lands on branch is related to what landed on trunk.

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: ATSUI clustering patch for 1.9.1, ready for landing [Checkin: Comment 24]

The actual code patches are unchanged from trunk (just offset by a few lines), but the test list patches needed to be updated for 1.9.1. So this patch is now ready to land on branch, and the corresponding patch from bug 490777 should land at the same time.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 376483 [details] [diff] [review]
ATSUI clustering patch for 1.9.1, ready for landing
[Checkin: Comment 24]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c03c2a41f0a0