479156 - Seams between tiles of a border-image

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

Description

[Zack Weinberg (:zwol)]

Attachment: test case

At some zoom levels hairline seams can appear between the tiles of a border-image, where any underlying color is visible.  In the attached test case, the image is white throughout, so the only thing that appears in the page should be the words "There should be no black lines at any zoom level."  But, since there's a bug, at almost all zoom levels there *are* black lines - between border tiles, or at the edges of the image.

The seams vanish with slight changes to either the image itself or the styling.  The attached testcase reliably reproduces the effect for me on Linux but I wouldn't be surprised if it takes a different image or different styling for someone else.

This was discovered in the course of fixing bug 477732.  I've confirmed by variable inspection that nsCSSRendering::DrawBorderImageComponent is calling nsLayoutUtils::DrawImage with correct (pixel-aligned) arguments for all components of the image, so the problem is either in DrawImage or in gfx.

Comment 1

[philippe (part-time)]

(In reply to comment #0)
> Created an attachment (id=363013) [details]
> test case

On OS X 10.5.6, depending on the zoom level, I get vertical lines, horizontal lines or even squares.

Comment 2

[Zack Weinberg (:zwol)]

So for once it's *not* the Linux EXTEND_PAD headache.  Good to know.

Comment 3

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

I think it would be good to track this down for 1.9.1.

Comment 4

[Dave Townsend [:mossop]]

I think we should block on this since it is very apparent in the error console and add-ons manager on OSX

Comment 5

[Zack Weinberg (:zwol)]

Attachment: (rev 1) partial fix: correct scale factor used for "unscaled" tiles

Here's part of the fix - this should get rid of the seams in the middle of the OSX buttons, at least at most zoom levels.  This part turned out to be a bug in the border-image code, after all: under some circumstances a tile is "not scaled" per CSS, and I'd coded that as a scale factor of 1.0, but it needs to be a scale factor of CSSPixelsToAppUnits(1) instead (because no matter what the tile scaling is, that scale factor must also include a conversion from image/CSS pixels to app units).

I've left a bunch of debugging code in deliberately, because there's still a bug: you can see it in attachment 363013 [details] even with this patch applied, and I'm about to attach another test case that triggers it without use of border-image.  There is a pixel-snapping problem, I think somewhere in DrawImage, that causes the outer boundaries of scaled images to be not quite right.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: test case 2: seaming at image edges w/o border-image

This test case does not use border-image, but still demonstrates a pixel-snapping issue -- specifically, disagreement on exactly how pixels are to be snapped between the background-image code and the regular border code.  There are three CSS boxes, all exactly the same size, stacked on top of each other, styled like this:

 - z-index 0: background:black, no border.
 - z-index 1: background-image that's white in the middle,
              but has a 1px transparent boundary; no border.
 - z-index 2: no background, 1px solid white border.

At the default zoom level you won't see any black lines, but at most other zoom levels you will.

Comment 7

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

rev 1 looks good, but we should change the comments to make it clear that hFactor/vFactor/factor contain not just a scale factor but also the CSS-pixels-to-appunits conversion. It might make things clearer if they did not contain that conversion, actually.

test case 2 fails by design for at least two reasons.
1) border edges are rounded to device pixels (in the style system). So at various zoom levels the borders aren't (even in principle) the same size as pixels in an image.
2) bilinear sampling causes image pixels to "bleed" so that you can't just cover them up with a mask where you expect the pixels to be.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: (rev 2) - debugging printfs, + testcases, + comments

So if there's no expectation of no seams in the "test case 2" situation, then this patch is sufficient.  I added a whole bunch of test cases and a comment about the unit conversion, and I took out all the debugging printfs.

We naturally have app units for the width of the border, and CSS/image pixels for the width of the image tiles, and right now we just divide one by another to get the scale factor.  Taking the implicit unit conversion out of the ratios would mean sprinking explicit conversions through the DrawBorderImage loop and I think it would be harder to read overall.

Comment 9

[Zack Weinberg (:zwol)]

Note that 455364 has gone onto 1.9.1 now, so it would be good to get this through review+landing+trunk bake as quickly as possible and onto 1.9.1 as well.  I believe this is the last of the fallout from that patch.

Comment 10

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

Pushed http://hg.mozilla.org/mozilla-central/rev/8c469e7d1257

Comment 11

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

Oh, your patch didn't actually add to reftest.list. Can you do that and verify that the tests pass on whatever platforms you've got?

Comment 12

[Zack Weinberg (:zwol)]

Attachment: reftest.list additions for patch v2

(In reply to comment #11)
> Oh, your patch didn't actually add to reftest.list. Can you do that and verify
> that the tests pass on whatever platforms you've got?

Here's a patch that just adds the appropriate things to reftest.list.  Reftests pass on Linux.  I was going to run 'em on Windows too, but a Windows build has been running for the past SIX HOURS and is not done and I need to leave.  Maybe when I get back from campus.

Comment 13

[Zack Weinberg (:zwol)]

Reftests went through fine on Windows too.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mmh this patch was checked into trunk at 0:49am. Now I wanted to verify that it is fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090224 Minefield/3.2a1pre. But using Cmd++ at least twice still shows me black lines which end-up in a rectangle. Is that the intended behavior?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, I meant by using both testcases which are attached to this bug.

Comment 16

[Zack Weinberg (:zwol)]

> using Cmd++ at least twice still shows me black lines which
> end-up in a rectangle.

Both test cases have a construct which will, per comment #7, always show black lines at some zoom levels.  It's not because of the bug.  (The test cases checked in with the bug are different.)

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the info Zack. I checked with the given reftests (
layout/reftests/border-image/center-scaling-4*) on Windows and OS X and it looks fine. Marking this bug as verified.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre ID:20090225020500

Comment 18

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

Comment on attachment 363817 [details] [diff] [review]
(rev 2) - debugging printfs, + testcases, + comments

a1.9.1=dbaron

Comment 19

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

Comment on attachment 363978 [details] [diff] [review]
reftest.list additions for patch v2

a1.9.1=dbaron

Comment 20

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

The reftests patch needs to be landed before we take this on 1.9.1

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

First patch fails to apply now:

cpearce@mystery:~/work/src/mozilla-central$ hg import -m "Zack Weinberg - Bug 479156 - Seams between tiles of a border-image r+/sr+ roc" ~/work/bugs/landing-20090309/7_479156-border-image-seams.diff 
applying /home/cpearce/work/bugs/landing-20090309/7_479156-border-image-seams.diff
patching file layout/base/nsCSSRendering.cpp
Hunk #1 FAILED at 1930
1 out of 1 hunk FAILED -- saving rejects to file layout/base/nsCSSRendering.cpp.rej
file layout/reftests/border-image/center-scaling-4b-ref.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4b-ref.html.rej
file layout/reftests/border-image/center-scaling-4b.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4b.html.rej
file layout/reftests/border-image/center-scaling-4l-ref.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4l-ref.html.rej
file layout/reftests/border-image/center-scaling-4l.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4l.html.rej
file layout/reftests/border-image/center-scaling-4lr-ref.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4lr-ref.html.rej
file layout/reftests/border-image/center-scaling-4lr.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4lr.html.rej
file layout/reftests/border-image/center-scaling-4r-ref.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4r-ref.html.rej
file layout/reftests/border-image/center-scaling-4r.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4r.html.rej
file layout/reftests/border-image/center-scaling-4t-ref.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4t-ref.html.rej
file layout/reftests/border-image/center-scaling-4t.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4t.html.rej
file layout/reftests/border-image/center-scaling-4tb-ref.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4tb-ref.html.rej
file layout/reftests/border-image/center-scaling-4tb.html already exists
1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/center-scaling-4tb.html.rej
abort: patch failed to apply

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Oops, I see it's the second patch that needs checking in, it applies fine...

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/b0016bfc6c33

Comment 24

[Markus Stange [:mstange]]

pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f1ac510e9267

I forgot to set the patch author, sorry Zack!

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090312 Shiretoko/3.1b4pre ID:20090312034554