Closed Bug 479156 Opened 11 years ago Closed 11 years ago

Seams between tiles of a border-image

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: zwol, Assigned: zwol)

References

Details

(Keywords: verified1.9.1)

Attachments

(4 files, 1 obsolete file)

Attached file 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.
(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.
OS: Linux → All
Hardware: x86 → All
So for once it's *not* the Linux EXTEND_PAD headache.  Good to know.
I think it would be good to track this down for 1.9.1.
Flags: wanted1.9.1?
I think we should block on this since it is very apparent in the error console and add-ons manager on OSX
Flags: blocking1.9.1?
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.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
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.
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.
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.
Attachment #363716 - Attachment is obsolete: true
Attachment #363817 - Flags: superreview?(roc)
Attachment #363817 - Flags: review?(roc)
Attachment #363817 - Flags: approval1.9.1?
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.
Attachment #363817 - Flags: superreview?(roc)
Attachment #363817 - Flags: superreview+
Attachment #363817 - Flags: review?(roc)
Attachment #363817 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/8c469e7d1257
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
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?
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P2
(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.
Attachment #363978 - Flags: review?(roc)
Attachment #363978 - Flags: approval1.9.1?
Reftests went through fine on Windows too.
Whiteboard: [needs 191 approval] → [needs 191 approval][needs landing]
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?
Target Milestone: --- → mozilla1.9.2a1
Oh, I meant by using both testcases which are attached to this bug.
> 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.)
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
Status: RESOLVED → VERIFIED
Attachment #363817 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 363817 [details] [diff] [review]
(rev 2) - debugging printfs, + testcases, + comments

a1.9.1=dbaron
Attachment #363978 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 363978 [details] [diff] [review]
reftest.list additions for patch v2

a1.9.1=dbaron
Whiteboard: [needs 191 approval][needs landing] → [needs 191 landing]
The reftests patch needs to be landed before we take this on 1.9.1
Whiteboard: [needs 191 landing] → [needs 191 landing][needs landing]
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
Oops, I see it's the second patch that needs checking in, it applies fine...
Pushed: http://hg.mozilla.org/mozilla-central/rev/b0016bfc6c33
Whiteboard: [needs 191 landing][needs landing] → [baking for 1.9.1]
pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f1ac510e9267

I forgot to set the patch author, sorry Zack!
Keywords: fixed1.9.1
Whiteboard: [baking for 1.9.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
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.