Jeff Muizelaar [:jrmuizel] Reporter
	Description • 15 years ago

This is presumably because we rescale the background image on every paint. We shouldn't do this, and I'm not yet sure why we do.

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 1 • 15 years ago

This is because we call createOffscreenSurface() every time we draw the image and so we don't hit the destination scaled image cache.

createOffscreenSurface() is called because the sourceRect is much larger then we actually need and so we think that the image needs tiling. Even still we shouldn't call createOffscreenSurface() for tiled images.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 2 • 15 years ago

What testcase are you looking at?

nsThebesImage::Draw could cache the temporary surface it creates for tiled images. That's probably the best thing to do, short of actually being able to limit the area we sample from in tiled image space.

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 3 • 15 years ago

This page simulates the effect using 'background-size':
http://people.mozilla.com/~jmuizelaar/scroll/fixed-downscale.html

I don't actually understand why we need a temporary surface at all.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 15 years ago

I'm surprised, because we've discussed it several times.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 15 years ago

But to recap: suppose we have a 20x20 image whose left half is black and whose right half is white. Suppose we tile that image to cover a 100x100 area. Suppose there is a non-1.0 zoom, which triggers bilinear sampling. cairo with EXTEND_REPEAT will draw the rightmost column of pixels by sampling from the rightmost row of pixels of the image *and* the leftmost row of pixels of the image, producing a gray line. This is an artifact because if there's no zoom, the area around and containing that line is solid white.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 15 years ago

(assuming a white background color for the page)

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 7 • 15 years ago

(In reply to comment #5)
> But to recap: suppose we have a 20x20 image whose left half is black and whose
> right half is white. Suppose we tile that image to cover a 100x100 area.
> Suppose there is a non-1.0 zoom, which triggers bilinear sampling. cairo with
> EXTEND_REPEAT will draw the rightmost column of pixels by sampling from the
> rightmost row of pixels of the image *and* the leftmost row of pixels of the
> image, producing a gray line. This is an artifact because if there's no zoom,
> the area around and containing that line is solid white.

I understand that scenario. However, I was expecting a page with a fixed background to be indistinguishable from one with a scrolling background provided the content size was less than window size. This currently isn't the case, and in thinking it over I'm not sure which makes more sense.

I also didn't realize some of the implications of our current design. For example,
http://people.mozilla.com/~jmuizelaar/scale/test.html is quite slow. No good way to make it fast, well keeping the same rendering, using CoreGraphics comes immediately to mind.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 15 years ago

(In reply to comment #7)
> I understand that scenario. However, I was expecting a page with a fixed
> background to be indistinguishable from one with a scrolling background
> provided the content size was less than window size. This currently isn't the
> case, and in thinking it over I'm not sure which makes more sense.

I see your point. What's happening here is simply that background-attachment:fixed doesn't change the "fill area" over which the background is drawn. In this case, that is the whole document, i.e., logically the background is tiled over the whole document. In practice that doesn't really matter since you can only ever see the one tile. In fact it does matter a tiny bit: for example it implies that the pixels at the bottom of the window can/should be sampled using the first row of the next tile as well as the last row of the first tile.

However, in this case I think it would be quite justified to forget about that and limit the fill area for fixed backgrounds to the viewport, always. I'll write a patch for that.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 15 years ago

That will mean that drawWindow calls or other operations that try to render content outside the viewport won't get the background. Too bad!

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 15 years ago

This causes all background-attachment:fixed backgrounds to be clipped to the viewport. Probably the right thing to do even though it's technically incorrect.

	David Baron :dbaron:
	Comment 11 • 15 years ago

Are you changing the clip area just to avoid hitting a codepath that will trigger the extra surface?  Could we pass a boolean to do that instead, and not break drawWindow?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 15 years ago

We could condition this change on FLAG_DESTINED_FOR_SCREEN, I guess.

We can't just "disable the extra surface", since that would lead to some kind of incorrect results when using drawWindow, e.g. we'd trigger artifacts such as those described in comment #5 in some situations.

	David Baron :dbaron:
	Comment 13 • 15 years ago

(In reply to comment #12)
> We could condition this change on FLAG_DESTINED_FOR_SCREEN, I guess.

If that's not too hard, it sounds like a good idea.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 15 years ago

It's easy.

	David Baron :dbaron:
	Comment 15 • 15 years ago

Comment on attachment 416525 [details] [diff] [review]
fix v2

r=dbaron

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/3ec4205e16df