559426 - Rev3 Fedora permaorange: borders in layout/reftests/box-shadow/boxshadow-inner-basic.html

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Phil Ringnalda (:philor)]

Attachment: reftest log

RelEng is switching from running reftests on the old CentOS refplatform to the
Talos Rev3 Fedora 12 refplatform. Two of the consistent failures blocking the switch, which I'm guessing are a single thing, are layout/reftests/box-shadow/boxshadow-inner-basic.html and reftests/bugs/456219-1c.html, both of which are differing by a single pixel at the start/end of some curved borders.

Comment 1

[Armen [:armenzg]]

Hi all,
Could one of you (bzbarsky, dbaron, roc or zweinberg) have a look at the log and see if this test could be fixed?

If I got you access to a Fedora machine would it help to find out?

Comment 2

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

Zack, could you take a look?

Comment 3

[Zack Weinberg (:zwol)]

This looks like yet another curved seam aliasing problem (i.e. bug 466572).  I think I can fix boxshadow-inner-basic.html by rewriting the reference as an SVG file that performs exactly the same paint operation that the test does.  For 456219-1c.html, probably the most expedient thing is to add more dots to the mask images that are already in place; creating mask images is also a fallback option for boxshadow-inner-basic if I can't manage an exact match with SVG.

Any objections to this plan?

Comment 4

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

Sounds good to me

Comment 5

[Zack Weinberg (:zwol)]

Attachment: partial patch

Here's the beginnings of a patch.  It *should* fix 456219c - I masked the pixels that the log says need masking, but I wasn't getting the failures on my machine anyway.  I *do* see the box-shadow failure, and there's changes to that test in here too, but after an hour of failing to get an exact match from SVG for the paint operations being done in PaintBoxShadowInner, I give up.  Maybe someone else can do something with this.  Perhaps it will be mventnor, since he seems to have written PaintBoxShadowInner.

Comment 6

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

Comment on attachment 442272 [details] [diff] [review]
partial patch

It's not clear to me what you want feedback on.  (Feel free to ask for feedback again, though.)

Comment 7

[Zack Weinberg (:zwol)]

Comment on attachment 442272 [details] [diff] [review]
partial patch

I see that "feedback" is not the same thing as "cry for help" now :)

Comment 8

[Zack Weinberg (:zwol)]

Attachment: patch for just the 456219c failures

This is the piece of attachment 442272 [details] [diff] [review] that should fix the 456219c failure; it can go in now, I think.

Comment 9

[Zack Weinberg (:zwol)]

Pushed attachment 443398 [details] [diff] [review] as http://hg.mozilla.org/mozilla-central/rev/c9eb3b4539bb

Comment 10

[Armen [:armenzg]]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1273164808.1273166070.19050.gz&fulltext=1
Rev3 Fedora 12 mozilla-central opt test reftest on 2010/05/06 09:53:28  
s: talos-r3-fed-026

Have you given a try to borrow one of the Fedora Rev3 machines?

Comment 11

[Zack Weinberg (:zwol)]

(In reply to comment #10)
> 
> Have you given a try to borrow one of the Fedora Rev3 machines?

That's not the problem - I can reproduce the failure on my own hardware just fine.  The problem is that I have not been able to get an SVG file to match the drawing operations done by -moz-box-shadow.  I think mventnor has a better chance of being able to do it, since he wrote the -moz-box-shadow code in the first place.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: second patch for 456219 failures

Based on analysis of the log in comment 10, it looks like we need still more pixels masked.

Comment 13

[Zack Weinberg (:zwol)]

Hm, there seems to be another patch for the 456219 failures over in bug 564274.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: SVG reference for boxshadow-inner-basic.html - works now

I managed to nail down an SVG reference that actually does draw everything just as the HTML does, with a little help from cairo-trace.  See long cranky comment in the new file for gory details.

Comment 15

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

Comment on attachment 444524 [details] [diff] [review]
SVG reference for boxshadow-inner-basic.html - works now

We probably should fix that bezier conversion for 'A', eh?

Comment 16

[Zack Weinberg (:zwol)]

Will land either later tonight (PDT) or tomorrow morning depending on how the tree behaves and what I decide to do after dinner.

> We probably should fix that bezier conversion for 'A', eh?

I thought we already had a bug for this but couldn't find it, so I filed bug 564960.

Comment 17

[Zack Weinberg (:zwol)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/49a4d88f4bc2
http://hg.mozilla.org/mozilla-central/rev/c1b073401621

but I'm going to leave this bug (and bug 5647274) open until I see the effect on the F12 reftest column.

Comment 18

[Zack Weinberg (:zwol)]

Um, I meant bug 564274.

Comment 19

[Zack Weinberg (:zwol)]

Per http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1273607328.1273608335.4170.gz the 456219-1 and boxshadow-inner-basic failures are resolved.  The remaining F12 reftest failures are:

REFTEST TEST-UNEXPECTED-FAIL | bugs/371043-1.html |
REFTEST TEST-UNEXPECTED-FAIL | bugs/393760-2.xml |
REFTEST TEST-UNEXPECTED-FAIL | bugs/449171-1.html |
REFTEST TEST-UNEXPECTED-FAIL | bugs/482592-1a.xhtml |
REFTEST TEST-UNEXPECTED-FAIL | bugs/482592-1b.xhtml |
REFTEST TEST-UNEXPECTED-FAIL | pagination/border-breaking-000-cols.xhtml |

Closing.