GNOME Bugzilla – Bug 118472
some ligatures substitutions wrongly skipped
Last modified: 2003-07-29 16:31:58 UTC
After the latest merge from freetype, ftxgdef.c, Lookup_LigatureSubst() does not handle apply the ligature substitution if either the ligature IDs don't match OR component IDs don't match. But two ligatures can be combined into a new ligature if such a gsub rule exists. So the correct thing is to skip handlig the ligature substitution if neither the ligature IDs NOR the component IDs match. This patch also fixes boundary condition error where we overrun the in->length. A patch and screenshots and utf test case is attached to this bug report.
Created attachment 18668 [details] utf testcase
Created attachment 18669 [details] bad rendering (using tunga.ttf)
Created attachment 18670 [details] correct rendering (with this fix)
Created attachment 18671 [details] [review] patch fix (cvs diff)
The revised check doesn't make much sense to me if you look at the comments. If the ligature ID's are different, comparing the components doesn't seem like a sensical operation. I suspect the original patch is just wrong. I'll revert it and ask Werner Lemberg if he remembers what he was trying to fix. As for the length checks (really should be a separate bug report, likely), I don't understand them; don't the checks: if ( in->pos + lig->ComponentCount > in->length ) continue; /* Not enough glyphs in input */ [...] if ( in->pos + sr[k].GlyphCount > in->length ) continue; /* context is too long */ [...] /* check whether context is too long; it is a first guess only */ if ( bgc > in->pos || in->pos + igc + lgc > in->length ) continue; Already take care of this?
Reverted out the ligature patch. If you are happy with the existing length checks, you can go ahead and close this bug, I think. Sat Jul 26 09:41:22 2003 Owen Taylor <otaylor@redhat.com> * pango/opentype/ftxgsub.c (Lookup_LigatureSubst): Revert back out the FreeType patch preventing ligatures of not-originally adjacent glyphs; it doesn't work for all scripts. (#118472, Kailash C. Chowksey)
This check before the loop is not sufficient because it does not take into account glyphs that might be skipped because CHECK_Property() said ignore them. if ( in->pos + lig->ComponentCount > in->length ) continue; /* Not enough glyphs in input */ eg take the string U+0C95,U+0CCD,U+0CB7,U+0CC3. I have attached wrong rendering and correct rendering with this.
Created attachment 18703 [details] bad rendering due to length overrun
Created attachment 18704 [details] correct rendering with no length overrun
OK, I see the issue. Yep, there is a problem there. I don't think your patch is quite complete: - It's necessary to check j before checking s_in[j] the first time, since j++ in the main loop might take you past the end. - There is an off-by-one in the existing code. if ( in->pos + j < in->length ) j++; else break; since after the increment in->pos + j == in->length which is one off the end. - There are *lots* more places that need fixing; I think about 25 total in ftxgsub.c and ftxgpos.c. (Help with this would definitely be appreciated!) I've filed a separate bug about this, bug 118592, and will close this bug, since the other problem is OK now.