Closed Bug 454098 Opened 12 years ago Closed 11 years ago

U+0654 causes the word to be rendered above its position

Categories

(Core :: Layout: Text and Fonts, defect, P2)

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: ehsan, Assigned: jfkthame)

References

(Blocks 1 open bug, )

Details

(Keywords: fixed1.9.1, regression)

Attachments

(6 files, 5 obsolete files)

2.28 KB, patch
Details | Diff | Splinter Review
597 bytes, text/html
Details
4.83 KB, text/plain
Details
10.82 KB, image/png
Details
17.02 KB, image/png
Details
4.39 KB, patch
Details | Diff | Splinter Review
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080907202621 Minefield/3.1b1pre

In Persian and Arabic text, any word ending with the "Arabic Hamza Above" character (U+0654) is rendered above its intended position.

For an example, check out <https://addons.mozilla.org/fa/firefox>.  At the top of the page, the word "تجربهٔ" below the big red "افزودنی‌های فایرفاکس" text in the page header is rendered above the correct position.

Another observation is if you start selecting the text, the selected parts of the word appear in the correct position as long as the U+0654 is not included in the selection.  The same goes with highlighted find bar results (which use DOM selections).

This is a serious regression in 1.9.1 for all of the Arabic and Persian speaking users, so I'm requesting the blocking flag here.
Flags: blocking1.9.1?
FWIW, this doesn't seem to happen in Linux.
Can someone give me a hint on how to start debugging this issue?  I'd like to work on it if possible.
I would begin by checking out mYOffset in the detailedGlyphs array in SetGlyphs() in gfxWindowsFonts.cpp

I thought this was a dupe, but I can't find the original.
break on gfxWindowsFonts::InitTextRun and set conditional breakpoints so that it breaks when we're creating the textrun for the word you care about. Textruns are cached so you may find it useful to zoom in/out to force new textruns to be generated for the new size of the text.

Then inspect the gfxTextRun object that gets created to see where the final glyph is positioned. That's a bit tricky but you'll figure it out. That will tell us whether the bug is in textrun construction or somewhere else. If it's in textrun construction then you'll have to dig into gfxWindowsFonts and our interaction with Uniscribe.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Simon, can you take this? If not, unset yourself and I'll look for another owner. Ehsan's busy with private browsing stuff I think :-)
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
In comment 3 I was thinking of bug 418790. I'm not sure if it's a dupe or not: the testcase at http://pakdata.com/firefox-complex-script-font-rendering-problem refers to a number of different issues, some of which are fixed already, but it also includes problems with vertical positioning in Arabic with the Nafees Nastaliq font.

Ehsan, do you see this bug with all fonts? I haven't been able to reproduce yet.
Blocks: 418790
Firstly, I'm not sure if this is related to bug 418790 or not, but I guess with this reftest things will be a bit easier to investigate.

So, I tried to reduce the problem as observed on the AMO page to a reftest, and I found out that the problem only happens with the Verdana font.  Now, Verdana itself doesn't have any Arabic/Persian characters, so it will be replaces by the default font (usually Times New Roman) when rendering.  In theory, this should have the same effect as if the auther had specified a non-existing font family, but Gecko fails to substitute the font correctly.  In this refrest, I compare the rendering of the same text with Verdana and NonExistingFont to that of the default font.  And I do that for both the case where U+0654 occurs at the end and in the middle of the text to demonstrate that the problem affects even words without U+0654 (and also to demonstrate that the position of U+0654 could lead to different rendering problems).

This still fails on hg tip.  It should be easy to reproduce because this test uses only system default fonts.

I was under the impression that this has regressed from 1.9, but I found out that 1.9 doesn't render the Verdana case exactly correctly.  However, the mis-positioning under 1.9 was subtle so it made it impossible for the eye to detect this problem in usual page texts.

I tested this under 1.8 as well, and found out that it has the same rendering as 1.9 (which to me is still incorrect).  Anyway, I think the 1.8/1.9 behavior here is way better than that of 1.9.1, and I think that Verdana/NonExistingFont renderings should exactly match as well.
Jonathan Kew, can you reproduce this?
Not yet; I need to get a Windows dev system up as it seems to be OS- and font-specific. Will email you re that.
Got that Windows system up yet?
I have a recent build running on XP (haven't done Vista yet, though I guess I could give that a try too). I don't see the problem described on the https://addons.mozilla.org/fa/firefox page; words ending with the hamza-above look fine.

I do see a difference in positioning on the proposed reftest; the lines using NonExistingFont appear slightly higher, suggesting that they are reporting a smaller ascent value, and the hamza falls slightly outside the paragraph border, whereas the lines that specify Verdana fit neatly within.

So I can partially reproduce this; there's something odd about the line height metrics we're getting, apparently. But I haven't seen the mis-positioning of words ending with U+0654.
Ehsan, could you see if you can reproduce it in XP? Maybe this is Vista only?
I could swear that I had seen this in XP as well, but I just tested it on another machine with XP and it doesn't have this problem.  I tested with 3.1b2 on both machines (XP and Vista), so yes I think this is Vista-specific.
Jonathan, are you supposed to be getting Vista soon?
I have a Vista license; will try to get that set up in another VM and test a current FF build to see if I can reproduce the problem. I'll work on setting that up today and see how things go.
I can't seem to access the AMO/fa page at the moment, but I can reproduce this with a local test case on Vista.

It looks as if the problem may have something to do with the situation where the requested font doesn't include the characters used, and we fall back to another font; we may still be using line-height-related metrics (such as font ascent/descent) from the font that the document specified, and if these differ from the substituted font, vertical positioning is disrupted.

Not sure yet exactly what the interaction is, though.
But wrong. That probably does account for some vertical positioning discrepancies we see (on all platforms, I think), but the U+0654 issue is different. I can reproduce it with a sample that's entirely specified as Times New Roman, which does support all the characters involved and so no fallback should be happening.

It seems that we get the problem when U+0654 is the last (leftmost) character of a run of text in the Times font. Not sure yet if other fonts may suffer from the same problem.

In Ehsan's test case, the line that specifies Verdana behaves differently from the line that specifies NonExistentFont because in the latter case, the entire line *including spaces* falls back to Times, and so the word-final U+0654 is not at the end of the style run, whereas in the former, the spaces are still Verdana but the Arabic letters in the words themselves fall back to Times, and so we get a font-run boundary at the end of every word.
OK, this is not specifically related to any one font, or to that particular character, or even to Arabic script. It seems to be an error in how we handle the positioning data that we get back from Uniscribe, though I don't have a precise analysis and a patch yet.

I can reproduce this using the Vista Times New Roman font with Latin-script text and diacritics, if I use bidi overrides to make it run right-to-left. The problem occurs when the leftmost glyph in a font-run is shifted vertically by OpenType GPOS rules.

This happens with Arabic diacritics on the last character of a word, when using fonts like TNR or Scheherazade that have positioning data, and if the end of the word is also the end of the font run. But it could equally well occur in Hebrew, Thaana, Syriac, etc. It would probably also happen with a complex calligraphic Latin font that used GPOS to shift the *first* character of a word off the baseline, though I don't have an example of that on hand.
Assignee: smontagu → jfkthame
Is the selection behavior I described in comment 0 also explained by the textrun boundaries observation?
Yes, because the selected and non-selected fragments are drawn separately; the edge of the selection becomes a (temporary) boundary.
Patch to fix the problems with vertical positioning of textruns whose leftmost glyph is offset from the baseline. (Further discussion, reftests, etc to follow after more extensive testing of the patch; has not yet been tried on non-Windows platforms.)
Looks weird, so I'm looking forward to hearing the reasoning behind this :-)
Yes, I'm not thrilled about patching here. Another possibility is that we're actually seeing the result of a Cairo Windows backend bug; I haven't yet convinced myself one way or the other on that (need to write some test programs). If that's the case, though, this patch (just pushed to the try server) ought to break something for the other platforms, so we'll know!
The try server's not running tests though, AFAIK.
The documentation of the ETO_EDY mode in MSDN is rather vague, so I wouldn't be surprised to learn that there is a bug here in Vista or something like that. I presume you're making some simple Win32 program to test. If that is the case, then cairo is the right place to fix it --- in _cairo_win32_surface_show_glyphs, to be precise.
FWIW there have been bugs with vertical offsets in Cairo Windows before -- see bug 361782 (which has testcases in various scripts) and bug 365021.
Attaching a sample that illustrates the problem, without relying on Arabic or other "exotic" fonts.

The sample has Latin text with multiple diacritics; viewed on Vista, the version of Times New Roman there has GPOS positioning data to stack these. (The example will also work with Charis SIL.) The same line of text is displayed both left-to-right and right-to-left (by overriding the direction).

Note that both lines of text render correctly, with the diacritics properly stacked. This would seem to indicate that we are interpreting the glyph positions from Uniscribe properly, storing them into the textRun, and passing them through Cairo to ExtTextOutW successfully when painting the run.

Now try dragging slowly across the text to select it. With the left-to-right text, this works fine. However, with the right-to-left version, the text will jump up and down drastically as the selection passes over the characters with diacritics, depending where the edge of the selection falls within the string.

The exact behavior is that whenever the leftmost character of a fragment being painted (whether selected or not) is a vertically-shifted diacritic, that fragment will be vertically offset from its proper baseline. However, note that we are not rebuilding text runs during this process; we are just repainting fragments of the text run that we have already seen drawn with all glyphs correctly positioned.

The problem seems to occur when the first (leftmost) glyph being drawn via cairo_show_glyphs is offset from the current baseline. This is very unlikely to occur in LTR text, though in theory it is possible, but it can easily happen in RTL text because of the reversal that is done in GlyphBuffer::Flush(). I note that in Firefox 3.0.x, the glyph array was not reversed there, and that's why this issue was not seen in the earlier version; it would have required a font that uses vertical displacements of base characters in order to trigger such a situation.

Further investigation of cairo_show_glyphs() etc is ongoing....
BTW, digging into history indicates that the regression from 3.0.x behavior is a result of the fix to bug 452567, as that is when the reversal of RTL glyph arrays was added to Flush().

Thinking about this a little, I'm not sure that reversal is a good thing. It means that we are typcially painting each word of RTL text in reverse order. This may not matter as far as appearance is concerned, but in the case where the page is being drawn to a PDF, it will likely make it harder for people (or tools) to do anything useful with the text, such as copy/paste or search, as the glyphs will no longer be in logical order.
True. But i fwe don't reverse the glyphs, we get subtle rasterization differences in RTL vs LTR that mess up reftests.

Maybe we could take out the reversal, and rewrite those reftests to use fonts (possibly our own @font-face font) that avoid the rasterization subtleties.
Yes, I saw the reason why that was done. If that was the only motivation, then yes, I think re-examining the tests might be a better option.

Note that even if we do revert that reversal, though, the current bug won't disappear - it'll just be much less likely to show up with "normal" fonts, so it might look like it's no longer a problem. But we should really fix the underlying issue here.
The patch above is definitely not the right approach. After coming up with a comparable situation using an AAT font on the Mac (see bug 474819), I've confirmed that this is specific to the Cairo/Windows setup. Applying the patch here actually *breaks* the equivalent situation on the Mac.
Comment on attachment 358026 [details] [diff] [review]
patch to fix incorrect vertical positioning of words

Marking this patch obsolete, as it is the wrong place to correct the positioning, and breaks other platforms.
Attachment #358026 - Attachment is obsolete: true
OK, I think I know what's going on here, and should have a patch shortly.

I think the Cairo Windows backend handles vertical glyph offsets incorrectly (sign error) when passing them to GDI. We normally don't see any problem from this because our gfxWindowsFont compensates for that when it puts the glyphs into the gfxTextRun, so the glyphs still appear in the right places where a sequence of glyphs includes vertical deltas for glyphs other than the first.

(Note that this means we'd have problems if we used a gfxTextRun generated by a different gfxFont, but we don't have any way to "mix-and-match" font backends and Cairo backends, so being inconsistent about the direction of the gfxTextRun glyph position coordinates doesn't hurt us.... yet!)

However, this compensating pair of "vertical offset inversions" breaks down if the first glyph in the run that's passed to Cairo is not at the baseline of the text, because of the different coordinate system origins involved. The position of the first glyph becomes the starting point for the GDI ExtTextOutW call, but its offset from the text baseline (which in turn must be accounted for by coordinate system translation, I guess.... haven't traced all of that completely) is being interpreted incorrectly and so the entire glyph run gets offset.

Patching both Cairo and gfxWindowsFont to reverse the sign of vertical deltas resolves the problem we are seeing.

I'm about to try writing a demo program to prove that this is indeed a Cairo problem.... assuming that works as I expect, I'll report the bug upstream and produce the patch we need.
The attached demo program for W32 displays a window and draws a set of positioned glyphs using cairo_show_glyphs; the glyphs have different y positions.

When the glyphs are drawn individually using separate calls to cairo_show_glyphs, they are placed correctly. However, when they are drawn using a single cairo_show_glyphs call, only the first glyph is placed correctly; the subsequent glyphs are offset from this "baseline" in the wrong direction. (See screenshot to follow.)

This happens because of co-ordinate system inversion; _cairo_win32_surface_show_glyphs needs to negate the vertical deltas that it passes to ExtTextOutW.
Running an equivalent test program on OS X shows that the cairo-quartz backend does not have the same problem with vertical offsets in cairo_show_glyphs.
This patch resolves the issue described here in all its manifestations (both for Arabic/Persian as originally reported and the artificial RTL-Latin example). It also resolves the remaining problems with the Pakdata Urdu font as described in bug 418790; the test page referred to there now displays correctly.
Attachment #358457 - Flags: review?(roc)
Comment on attachment 358457 [details] [diff] [review]
patch for cairo (win32 backend) and thebes (windows fonts) to fix vertically-shifted glyphs

Great, thanks!! For the cairo part, we'll need to check this in locally until someone (Vlad?) can get this upstream. That means adding a patch to gfx/cairo like the other patches there, so please take care of that.

Seems to me with some creative font munging we could have a reftest for this. If anyone can do that easily, it'd be you, so it'd be great if you could have a go :-).
(In reply to comment #39)
> That means adding a patch to gfx/cairo like the other patches there, so please
> take care of that.

I mean, please submit a patch that adds those changes, and then we can put this bug to bed!
Here is a reftest based on the Vista version of Times New Roman. (With earlier fonts that don't include diacritic positioning information, it ought to simply pass as this bug won't be triggered.)

Unfortunately, the reftest currently fails even with the cairo + thebes patches applied. I cannot see any difference in the test and reference pages, even under high magnification; however, looking at the PNG data dumped in the reftest output shows that one of the images is incorrect there (a glyph is *horizontally* offset) even though it looks fine in the actual browser window! I suspect, therefore, that we have a separate bug in the graphics back-end that creates the bitmap image for comparison purposes. Does that involve drawing through a different kind of Cairo surface?

I'm actually getting a number of reftest failures on Windows at the moment, especially with ClearType enabled. Some of these may be tiny differences in antialiasing behavior (although I can't see them, in the cases I've looked at), but that doesn't seem to the the case with this one; this is a genuine bug somewhere, judging by the reftest PNG. I'll put together a demo and file it as a new bug, I guess.
It shouldn't; canvas is using a win32 surface just like normal painting.  However, it's using an ARGB surface, whereas most likely the normal painting path will be using RGB -- try editing the reftest code and setting "moz-opaque" attribute to "true" on the canvas element before calling getContext, and see if that makes a difference?
If your tests pass without ClearTyped turned on, I think you're being bitten by bug 469208.  I was when writing a test for bug 441782, and I ended up disabling the tests on Windows.
No, the issue here is different; I'm seeing major horizontal displacement of a glyph in the reftest image (but not when viewing the reftest directly in the browser). I believe this is a Windows graphics backend bug, not reftest framework, because it looks like the same thing happens when a drag image is created to drag some selected text. See bug 475092.
The patch at bug 475092 does indeed resolve the glyph displacement error seen in the reftest image. With that patch applied, the reftest is left with 2 differing pixels (invisible to my eye) on Vista, which is surely due to antialiasing coming out slightly differently depending on the order in which the glyphs are painted, or how they're split between ExtTextOutW calls.

(I think we need a way for reftests to be able to say "allow up to N differing pixels", as it may not always be feasible to write the tests in such a way that test and reference get painted identically by the platform, but these are not "real" errors.)
(In reply to comment #45)
> (I think we need a way for reftests to be able to say "allow up to N differing
> pixels", as it may not always be feasible to write the tests in such a way that
> test and reference get painted identically by the platform, but these are not
> "real" errors.)

I really don't want to go down that path. There is no similarity algorithm or tolerance parameter setting that will satisfy everyone. We'd end up tweaking things with impossible-to-predict consequences for false negatives/false positives.

Instead, we've generally been quite successful at finding ways to make tests match exactly. One tool in our arsenal is SVG filters, which allow us to apply pixel-processing effets. In particular, layout/reftests/filters.svg contains two filters you can use to map all non-black pixels of some element to white, or map all non-white pixels to black.

The fact that rasterizers draw differently depending on how the glyphs are ordered, or on how the glyph buffers are split between calls, is unfortunate and it's really the only problem I'm aware of where we don't currently have a perfect solution for exact reftesting. We're currently using those filters in some of the RTL tests to work around the problem, so I suggest you try using them here too.
This patch combines the fixes for bug 454098 and bug 475092 into a single patch file for the cairo directory; this should be suitable for pushing upstream. The actual fixes to our tree copy of Cairo are not part of this, they are still in the separate patches on each bug.
It would be a little better if there was one landable patch for this bug and and other separately landable patch for bug 475092.
I've updated the patch to include the gfx/cairo patch file as well as the actual cairo backend fix and thebes fix. I will include the reftest with bug 475092 instead of here, as it turns out that it doesn't really test this bug at all, it tests the separate code path for drawing to a canvas, which had its own separate bug!
Attachment #358457 - Attachment is obsolete: true
Attachment #358477 - Attachment is obsolete: true
Attachment #358589 - Attachment is obsolete: true
Attachment #358858 - Flags: review?(roc)
Pushed http://hg.mozilla.org/mozilla-central/rev/a6690fa2baba
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Jeff, I'm assuming from Vlad's comment on (similar but separate) bug 475092 that you can take care of upstreaming the patch for Cairo here (see gfx/cairo/win32-vertically-offset-glyph.patch)? If not, and I need to do something else about it, please let me know.
You need to log in before you can comment on or make changes to this bug.