Steps to reproduce: 1. Log into the new version of Gmail. (Not yet available to everyone.) 2. Cmd+- (zoom out). Result: magenta and cyan lines under the stars in the message list and elsewhere. This happens because of Gmail's use of the background-position hack to put multiple images in a single file (similar to the -moz-image-region hack). I'll attach a testcase in a minute.

Also happens on Vista.

These are rounding issues -- or maybe lack of rounding; we're pulling in stuff from adjacent parts of the image. I can figure out which one is happening.

It's not really rounding. What's happening is that when images are scaled up or down, when we go to draw the subimage, we end up applying a sampling filter to the source image to get the transform right. These images have the pink background tight against the image itself -- so when scaled up/down, we'll end up sampling the pixels there, giving the pink fringe. There isn't anything we can do about this; an ideal solution would be an evangelism one; web authors should continue to use these kinds of tiled images, but should leave a few pixels of padding around their image in the same color as the background on top of which the image itself is painted. (If there is no background, transparent would work fine.) Any ways for us to solve this would be a significant slowdown; we'd basically have to copy out the image segment each time we wanted to render it.

Can't we tell the sampling filter to not go outside the boundaries? I don't think evangelism will work well for this kind of thing, since it only shows up when users zoom, and since the site arguably isn't doing anything wrong.

This is a common technique for Web authors; I think it's even used on mozilla.com. We need to handle it correctly.

We do handle it correctly, if we don't scale. If we scale, we can handle it -slowly- and correctly. If that's what we have to do, then I guess that's what we have to do. I would much rather use the opportunity to teach web authors how to do what they're doing correctly (like I said, a single pixel padding around each image in the group would solve the problem), but I'm not sure if that kind of evangelism is really possible.

I don't agree that Web authors should have to learn about this. In principle we could implement it efficiently with source clipping or whatever. And if we use hardware acceleration effectively the temporary copy will be low overhead. This is our problem.

But we don't have source clipping, so that doesn't help us, and the temporary copy may be low overhead, but it'll still be an extra copy; it really depends how often those images get drawn. gmail for example draws tons of those per page. The underlying problem is that we have no way of distinguishing a sub-tile drawing from a drawing of just a portion of an image due to an invalidation. We handle both those identically now. However, to solve this, we'd need to handle them differently. Right now the only way to do subtile rendering is by using a background image and background offset; so perhaps we just need to pass in yet another rectangle to the image drawing methods, which is the rectangle of the full image -- right now we have source rect and dest rect, we'd also need to pass in the "full source rect".

Yeah. I realize this is hard for us.

I'm going to punt this from the blocker list. I'd like to fix it, but I think that the changes needed are potentially too fragile this late in the game (since they touch a cross-section of gfx and widget).

This is a pretty bad regression. Why can't we do the last paragraph of comment #10?

It shouldn't involve widget, just gfx and layout. Or maybe even just nsCSSRendering.

Er sorry, I meant gfx and layout. It's not a regression though -- we didn't have full page zoom before. Doing my suggestion would mean we'd have to both pass that rectangle down, and then create a temporary surface and copy in the portion of the image in nsThebesImage. How hard is it to get that rectangle (the "what's the maximum portion of the image that would be rendered" rectangle)?

Not that hard. Yes, we didn't support full zoom before, but now that we do, it should display zoomed pages as well as unzoomed ones. Users and Web developers aren't easily going to make the connection between zooming and these image artifacts, and once they do Web developers will feel obliged to code around them --- oops, we just made Web development harder. I think I should work on this.

Ok, fair enough; putting back b1.9+ based on "Not that hard" :)

Taking

I think I'm going to need dbaron review on this as well, but let me know if you don't agree.

Comment on attachment 305707 [details] [diff] [review] fix ><HTML><HEAD><LINK rel="icon" href="https://people.mozilla.com/~vladimir/favicons/bugzilla.png" type="image/png"/><LINK rel="shortcut icon" href="https://people.mozilla.com/~vladimir/favicons/bugzilla.png" type="image/png"/></HEAD><BODY><PRE>diff -NrpU12 mozilla-trunk.b8d2f0e0a468/gfx/public/nsIImage.h mozilla-trunk/gfx/public/nsIImage.h >--- mozilla-trunk.b8d2f0e0a468/gfx/public/nsIImage.h 2008-02-26 21:58:09.000000000 +1300 >+++ mozilla-trunk/gfx/public/nsIImage.h 2008-02-26 21:58:09.000000000 +1300 >@@ -180,28 +180,32 @@ public: > virtual nsresult Optimize(nsIDeviceContext* aContext) = 0; > > /** > * Get the colormap for the nsIImage > * @update - dwc 2/1/99 > * @return if non null, the colormap for the pixelmap,otherwise the image is not color mapped > */ > virtual nsColorMap * GetColorMap() = 0; > > /** > * BitBlit the nsIImage to a device, the source and dest can be scaled > * @param aSourceRect source rectangle, in image pixels >+ * @param aSubimageRect the subimage that we're extracting the contents from. >+ * It must contain aSourceRect. Pixels outside this rectangle must not >+ * be sampled. > * @param aDestRect destination rectangle, in device pixels > */ > NS_IMETHOD Draw(nsIRenderingContext &aContext, > const gfxRect &aSourceRect, >+ const gfxRect &aSubimageRect, > const gfxRect &aDestRect) = 0; > Do we have to always specify the subimagerect? Seems like it would simplify the code if you were to pass a pointer in, and have NULL mean "the whole image" (instead of needing to compare to 0,0,w,h). I would even have NULL be a default parameter here, but I don't think you're a fan of default params :) >diff -NrpU12 mozilla-trunk.b8d2f0e0a468/gfx/src/thebes/nsThebesImage.cpp mozilla-trunk/gfx/src/thebes/nsThebesImage.cpp >@@ -443,99 +444,124 @@ nsThebesImage::Draw(nsIRenderingContext > // Reject over-wide or over-tall images. > if (!AllowedImageSize(destRect.size.width + 1, destRect.size.height + 1)) > return NS_ERROR_FAILURE; > > nsRefPtr<gfxPattern> pat; >+ if ((xscale == 1.0 && yscale == 1.0 && >+ !ctx->CurrentMatrix().HasNonTranslation()) || >+ subimageRect == gfxRect(0, 0, mWidth, mHeight)) { nit: { on a new line after a multi-line if clause :) >+ // No need to worry about sampling outside the subimage rectangle, >+ // so no need for a temporary >+ // XXX should we also check for situations where the source rect >+ // is well inside the subimage so we can't sample outside? >+ pat = new gfxPattern(ThebesSurface()); >+ } else { >+ gfxIntSize size(NSToIntCeil(subimageRect.Width()), >+ NSToIntCeil(subimageRect.Height())); >+ nsRefPtr<gfxASurface> temp = >+ gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, mFormat); >+ if (!temp || temp->CairoStatus() != 0) >+ return NS_ERROR_FAILURE; >+ >+ gfxContext tempctx(temp); >+ tempctx.SetSource(ThebesSurface(), -subimageRect.pos); >+ tempctx.SetOperator(gfxContext::OPERATOR_SOURCE); >+ // Use Fill instead of Paint because we want to sample exactly >+ // the subimageRect here. >+ tempctx.Rectangle(gfxRect(gfxPoint(0, 0), subimageRect.size)); >+ tempctx.Fill(); I guess Fill() instead of Paint() so that you can support non-integer sizes? Ok, I buy that. :) The rest looks fine here. I'm fine either way on the pointer-vs-ref issue; I guess Draw isn't called in enough places to make it any easier to use default-NULL, really.

(In reply to comment #22) > Do we have to always specify the subimagerect? Seems like it would simplify > the code if you were to pass a pointer in, and have NULL mean "the whole > image" (instead of needing to compare to 0,0,w,h). I would even have NULL be > a default parameter here, but I don't think you're a fan of default params :) There are only two callers, one from nsLayoutUtils and one from nsBaseDragService. So it doesn't seem worth doing anything here. > >+ gfxContext tempctx(temp); > >+ tempctx.SetSource(ThebesSurface(), -subimageRect.pos); > >+ tempctx.SetOperator(gfxContext::OPERATOR_SOURCE); > >+ // Use Fill instead of Paint because we want to sample exactly > >+ // the subimageRect here. > >+ tempctx.Rectangle(gfxRect(gfxPoint(0, 0), subimageRect.size)); > >+ tempctx.Fill(); > > I guess Fill() instead of Paint() so that you can support non-integer sizes? > Ok, I buy that. :) Yeah, that's what my comment was trying to say, I'll fix that up.

Comment on attachment 305707 [details] [diff] [review] fix This seems like something that would be good to put in beta4 if we can.

Comment on attachment 305707 [details] [diff] [review] fix sr=dbaron

Comment on attachment 305707 [details] [diff] [review] fix Rendering bug with zooming that we should land for beta4 since it impacts site rendering in a significant way.

Comment on attachment 305707 [details] [diff] [review] fix a1.9b4=beltzner

checked in

I backed this out, it caused orange on various platforms: Mac and Windows: REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/bugs/368020-2.html Windows and Linux: REFTEST UNEXPECTED FAIL: file:///C:/slave/trunk_2k3/mozilla/layout/reftests/bugs/403181-1.xml REFTEST UNEXPECTED FAIL: file:///C:/slave/trunk_2k3/mozilla/layout/reftests/bugs/412093-1.html I wonder why this test didn't pass on Windows or Linux...

368020-2.html fails just because we have an element with fractional width W and a background image larger than the element's background area. nsLayoutUtils::DrawImage snaps the destination rectangle to pixels (width W') but does not adjust the source rectangle or the subimage rectangle, which still have fractional width W. Then, since we're now scaling (not by much, just W'/W) and we're not drawing the whole image, with this patch nsThebesImage::Draw makes a temporary surface and copies to it. Because the subimage rectangle is still fractional width W', the last column of pixels is blended with the current contents of the temporary surface (black). Then when we paint from the temporary surface to the destination rectangle, those last pixels contribute to the painting. I'm not really sure what we want to do here. The key question is, what's the desired rendering of nsLayoutUtils::DrawImage when aSourceRect isn't pixel-aligned?

Does this need a relnote? Or is it not frequent enough a happening ...

Don't think that it needs a relnote, it only really happens under zoom and only on certain image uses. Zooming gmail is probably the most noticable.

I think the best approach here is to expand the subimage rect to nearest pixel boundaries in nsThebesImage::Draw. That will avoid this bug, it is closest to the before-patch behaviour, and it is logical given the definition of the subimage rectangle as defining the set of image pixels which may be sampled.

Yep, that approach sounds fine to me.

Okay, here's the approach which extends the subimage rect out to pixel boundaries before we use it. That fixes 368020 and 412093 tests. Linux and Windows failed this bug's test because during scaling they sample black outside the edges of the source pattern. We need to extend the EXTEND_PAD (for Windows) and SetFilter(0) (for Linux) path to handle not just xscale/yscale scaling but also a scale in 'ctx'. This patch does that too. Only nsThebesImage::Draw has changed in this patch.

Comment on attachment 307098 [details] [diff] [review] fix v2 Looks fine, two small style nits: >+ // Expand the subimageRect to place its edges on integer coordinates. >+ // Basically, if we're allowed to sample part of a pixel we can >+ // sample the whole pixel. >+ subimageRect.RoundOut(); >+ > nsRefPtr<gfxPattern> pat; >+ if ((xscale == 1.0 && yscale == 1.0 && >+ !ctx->CurrentMatrix().HasNonTranslation()) || >+ subimageRect == gfxRect(0, 0, mWidth, mHeight)) { ^ nit: { on new line on multi-line condition clauses (otherwise things run together oddly on the left side and it's hard to see where the condition ends and where the block begins). [...] > pat->SetMatrix(mat); > >- if (xscale != 1.0 || yscale != 1.0) { >+ if (xscale != 1.0 || yscale != 1.0 || >+ ctx->CurrentMatrix().HasNonTranslation()) { ^ same nit as above

relanded

I had to back it out again. The same tests failed, but this time the differences are not visually perceptible :-(. I guess I should have run the automated tests just in case.

For the record, here's what I checked in, it needed a little merging.

Some of the problems I'm having seem to be due to a recent regression in pixman, which I was able to reproduce with a small cairo-only testcase: http://lists.cairographics.org/archives/cairo/2008-March/013294.html

This is actually fixed in pixman -- I didn't realize this is what you were running into. I pushed it upstream into pixman yesterday, I'll push it into our pixman code today.

Hm, or maybe not, I was thinking of another issue (with EXTEND_PAD).

It's not, unless you fixed it in the last 8 hours. I reproduced it with git-head pixman.

Now I'm fighting a regression that doesn't appear with the image-surface backend, but does appear with the Win32 backend. Argh.

Oh, now it appears in the image-surface backend again. I'm scared.

I reduced the failure on 368020-2.html on Windows down to this testcase (yay live reftest-analyzer!!) <!DOCTYPE HTML> <html> <body> <span style="background: url(repeatable-diagonal-gradient.png); -moz-background-clip: padding; background-clip: padding; border: medium solid; border-width: 0 0 0 5px;"> blah<br> </span> </body> </html> Removing the background-clip changes the R value of the bottom right image pixel by 1/255. Which is strange since this element doesn't have any padding.

Oh wait, I'm dumb, the effect of that is to clip to the padding-box, so there is an effect because we have a border.

OK, so the difference is that we're taking the DrawTile path for the *reference* testcase. So there are two bugs here: DrawTile isn't consistent with Draw, and that we're actually taking the DrawTile path for this testcase at all. :-(

Ah, the inconsistency starts with layout, where nsCSSRendering::PaintBackgroundWithSC decides to take the tile path or not based on whether the rect to draw fits into one tile ... and then rounds the destination rect to pixels on the non-tile path, but no rounding happens on the tile path. That seems wrong, but we've done it for a long time so I won't try to fix that here. But I do need to try to fix the problem that DrawTile is being invoked unnecessarily in the reference testcase.

OK, so DrawTile is being invoked because the default background-origin is "padding", so we are actually drawing two tiles, although one of them is entirely covered by the left border. I think I should just disable the 368020-2 test. So far it's only worked by luck (and hasn't worked at all on gtk2, which is why it's disabled for gtk2).

The 412093-1 test is regressing because <br>s are one appunit wide, which makes some spans one appunit wider than you'd expect, and this is changing the rasterization of background images in those spans in very subtle ways.

Okay! So this is the previous patch with the following changes: -- local backout of the pixman patch that caused the regression I mentioned. -- nsThebesImage::Draw now does its SetFilter/EXTEND_PAD thing based on the surface type, not #ifdefs. -- Put the test 412093-1 into quirks mode, so that <br> has zero width and the test won't regress. -- Mark the test for 368020-2 random, since it depends on the rasterization of DrawImage and DrawTile being consistent as described in bug 421438, and they haven't been for a while. Reftests finally pass on Windows. Haven't tested Mac or Linux yet but I'll do both.

Comment on attachment 307869 [details] [diff] [review] fix v3 Changes look good, can you add something to gfx/cairo/README about the backout of the pixman patch, and maybe put the backout in a .patch file there? Just so I don't accidentally blow this away.

Yeah, will do.

Checked in. Hoping it sticks this time...

Backed out again :-(. This time, failed in JPEG encoding tests :-(. ../../../_tests/xpcshell-simple/test_libpr0n/unit/test_imgtools.js: FAIL FAILED in test #2 -- test encoding a scaled JPEG: arrays differ at index 627 On the bright side, reftests passed on all platforms.

It looks like backing out the pixman change requires us to regenerate some test images --- those images were regenerated when the pixman change was landed. The image scaling tests would be affected by the bug, in fact. Vlad, is there an easy way to regenerate those? You must have done it a few times now :-)

Ug, not really. I've always done it manually, and it's extremely painful since the test has a way to generate images, but only one image at a time. Are the same test image problems present with Soren's patch (with the typo that you caught)?

Soren's patch gives the same behaviour for bilinear filters --- which is what the libpr0n tests use --- as my backout. The difference is that his patch re-adds a fix for nearest-neighbour sampling. So, as you'd expect, simply reverting the updates you made to the test images when you landed the cairo branch containing the pixman regression makes tests pass again. I'm going to reland again with that.

Sounds good to me.

Relanded. Fourth time lucky?

FINALLY!