475092 - drag image for text with vertically-offset glyphs is garbled

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Jonathan Kew [:jfkthame]]

Attachment: screenshot of Urdu text being dragged

When dragging a range of selected text that includes glyphs (often diacritics) with vertical offsets, the "ghost" image of the text that is created has misplaced glyphs: it looks as if neighboring glyphs to those with vertical offsets actually get shifted *horizontally* from their correct positions.

This can be seen when dragging a selected fragment of text in Urdu script (which involves vertically-offset glyphs) on the page at <http://pakdata.com/firefox-complex-script-font-rendering-problem>, for example. A screenshot is attached showing the effect; compare the correctly rendered text on the page itself (with blue highlighting) with the drag-image version above (garbled).

This problem does not occur on Linux or on Mac OS X.

I suspect this is another manifestation of the problem mentioned in bug 454098#c41, where a reftest fails because the image generated has a misplaced glyph, even though it looks correct when viewing the page in a browser window.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: another screenshot of a bad drag image (Latin text with diacritics)

This screenshot shows text from the bug 454098 test file, attachment 358178 [details], being dragged on Vista; note the horizontally misplaced glyphs in the neighborhood of the vertically-shifted accents.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: fix for horizontal glyph positioning bug in cairo win32 font backend

This turns out to be an error in cairo-win32-font, in the _cairo_win32_scaled_font_backend version of show_glyphs. This code collects glyph runs to hand to ExtTextOutW until the y-offset changes, then flushes the glyphs buffered so far. As each glyph is buffered, it also calculates and buffers the dx value for the preceding glyph.

However, when it sees a change in dy and decides to flush, it should *not* append an entry to the dx buffer, as this would be the "dx" of the previous glyph, and instead the new start_x value will be used for the new glyph run that's being collected. This bug means that after any vertically-offset glyph, the remaining glyphs in the run will get incorrect dx values (horizontal escapement).

The patch here resolves this, and the drag image is now correct. This also corrects the problem observed with a reftest image in bug 454098, as this code path is used when the reftest framework draws the page to a canvas.

After review, this patch needs to go upstream to Cairo as well.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: updated patch to include reftest (moved from bug 454098)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: updated for current trunk (merged change to gfx/cairo/README) - ready to land after bug 454098

Comment 5

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

Yeah, let's fix this on 1.9.1.

Jeff, can you upstream this to cairo?

Comment 6

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

Pushed http://hg.mozilla.org/mozilla-central/rev/360b4a2d0aa8

Comment 7

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

The test failed on Tinderbox. I'll back this out.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1233136218.1233143404.16550.gz#err0

Comment 8

[Jonathan Kew [:jfkthame]]

Drat. That test ran on Server 2003, didn't it? They don't have the font support for the combining marks, and that makes it go haywire. (The test failure is kind of irrelevant there as the text will look like junk anyway!) But I'll see if I can come up with something to solve this.

Comment 9

[(no longer active)]

If this test needs something special in the font, can we use a dynamic font and include the font file in the test suite to solve this issue?

Comment 10

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

That sounds like a good plan to me.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: revised reftest to avoid dependence on Vista fonts; code patch is unchanged

I've made a new reftest (moved it to layout/reftests/text, as it is no longer really bidi-related) that should be more robust. WFM on both XP and Vista, both with and without ClearType.

The test uses a couple of custom OpenType fonts via @font-face; one of the fonts uses a GPOS lookup to shift a glyph vertically, which will trigger this bug when drawn using semi-opaque color. The other font uses glyph substitution to achieve the same result, and will be unaffected by the bug.

Comment 12

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

Comment on attachment 359387 [details] [diff] [review]
revised reftest to avoid dependence on Vista fonts; code patch is unchanged

Fabulous!

Comment 13

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

Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/c7385f0a0168

Comment 14

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

The test 475092-pos.html failed on Linux, so I disabled it and filed bug 476137:
http://hg.mozilla.org/mozilla-central/rev/7f5292b5b9e2

Comment 15

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

Actually, the tests failed on Windows too, so I just backed this out:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1233304897.1233310200.1924.gz#err0
http://hg.mozilla.org/mozilla-central/rev/5d9f0b3f9bc3

Comment 16

[Jonathan Kew [:jfkthame]]

From the image in the log, it appears that the OpenType feature in the test font isn't being applied on the test machine (although it works fine for me on both XP SP3 and Vista). I suspect this means that the test box (running Windows Server 2003, I think?) has an older version of Uniscribe that doesn't apply features to Latin script text. Is there any way we can confirm details of the environment on those machines, in particular the version of USP10.DLL that's present?

I guess we have a couple of options: we could just punt and mark the test as random, given that the results depend on the system software environment; or I can have another shot at creating a test that works on older systems. If you want me to do that, I suppose I'd better request a Server 2003 license so I can try to reproduce the behavior locally.

Oh, one way to evade the issue would be to just compare the renderings of the -sub and -pos versions of the test file, and not compare them with the -ref at all. On systems that don't apply the OpenType features in Latin script, they'll match because they'll both be "wrong" in the same way! As currently written, the test is really testing two things at once: (a) that the OpenType features work, and (b) that the text with vertically-shifted glyphs is drawn correctly (this bug). If the features aren't working at all, then (b) is meaningless.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to comment #14)
> The test 475092-pos.html failed on Linux, so I disabled it and filed bug
> 476137:
> http://hg.mozilla.org/mozilla-central/rev/7f5292b5b9e2

Hmmm. It's basically doing the right thing, but a rounding or hinting issue is causing a 1-pixel offset between the glyphs that are shifted using GPOS and those that were "pre-shifted" within the font. That's either a Pango or FreeType-related issue, most likely, and difficult to resolve reliably. With a bit of trial and error we might be able to find a font size at which they happen to match up, but that's just hiding the issue rather than solving it.

Disabling the test on Linux would be OK for now, as the original bug we're trying to patch here is in Windows-only code anyway. I considered making the test Windows-only in the first place for that reason, but in principle it ought to pass on the other platforms as well. So it's interesting to know about the discrepancy on Linux, and we might want to try and improve that some day for better consistency. Perhaps this test should be marked TODO on Linux?

Comment 18

[Jonathan Kew [:jfkthame]]

OK, here's a suggested way forward for now. We have 3 versions of the file that draws "Hello World" with raised "o" glyphs:
-ref    requires no OpenType support, always draws raised "o"
-sub  uses OpenType substitution, works provided the shaping engine applies features
-pos  uses OpenType positioning, works provided the shaping engine applies features, but rendering is affected by this bug on Windows; on Linux, rounding/hinting causes test failure although behavior is basically correct.

So rather than testing "ref==sub" and "ref==pos", we can separate out the issues a bit:
(1) ref==sub  tests whether the shaping engine is applying OpenType features; we mark this as random on Windows because older Uniscribe doesn't handle it.
(2) sub==pos  tests whether the vertical positioning bug (this one) is fixed, provided OpenType features are working; we mark this as random (or todo, if that be flagged for a specific platform) on Linux because of the rounding discrepancy, which is a separate issue.

This should let the tests pass everywhere, I believe, and give us at least some test coverage. The Windows 2003 Server box won't really be testing this bug, unfortunately, but that's because it's not doing the glyph positioning at all so it can't be getting it wrong! Whenever the test is run on Vista it will be checking that this bug is fixed. (I guess some day Tinderbox will run unit tests on Vista as well? Seems like that would be a good thing.)

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: revised reftests again to try and make this stick; also added license (OFL) to the test fonts

Revised the test cases as described in comment #18; AFAICT this should allow the patch to re-land without test failures.

Also added license info to the test fonts; I used glyphs from SIL's Charis, so the Open Font License requires these derivative fonts to remain under the same license. (Just a formality really, it's not like anyone will want them!)

Comment 20

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

Comment on attachment 359741 [details] [diff] [review]
revised reftests again to try and make this stick; also added license (OFL) to the test fonts

OK

Comment 21

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

Oh, one more thing: can you update the patch to modify layout/reftests/fonts/README to add a description of these new fonts? Thanks

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: updated layout/reftests/fonts/README as requested

AFAIK this should be ready to re-land on trunk, and hope it stays there this time.

Comment 23

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

Pushed http://hg.mozilla.org/mozilla-central/rev/85d4ec9cbb8b

Comment 24

[(no longer active)]

Is this ready to land on 1.9.1?  The tests seem to have passed on trunk, IINM.

Comment 25

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

Yeah.

Comment 26

[(no longer active)]

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