430465 - After scrolling down then up, some PNG tiled background images exhibit odd artefacts

Status: RESOLVED INCOMPLETE

(Core Graveyard :: GFX, defect)

Description

[David Bailey]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

Odd artefacts appear in background PNG images after scrolling

Reproducible: Always

Steps to Reproduce:
1. Load long web page with a PNG background image file that is tiled
2. Scroll down then back up
3. Odd artefacts appear in image (see uploaded example)
Actual Results:  
See uploaded sample file

Expected Results:  
No artefacts should appear

Comment 1

[David Bailey]

Attachment: Screen shot of artifacts which appear after scrolling

Comment 2

[David Bailey]

Running on Windows XP SP2, fully patched.  Pentium D processor 3GHz with 2GB RAM.  NVIDIA Quadro FX560 graphics card.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042223 Minefield/3.0pre

Regression window is
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1205098020&maxdate=1205103479
Bug 403181 seems the most likely cause.

Comment 4

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

I have to zoom in to see this. David, do you see this without zooming?

Comment 5

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

Wait, sometimes I can reproduce it without zooming.

Comment 6

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

The key to reproducing this is to scroll *slowly*. It affects the grey bar on the right as well as the rightmost blue bar.

Comment 7

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

Attachment: testcase

Comment 8

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

I'm pretty sure that when we scroll slowly, the dirty rect contains only one tile so we take the single-tile path through nsLayoutUtils::DrawImage. That appears to be scaling the image, which it probably shouldn't be since the image coordinates should have been snapped to device pixels in a way that leaves the image size intact.

Comment 9

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

Attachment: simplified testcase

The edges of the image are being antialiased in this case, but they really shouldn't.

Comment 10

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

Attachment: fix the testcase

This patch fixes the testcase.

The problem in the testcase, which I've seen elsewhere too, is that we're snapping the destination and dirty rects which makes their width slightly different to the source rect, so we end up scaling the image by some ratio very close to 1. For example if the source and destination widths start off as 99.5px then we might snap the destination width to 100px and scale the image up a tiny bit. This makes it look blurry and is slow too.

So my idea is to treat the source rect as defining two things:
1) which pixels we may sample
2) a transformation (translation+scale) between source coordinates and coordinates within aDestRect

1) becomes pxSubimage which we round out and pass down to gfx.
2) is used to reconstruct the source rectangle by applying the reverse transformation to aDirtyRect. In particular (unless source clipping is going on) scale factors are preserved through nsLayoutUtils::DrawImage; if the source width equals the destination width on entry, then the source width will equal the dirty rect width when we call into gfx.

HOWEVER, this does not fix this bug as reported.

Comment 11

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

The Bath site uses background-position:center. We make no attempt to snap *source* coordinates at all, so our source rect is not pixel aligned and we sample fuzzy edges. That's OK. The problem is that for consistency we should also make the *tile* path sample the same way.

Comment 12

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

Attachment: fix

This fixes the reported bug. I could never figure out what the rounding there was for, anyway.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 317460 [details] [diff] [review]
fix

Looks fine; I can't quite remember what the rounding was for any more either.

Comment 14

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

Comment on attachment 317460 [details] [diff] [review]
fix

Fixes some visual artifacts scrolling certain pages. Fairly safe.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317460 [details] [diff] [review]
fix

a1.9=beltzner

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I backed out the patch+bustage fix because of orange in tbox.
Not sure if the cause was the reftest or the "bustage fix" to reftest.list or
what.

Comment 17

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

Sorry Olli, I was delinquent.

Bug 421069 failed on all three platforms, and the reftest in this bug failed on Mac. Which is strange since I manually checked the color values on Mac and they seemed right. I think we should just leave this unfixed for 1.9, it's a minor bug and not worth spending more energy/risk on.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 317460 [details] [diff] [review]
fix

Remove approval, since we don't want to land this.

Comment 19

[Dão Gottwald [:dao]]

Is this ready to land again?

Comment 20

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

Needs retesting.