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

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 1

[Simon Montagu :smontagu]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: image showing where the problem is

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.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: ugly patch

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Ah, right.  This uses a less ugly hack:  an svg:use to force the particular resource document to load.

Comment 8

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

(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?

Comment 9

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

(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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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).

Comment 11

[Ray Kiddy]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed in mozilla-central (and thus also on mozilla-1.9.1):
http://hg.mozilla.org/mozilla-central/rev/57a177c0d8fe

Comment 13

[Karl Tomlinson (:karlt)]

zwnj-01-notref.html now passes with Pango:
http://hg.mozilla.org/mozilla-central/rev/946b38048b4b