[Libreoffice] Horizontal glyph adjustments are ignored with ICU layout

[Khaled Hosny]

On Sat, Jan 08, 2011 at 11:14:49AM +0630, Keith Stribley wrote:
> Hi Khaled, Michael,
> 
> On 07/01/2011 8:21 PM, Michael Meeks wrote:
> >
> >	Drat; we took ages to reply. I suspect Thorsten might be able to help,
> >but failing that the best text expert I'm aware of is Tim (CC'd) who may
> >be able to give some advice. SIL's Graphite integration would
> >undoubtedly have fallen over the same sort of problems, and no doubt he
> >can give some pointers to how to work around it - Tim ? :-)
> 
> I've been working on the Graphite integration more recently, so I'll
> try to make a few comments.
> >
> >>Anyway, it turned out that the issue is not specific to kerning nor
> >>Arabic, but affects all horizontal glyph positioning in the ICU layout
> >>path; the problem does not show on Windows nor with Graphite fonts and
> >>of course not with con-CTL.
> It certainly sounds like it is worth fixing.
> >>The X adjustment of glyph widths is simply ignored, and glyphs are drawn
> >>stacked after each other baased on their original width, which results in
> >>kerning being ignored as well as other forms of glyph positioning like
> >>cursive anchors, however vertical glyph positions (in the Y access) are
> >>OK.
> 
> It depends on the rendering path, different parts of libo call the
> layout methods in different ways.

I've noted that at some stage, but after several hair pulling nights I
lost track of what is doing what.

> >>In source/glyphs/gcach_layout.cxx, ICU's layoutChars() is called and new
> >>glyph indices and positions are returned, which is then fed into
> >>SalLayout in the form of GlyphItem's. Though GlyphItem has maLinearPos
> >>which holds its absolute position, many places in the code re-calculate
> >>glyph position from its mnOrigWidth (original glyph width) and mnNewWidth
> >>(width after adjustments), but the ICU path simply sets mnNewWidth to
> >>mnOrigWidth since ICU layout engine does not return individual glyph
> >>widths, resulting in failure of glyph position re-calculation which
> >>manifests in this bug.
> >>
> >>As a prove of concept, the attached patch trays to set mnNewWidth in a
> >>very hackish way by subtracting current glyph position from the of next
> >>non-diacritic (+ve) glyph. It does indeed fix the problem (see the
> >>attached screenshots), but it looks very unreliable to me.
> 
> I think subtracting the glyph positions is a reasonable approach,
> something similar is done in the graphite integration code to get
> the numbers into a form that works with libo's logic.

There are two main issues I'm worried about; combining marks and
reordered glyphs.  Detection of combining marks currently is just a hack
(both on old code and the patch) and I'm not sure how reliable is to
assume all combining marks have non +ve width (I've seen fonts having
+ve width marks), ideally marks should be marked is such in GDEF table,
but ICU don't pass such information to us, nor do I know how +ve width
marks are handled.  Also glyph reordering can be an issue if ICU is
outputting glyphs in logical rather than visual order.

> >>May be a
> >>cleaner solution is to re-implement all the broken virtual methods to
> >>eliminate the re-calculation part.
> 
> Do you mean implement ICU/ServerFontLayout specific versions of the
> methods that ServerFontLayout is currently using directly from
> GenericSalLayout? I think that would be quite a lot of work and you
> may still need something like mnNewWidth calculated in a similar way
> to your patch. I admit that GraphiteLayout does reimplement those
> methods so if it turns out there is an assumption in
> GenericSalLayout which doesn't fit well with ICU then that might
> still make sense.

I see.

> The recalculations are done in several situations such as for
> justification, text expansion, text contraction. In writer the text
> is rendered once with a small font size and once with a large font
> size. It then does some calculations to try to give a more accurate
> WYSIWYG positioning of the text on screen which results in small
> adjustments being applied to the glyph positions (but the request to
> adjust is based on characters). It is probably the later which is
> interacting with the incorrect mnNewWidth to give the bad positions.
> 
> Looking more carefully at the patch, there are a few points that may
> need more consideration:
> 
> a) It may be possible for glyphs to be out of order, in which case
> 
> nNewWidth = pNextPos->fX - pPos->fX;
> 
> might result in a negative value even when the nominal
> nNextGlyphWidth is positive, which could cause problems.

I've thought of that too, but shouldn't glyph output be in visual order?

> b) The glyphs are resorted a bit later in
> IcuLayoutEngine::operator() with a rLayout.SortGlyphItems(); call.
> This might upset the new width values. However, it seems to only
> reorder diacritics and those are skipped for the width calculation,
> so that is probably alright.
> 
> c) For the last glyph in a run, pNextPos will still be valid (see
> LayoutEngine::getGlyphPositions docs), so I think it should be used
> for the nNewWidth calculation.

The attached batch should do that (plus some clean up over the previous
one).

> It should be tested with some CTL scripts including reordering and
> lots of diacritics to check the edge cases are covered. The width
> will also affect font fallback (calls from MultiSalLayout), so that
> needs to be tested as well.

I've tested with fonts that make extensive use of combining marks and no
major problems so far, however width of fallback glyphs seems to be
miscalculated when followed by kerned glyphs but I'm yet to understand
what is going on with fallback. Testing with scripts that require glyph
reordering still needed, but I don't speak any.

Regards,
 Khaled

-- 
 Khaled Hosny
 Arabic localiser and member of Arabeyes.org team
 Free font developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcach_layout.diff
Type: text/x-diff
Size: 2628 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110111/f047aaf0/attachment-0001.diff>