Closed Bug 475092 Opened 11 years ago Closed 11 years ago

drag image for text with vertically-offset glyphs is garbled

Categories

(Core :: Graphics, defect, P2)

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jfkthame, Assigned: jfkthame)

References

()

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 5 obsolete files)

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.
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.
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.
Assignee: nobody → jfkthame
Attachment #358571 - Flags: review?(vladimir)
Attachment #358571 - Attachment is obsolete: true
Attachment #358860 - Flags: review?(vladimir)
Attachment #358571 - Flags: review?(vladimir)
Flags: blocking1.9.1?
Keywords: checkin-needed
Whiteboard: [needs landing]
Yeah, let's fix this on 1.9.1.

Jeff, can you upstream this to cairo?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Pushed http://hg.mozilla.org/mozilla-central/rev/360b4a2d0aa8
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
The test failed on Tinderbox. I'll back this out.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1233136218.1233143404.16550.gz#err0
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [needs 191 landing]
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.
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?
That sounds like a good plan to me.
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.
Attachment #359022 - Attachment is obsolete: true
Attachment #359387 - Flags: review?(roc)
Comment on attachment 359387 [details] [diff] [review]
revised reftest to avoid dependence on Vista fonts; code patch is unchanged

Fabulous!
Attachment #359387 - Flags: superreview+
Attachment #359387 - Flags: review?(roc)
Attachment #359387 - Flags: review+
Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/c7385f0a0168
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
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
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [needs 191 landing]
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.
(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?
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.)
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!)
Attachment #359387 - Attachment is obsolete: true
Attachment #359741 - Flags: review?(roc)
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
Oh, one more thing: can you update the patch to modify layout/reftests/fonts/README to add a description of these new fonts? Thanks
AFAIK this should be ready to re-land on trunk, and hope it stays there this time.
Attachment #359741 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed http://hg.mozilla.org/mozilla-central/rev/85d4ec9cbb8b
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Is this ready to land on 1.9.1?  The tests seem to have passed on trunk, IINM.
You need to log in before you can comment on or make changes to this bug.