Closed Bug 465140 Opened 11 years ago Closed 11 years ago

layout/reftests/text/zwnj-02.html fails on some machines

Categories

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

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(2 files, 1 obsolete file)

The reftest layout/reftests/text/zwnj-02.html fails on my Linux machine:  one of the antialiased pixels around the edge of the sets of characters varies slightly in shade between test and reference.  (This has been bugging me for quite a while; I haven't gotten around to investigating fully until now.)  (I think it failed on my Fedora box as well; I'm currently running Ubuntu, and I know it fails on Ubuntu 8.04 and 8.10.)

I think the test isn't technically valid:  since we paint the text twice for the test but only once for the reference (it's comparing that text joins correctly across spans), if there is a pixel that is partly opaque for both pieces of text but not fully opaque for either, it will end up shaded differently between test and reference.

I propose replacing the test with:
 * one pair (test and reference), equivalent to the current pair, except using SVG filters to turn all non-white pixels to black
 * one pair (test and reference), equivalent to the current pair, except using SVG filters to turn all non-black pixels to white
 * a test to check that the SVG filter constructs used don't change black or white (e.g., on pixel-aligned backgrounds and borders), with a test using SVG filters and a reference not using them

Does that seem reasonable?
The solution makes sense, but I don't fully understand the analysis of the problem: why does dividing text into spans mean that we paint it twice?
It doesn't mean we paint any character twice.  It means we paint the parts in the two spans separately.  The problem is that with antialiasing, those two separate parts can overlap, and any place where (a) they overlap and (b) neither is fully opaque can end up with a slightly different color.
This is an image that hopefully shows where the problem is.  I took zwnj-02.html, and made the first span red and the second span green.  This is a screenshot, scaled 20x, of the join between the spans in the first line of the test.  Notice that where the red and the green meet, there are four pixels that match neither the ones to their right nor the ones to their left.  The reftest failure for the first line is on the uppermost of those four pixels, where we paint both red and green, but neither fully opaque, since that case differs when we're painting the left and right parts separately or all the text at once.  (There are similar failures for most of the other lines, only one pixel per line.)
cc:ing roc as well in case he thinks the code that paints different parts of a text run separately ought to handle this case differently.
Attached patch ugly patch (obsolete) — Splinter Review
This is ugly because I needed to add a setTimeout (which probably won't really work on tinderbox); I suspect something is wrong with external resource documents (like the one I used for filters) blocking onload.  But if I don't do that, everything with the filters ends up blank in the test harness output.
External resource documents do block onload once they're loading, or at least they had better!  The problem here is that the external resource loads usually do not start until the relevant nsReferencedElement is actually asked for an element, which happens at paint time for filters.  If onload has already fired by this point, there's nothing to block.

Similar things happen for CSS background images if onload fires before we've resolved style on the node...

roc, do we perhaps want to more eagerly grab the referenced element?
Attached patch patchSplinter Review
Ah, right.  This uses a less ugly hack:  an svg:use to force the particular resource document to load.
Attachment #348438 - Attachment is obsolete: true
Attachment #348456 - Flags: review?(smontagu)
(In reply to comment #6)
> roc, do we perhaps want to more eagerly grab the referenced element?

That sounds like a good idea, but it might be tricky ... when would we grab it? Style resolution time?
(In reply to comment #4)
> cc:ing roc as well in case he thinks the code that paints different parts of a
> text run separately ought to handle this case differently.

Just to be clear, this problem doesn't seem to be related to ligatures or the use of clipping to split ligatures. In this case there are two glyphs which we paint separately.

So it appears that painting two glyphs in one call gives different results to painting two glyphs in separate calls. That actually makes sense, since in principle a single call that fills multiple glyph can/should compute exactly spatial coverage for each pixel before turning that into an alpha value. So a pixel where the left half is covered by one glyph and the right half is covered by another glyph should turn out solid, whereas for the separate calls case we'll get 75% opacity.

It would be nice to fix that by fusing glyph drawing calls when possible, but it's hard. One problem is that we currently *break up* calls that draw a large number of glyphs, to avoid having to heap-allocate large glyph buffers. Another problem is that we'd have to fuse drawing across frame boundaries. That means display list hacking to combine display list items (or something like that). Another tricky bit is that nsDisplayText draws text decorations and selection backgrounds as well as the actual text.

One direction we could try is to detect adjacent nsDisplayText items in the display list that "should" be merged and wrap them in a PushGroup and use OPERATOR_ADD to draw the items into the group. That could hurt performance pretty badly, I suspect (depending on how we define 'should'). I'm not convinced it's worth doing.
Grabbing during style resolution would make sense to me, except that as currently set up we need a node to call RequestExternalResource.  That node is used for content policy, etc, checks.  I suppose we could replace that node with the node's document (or just always pass in the document when calling from style resolution).
Attachment #348456 - Flags: review?(smontagu) → review+
FYI, I see this failure when running reftests on a freshly built, freshly checked out (2008/11/20 15:14:26 PST) Firefox on Mac OS X 10.4.11 (2.16 GHz Intel) also. In case this adds something, it is not just visible on a Linux box.
Fixed in mozilla-central (and thus also on mozilla-1.9.1):
http://hg.mozilla.org/mozilla-central/rev/57a177c0d8fe
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Depends on: 466376
You need to log in before you can comment on or make changes to this bug.