half-pixel-aligned image repaints in wrong place for partial exposes

RESOLVED FIXED

Status

()

Core
Layout: View Rendering
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

12 years ago
This is a regression from bug 317375.  It appears on http://en.wikipedia.org/wiki/International_Date_Line with my user prefs and a 13px minimum font size.  It also appears in the simplified testcase I'm about to attach, even without that minimum font size.  However, you may need to set the browser.screen_resolution pref to 116 to reproduce.

Steps to reproduce:
 1. go to about:config and set browser.screen_resolution to 116
 2. make browser window about 500-600 pixels high
 3. load attached testcase
 4. hit PageDown

Actual results: top of paints where middle of image should be

Expected results: scrolling should always show the correct part of the image
(Assignee)

Comment 1

12 years ago
Created attachment 210941 [details]
mostly-simplified testcase
(Assignee)

Comment 2

12 years ago
Created attachment 210942 [details]
more interesting testcase

Use the same steps to reproduce for this testcase.

It shows that the problem occurs when the top of the image is at certain points:  in particular, it occurs here for images whose margin-top is 6 twips, 18 twips, and 30 twips.  (dpi=116, p2t=12)
(Assignee)

Updated

12 years ago
Summary: image repaints in wrong place for partial exposes → half-pixel-aligned image repaints in wrong place for partial exposes
Sadly, this does not reproduce for me.
(Assignee)

Comment 4

12 years ago
Valgrind does NOT show any warnings when this happens.
(Assignee)

Comment 5

12 years ago
I have checked that I can reproduce this (using attachment 210942 [details]) in the 2006-01-26-04-trunk Linux nightly after setting the dpi pref to 116.
I've set the DPI and it still doesn't happen.
(Assignee)

Comment 7

12 years ago
The debugging code in nsImageGTK does show the following, which is of interest:

nsImageGTK::Draw(0x9f496c0) s=(   0  244   10  485) d=( 225    0   10  485)
nsImageGTK::Draw(0x9f496c0) s=(   0  244   10  485) d=( 239    0   10  485)
nsImageGTK::Draw(0x9f496c0) s=(   0  244   10  485) d=( 253    0   10  486)
nsImageGTK::Draw(0x9f496c0) s=(   0  243   10  486) d=( 267    0   10  486)
nsImageGTK::Draw(0x9f496c0) s=(   0  243   10  486) d=( 281    0   10  486)
(Assignee)

Comment 8

12 years ago
So it seems that nsImageGTK has tons of completely broken and hopefully also completely unused code:

  Relevant to this bug:  The entire section of nsImageGTK::Draw (10 params)
  inside of
    if ((srcWidth != dstWidth) || (srcHeight != dstHeight)) {
  ignores the aSX and aSY parameters.

  While I was there:  The 6 parameter version passes the arguments incorrectly
  to the 10 parameter version (uses mWidth/mHeight instead of aWidth/aHeight
  for the dest rect).

Since this code is going away soon, I think I'll just worry about the layout side of this.
(Assignee)

Comment 9

12 years ago
Created attachment 211775 [details] [diff] [review]
possible patch

This does fix the problem (note that it's close to, but not exactly, a reversion of something that was in roc's original patch).

However, I really don't think this should ever matter if the height is an integral number of pixels, which it really *should* be.
(Assignee)

Comment 10

12 years ago
(In reply to comment #9)
> However, I really don't think this should ever matter if the height is an
> integral number of pixels, which it really *should* be.

Though I suppose we end up with that problem because nsTransform2D is doing floating point math and then rounding, and after doing floating point math, something that's supposed to come out at exactly 0.5 could end up rounding either way.  At least, that's a theory -- I haven't actually tested it.
(Assignee)

Comment 11

12 years ago
Created attachment 211785 [details] [diff] [review]
another patch that fixes the bug

This also fixes the bug, but it breaks the invariant that nsTranform2D's rect transformation functions are equivalent to transforming both points.  I think it helps to show that the problem is inconsistent rounding at 0.5 (although I probably should just verify that in the debugger).

For a second I was thinking that this was actually a better fix, but it's not.

The other fix is still quite strange in that it really only fixes the problem for non-scaled DrawImage calls.
(Assignee)

Updated

12 years ago
Attachment #211785 - Flags: review-
(Assignee)

Comment 12

12 years ago
Comment on attachment 211775 [details] [diff] [review]
possible patch

I should also add a comment that this whole business only makes sense for non-scaled images.  I really don't have a clue for how one would fix this for scaled images, if this code were used for scaled images (which I suspect it's not, since nsImageGTK's scaling code is so broken -- do we do a complete paint of scaled images every time? -- still, could it break even that somehow?  I suspect it could change it, but it wouldn't be considered broken).
Attachment #211775 - Flags: superreview?(roc)
Attachment #211775 - Flags: review?(roc)
(Assignee)

Comment 13

12 years ago
(In reply to comment #11)
> it helps to show that the problem is inconsistent rounding at 0.5 (although I

What I mean by that, for what it's worth, is that 1/12 can't be represented precisely in floating point, so some odd multiples of 6, when multiplied by (1.0f/12.0f), yield a result that NSToCoordRound rounds up, and some yield a result that it rounds down.  At least that's my theory.
(Assignee)

Comment 14

12 years ago
(In reply to comment #13)
> What I mean by that, for what it's worth, is that 1/12 can't be represented
> precisely in floating point, so some odd multiples of 6, when multiplied by
> (1.0f/12.0f), yield a result that NSToCoordRound rounds up, and some yield a
> result that it rounds down.  At least that's my theory.

Still, I don't see how incorrect rounding would follow from inexact representation.
(Assignee)

Comment 15

12 years ago
An example of what's happening is this:

aSrcRect={0,8538,120,210}(120,8748) aDestRect={3036,6948,120,210}(3156,7158)
sr={0,712,10,17}(10,729) dr={253,0,10,18}(263,18)

(gdb) p (aSrcRect.x) * mTranMatrix->m11 + mTranMatrix->m21
$1 = -579
(gdb) p (aDestRect.x) * mTranMatrix->m11 + mTranMatrix->m21
$2 = -325.99999246001244
(gdb) p (aSrcRect.x + aSrcRect.height) * mTranMatrix->m11 + mTranMatrix->m21
$3 = -561.49999947845936
(gdb) p (aDestRect.x + aDestRect.height) * mTranMatrix->m11 + mTranMatrix->m21
$4 = -308.49999193847179

This doesn't completely agree with my analysis -- I'm not sure why we're dealing with actual half-pixel heights.
(In reply to comment #12)
> (From update of attachment 211775 [details] [diff] [review] [edit])
> I should also add a comment that this whole business only makes sense for
> non-scaled images.  I really don't have a clue for how one would fix this for
> scaled images, if this code were used for scaled images (which I suspect it's
> not, since nsImageGTK's scaling code is so broken -- do we do a complete
> paint of scaled images every time?

See http://lxr.mozilla.org/mozilla/source/layout/generic/nsImageFrame.cpp#1325
Comment on attachment 211775 [details] [diff] [review]
possible patch

This is a horrible disgusting mess, that thankfully will soon disappear into the ocean.
Attachment #211775 - Flags: superreview?(roc)
Attachment #211775 - Flags: superreview+
Attachment #211775 - Flags: review?(roc)
Attachment #211775 - Flags: review+
(Assignee)

Comment 18

12 years ago
Fix checked in to trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Assignee: roc → dbaron
You need to log in before you can comment on or make changes to this bug.