371434 - background images are sometimes scaled

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

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

Some of the reftests for background images (ones where only a single image is needed for the tile, so we use DrawImage rather than tiling) in layout/reftests/pixel-rounding/ are failing with symptoms of bug 371316.  I think this means that the images are being scaled (for some cases of non-pixel-alignment), which they should never be.

Comment 1

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

So in the case I'm looking at (the last test in background-image-height-5.html), things look fine near the end of nsCSSRendering::PaintBackgroundWithSC where it is about to make the calls into GFX:
  p2a=60
  sourceRect=900,870,600,630
  drawRect=10200,4200,600,630
which is correct.  So the problem seems to be in GFX.

Comment 2

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

Oh, I remember this problem.  This is the fun case where we have to transform the stuff that's in image coordinates based on where it's going to be placed (for cases where we're placing a non-round number of pixels of width/height of the image), at least for the cases where we're not scaling.  (So my patch yesterday in bug 371225 was partly wrong (for the source rect transformation.)

Comment 3

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

In other words, this is bug 326158 all over again.

Comment 4

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

Of course, now that we're doing scaling of images that involves averaging pixels, it's even harder to fix correctly.  To get that right, we really need to change the DrawImage API so it doesn't conflate scaling and clipping, since we want to scale the image fitting within a region snapped to pixels and then clip the result.

Comment 5

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

I think nsIRenderingContext::DrawImage needs to take three rects:
  const nsIntRect &aSourceRect, /* region of image, pixels */
  const nsRect &aDestRect, /* place where aSourceRect is drawn, coords */
  const nsRect &aClipRect, /* part of aDestRect that we are currently drawing */
and callers need to stop reducing aSourceRect and aDestRect based on clipping.  Then, pretty much the same thing for nsIImage::Draw, except they're all in pixels.

Comment 6

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

(And note that the only case I can think of in such a proposal where aSourceRect would not be the complete image dimensions would be when -moz-image-region is used to select a subregion of the image.  But there might be some other analogous feature I've forgotten about.)

Comment 7

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

Actually, -moz-image-region takes <length>s, so the region of the image actually has to be in app units as well.

Comment 8

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

Attachment: work in progress

Comment 9

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

Attachment: patch

Comment 10

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

The above patch seems to work, and fixes the background image reftest failures that are marked in the reftest.list, at least on Linux/GTK2.  I have low confidence in the nsTreeBodyFrame.cpp changes, though -- I need to test them more, and perhaps refactor that code a little more.

Comment 11

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

Attachment: patch

So I reread the nsTreeBodyFrame changes and tested them a bit, and I'm comfortable with them.

I'd like to test breaking images when printing, but I can't seem to get images to break at all.  I was sort of expecting either http://www.flickr.com/photo_zoom.gne?id=401336414&size=o or the image inside of it (in a standalone image document) to break, but it didn't -- in a trunk build without my changes.

In any case, stuart, could you review the gfx changes and the nsLayoutUtils.cpp changes, and roc could you review the rest and sr the whole thing?

Note that I moved nsRenderingContextThebes::DrawImage to nsLayoutUtils, but left the code in nsThebesImage where it is.  Stuart suggested entirely using thebes, but nsThebesImage encapsulates all the solid color stuff for single pixel images.  Perhaps at some point in the future nsThebesImage should move into thebes, and have an interface other than nsIImage to get at it.  But for now I'm just passing gfxRects through nsIImage, which was a bit ugly, but seems the least ugly simple solution I could think of.

Comment 12

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

+      destRect.size.width  = (srcRect.size.width)*xscale + 1 - xscale;
+      destRect.size.height = (srcRect.size.height)*yscale + 1 - yscale;

Now that we're not rounding, do we need this 1 - scale term? I'm not actually sure why it's there.

+   *   @param aDirtyRect        The region that was invalidated.

Don't you really mean "draw only this area"? This is more general than invalidation.

I need to think more about what you're doing here, but I'm suspicious of this claim:
+  // And likewise for the dirty rect.  (Is should be OK to round to
+  // nearest rather than outwards, since any dirty rects coming from the
+  // OS should be on pixel boundaries; the rest is other things it's
+  // been intersected with, and we should be rounding those consistently.)

E.g. if we're clipping to the bounds of a frame that's not on pixel boundaries, we could have any dirty rect.

Comment 13

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

(In reply to comment #12)
> +   *   @param aDirtyRect        The region that was invalidated.
> 
> Don't you really mean "draw only this area"? This is more general than
> invalidation.

Right, it's the intersection of invalidation and clipping.  I'll fix the comment to reflect that.

> I need to think more about what you're doing here, but I'm suspicious of this
> claim:
> +  // And likewise for the dirty rect.  (Is should be OK to round to
> +  // nearest rather than outwards, since any dirty rects coming from the
> +  // OS should be on pixel boundaries; the rest is other things it's
> +  // been intersected with, and we should be rounding those consistently.)
> 
> E.g. if we're clipping to the bounds of a frame that's not on pixel boundaries,
> we could have any dirty rect.

I still think this is ok -- the frame's border, background, and background image are all rounded in the same manner, so it seems like we should round its clipping rect the same way.  (I actually originally was going to do expansion, but decided that was unnecessary and wrote the comment above.)

I still need to look into the first comment.

Comment 14

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

It seems safer to me to round out the dirty rect (and clip it to the image rect). That way we're not relying on any assumptions about how it's being used.

Comment 15

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

(In reply to comment #14)
> It seems safer to me to round out the dirty rect (and clip it to the image
> rect). That way we're not relying on any assumptions about how it's being used.

We have to be consistent in the way we snap things to pixel boundaries or we'll break lots of things (cause gaps, misalignment, etc.).  So I think it really should be safe to assume that we're consistent.  (I'm testing that consistency in layout/reftests/pixel-rounding/ too.)

Comment 16

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

OK

Comment 17

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

(In reply to comment #12)
> +      destRect.size.width  = (srcRect.size.width)*xscale + 1 - xscale;
> +      destRect.size.height = (srcRect.size.height)*yscale + 1 - yscale;
> 
> Now that we're not rounding, do we need this 1 - scale term? I'm not actually
> sure why it's there.

I don't understand what that code is trying to do, so I'd rather not change it.

Comment 18

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

Comment on attachment 256589 [details] [diff] [review]
patch

-    nscoord y = GetContinuationOffset(&aMetrics.width);
-    aMetrics.height -= y + mBorderPadding.top;
+    aMetrics.width = GetPrevInFlow()->GetSize().width;

This is odd (the original code I mean). How about just not setting aMetrics.width here?

I think we should remove that not-understood 1-scale factor --- preferably as a separately checked-in patch so we can pinpoint regressions more easily.

Comment 19

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

Attachment: patch

This is slightly revised in nsImageFrame to fix asserts that it was triggering (by splitting UpdateIntrinsicSize and RecalculateTransform).  And switching review request from stuart to vlad per IRC discussion

Comment 20

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

Comment on attachment 259047 [details] [diff] [review]
patch

Looks fine to me..

For posterity, if someone wants to tweak this code, it may be possible to simplify it by clipping to pxDirty and always drawing pxSrc to pxDest, though you'd have to munge all of them for the imgFrame-wants-to-start-at-different-position issue.

Comment 21

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

(In reply to comment #18)
> (From update of attachment 256589 [details] [diff] [review])
> -    nscoord y = GetContinuationOffset(&aMetrics.width);
> -    aMetrics.height -= y + mBorderPadding.top;
> +    aMetrics.width = GetPrevInFlow()->GetSize().width;
> 
> This is odd (the original code I mean). How about just not setting
> aMetrics.width here?

I think we should actually keep this, at least for now, since things would be pretty broken if an image were split and it was at different widths (which it could be given different paper sizes and CSS @page).  That said, we should fix those things (it doesn't look too hard, at least if we have a way of testing it).  I'll file a followup bug.

> I think we should remove that not-understood 1-scale factor --- preferably as a
> separately checked-in patch so we can pinpoint regressions more easily.

OK, I'll try to remember to do that tomorrow...

Comment 22

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

> things would be pretty broken if an image were split and it was at different
> widths (which it could be given different paper sizes and CSS @page).

Why?

Comment 23

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

We figure out what part of the image to draw based on the assumption that it's all drawn at the same scale.  So we'd end up double-drawing some pieces or leaving gaps.

Comment 24

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

Anyway, patch checked in to trunk about an hour ago.

SeaMonkey sea-linux-tbox: Zdiff:-1632 (+7208/-8840)
Firefox argo-vm Linux nightly: Zdiff:-924 (+5944/-6868)

still need to file the followup bugs.

Comment 25

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

Filed bug 376178 and bug 376181 as followup; marking this bug as fixed.

Comment 26

[Jeff Walden [:Waldo]]

Already tested by the tests this bug fixed.