Issue 113301 - hyphenation within Graphite ligatures can freeze
Summary: hyphenation within Graphite ligatures can freeze
Alias: None
Product: gsl
Classification: Code
Component: code (show other issues)
Version: DEV300m84
Hardware: All All
: P3 Trivial (vote)
Target Milestone: OOo 3.3
QA Contact: issues@gsl
Depends on:
Blocks: 111112
  Show dependency tree
Reported: 2010-07-21 04:35 UTC by nemeth.lacko
Modified: 2010-09-07 11:58 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation in: ---
Developer Difficulty: ---

Hyphenation within the ffj ligatures with positional/kerning problem (58.51 KB, image/png)
2010-07-21 04:37 UTC, nemeth.lacko
no flags Details
test file (9.69 KB, application/vnd.oasis.opendocument.text)
2010-07-21 04:39 UTC, nemeth.lacko
no flags Details
patch to avoid calling graphite with zero length and avoid using cached segments stradling ligature (3.70 KB, patch)
2010-07-24 15:09 UTC, devel
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description nemeth.lacko 2010-07-21 04:35:39 UTC
I have tested the latest development version of OOo 3.3, and the hyphenation
works within ligatures, too. I have found only a frequent positional problem
here, see the attached screenshot. I was be able to fix it by modifying the
character formatting of the related words, but now freezes, when
I modify the character formatting the red characters only in the end of the line

The attached test file uses the following extension: (Hungarian dictionaries,
an older version with hyphenation within ffj)

and the following Graphite font with the "ffj" and "ff" ligatures:
Comment 1 nemeth.lacko 2010-07-21 04:37:13 UTC
Created attachment 70728 [details]
Hyphenation within the ffj ligatures with positional/kerning problem
Comment 2 nemeth.lacko 2010-07-21 04:39:25 UTC
Created attachment 70729 [details]
test file
Comment 3 2010-07-21 09:29:21 UTC
It would be interesting to know whether the problem still happens in the latest Graphite CWS (it is 
"approved by QA, but not integrated yet):
Comment 4 devel 2010-07-21 15:39:41 UTC
Unfortunately the problem does seem to be present in CWS graphite03. Looking at
it in the debugger, OutputDevice::GetTextWidth is being called with a zero
length at a position in the middle of the paragraph. This seems to cause
Graphite to try to use an out of bounds array index, which causes a crash, when
I test it, rather than a freeze.
The solution is probably to trap this case and avoid calling graphite, though I
need to be careful that the rest of the GraphiteLayout can handle the
consequences of not having a segment.

The positioning issue with the hyphenated word being rendered assuming the width
as if the ligature spanning the hyphenation point is still present will probably
be much harder to fix.
Comment 5 devel 2010-07-24 15:09:11 UTC
Created attachment 70800 [details]
patch to avoid calling graphite with zero length and avoid using cached segments stradling ligature
Comment 6 devel 2010-07-24 15:24:50 UTC
The patch prevents the crash by avoiding calling graphite when mnMinCharPos ==
mnEndCharPos. On windows the example document didn't crash when I changed the
format of the highlighted text, but that seems to be because it happened to use
an existing longer segment from the cache.
The positioning bug appears to be due to the cache of Graphite Segments held by
OOo. Normally, it is alright to use a cached segment which is for a longer
string than was requested, in the CTL case such context is often necessary to
get correct positions. However, in this case the cached segment had the
ligature, whereas the requested text does not due to the hyphen break. The patch
adds code to detect this case by querying the cached segment to determine if the
character at mnEndCharPos-1 maps to a glyph and if so whether that glyph spans
characters outside the requested range. If there is no glyph mapped for that
character or the glyph is a ligature crossing the run boundary, then the cached
segment is ignored and a new segment created with just the requested text.
There seems to be a remaining problem with the hyphenation highlighted in red,
because the top right hand end of the ff ligature is clipped by the rendering of
the background for the hyphen. I haven't yet tracked down where the background
is drawn from to see if it is possible to prevent that happening.
Comment 7 nemeth.lacko 2010-07-24 16:22:18 UTC
kstribley: I have tested your patch, and it works fine. Thanks,  László

Clipping by background color is a problem at character format boundaries, when I
use white background colors for paragraphs to solve other typographical issues
(bad repeating of left-top background image of a paragraph, when there is a page
break within the paragraph).
Comment 8 devel 2010-07-26 07:13:03 UTC
SwTxtPainter::DrawTextLine calls the Paint method on each portion in the line in
sw/source/core/text/itrpaint.cxx line 421

For a hyphenated word, the text before the hyphen is an SwTxtPortion and so the
paint is SwTxtPortion::Paint.
sw/source/core/text/portxt.cxx line 575
The hyphen itself is derived from an SwExpandPortion, whose paint method is:
sw/source/core/text/porexp.cxx line 108

Both of these paint methods call rInf.DrawBackBrush() before they paint the
text. The way to avoid the clipping, would presumably be to separate the
painting of the background (DrawBackBrush) and the painting of the text
(DrawText). This would require 2 loops over the portions in the line, one for
the background and one for the actual text. However, this might have a
performance impact.
This isn't graphite specific, so I suspect this would be better handled as a
separate issue.  @hdu are you aware of an existing issue for the clipping problem?
Comment 9 2010-07-26 08:04:54 UTC
@md or ul: I suggest a OOo33 target for this issue. Ok?

@kstribley: Thanks for analyzing the hang/crash and providing the patch to fix it. When the target is 
known I'll create a CWS graphite04 for it, feel free to push the fix there.

Regarding the clipping problem this is probably already known as issue 56015 or issue 94493.
Comment 10 mdxonefour 2010-07-26 08:33:04 UTC
@hdu: From my point of view this is worth to be a stopper for 3.3 so I'm adding
it to the meta issue and adjust it's target.
Comment 11 2010-07-26 12:45:51 UTC
Good. As soon as OOO330_m2 (which will contain CWS graphite03) is released the CWS graphite04 will be 
Comment 12 2010-07-27 14:22:28 UTC
CWS graphite04 is created and its repository will show up in the next hour or so on
Comment 13 2010-07-28 10:01:35 UTC
@kstribley: do you prefer to commit+push the fix yourself or should I do it?
Comment 14 2010-07-28 13:47:46 UTC
Applied in CWS graphite04
Comment 15 devel 2010-07-28 15:21:03 UTC
@hdu: thanks for applying the patch. I had problems with my connection today, so
still waiting for the pull.
Comment 16 2010-07-29 08:48:29 UTC
@sba: please verify in CWS graphite04
Comment 17 2010-07-30 13:28:39 UTC
@es: please verify in CWS graphite04
Comment 18 eric.savary 2010-08-04 12:18:16 UTC
@HDU: Cannot reproduce a crash or freeze in the master.
Please verify per code review.
Comment 19 2010-08-04 12:48:28 UTC
Ok in CWS graphite04.
Comment 20 2010-09-07 11:58:44 UTC
Got into OOO330_m5 => closing