382458 - repainting problems with not pixel aligned controls and active visual style (native theme painting)

Status: RESOLVED WORKSFORME

(Core Graveyard :: GFX: Win32, defect, P2)

Description

[Jonathan Haas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070529 Minefield/3.0a5pre
Build Identifier: 

There are some problems when redrawing controls with are not pixel aligned. See testcase. A similar problem happens sometimes when scrolling.

Reproducible: Always

Steps to Reproduce:
1. Make sure you use windows and a visual style (like Luna).
1. Load testcase
2. Click on "Start testcase" or "Start testcase2".
Actual Results:  
Input-Background disappears

Expected Results:  
No Problems

Comment 1

[Jonathan Haas]

Attachment: Testcase

Comment 2

[Jonathan Haas]

Attachment: Another Testcase

Comment 3

[Jonathan Haas]

I did not checked it but I would bet that this regressed because of the "fix the use of units".

Comment 4

[-fullmetaljacket-]

why this blocks 376124?

Comment 5

[Jonathan Haas]

Because in testcase 2 there are random lines rendered when scrolling (as described in bug 376124). If the blocking is wrong I can remove it.

Comment 6

[Jean-Marc Desperrier]

Reporter, you say this regressed, but do you have any date ? (AFAIC testcase 1 saw some enhancement in recent nightlies, the button only partially disappears now in vertical mode)

It's also not 100% proven there's a connexion between the problem in testcase 1 and in testcase2.

Comment 7

[Jonathan Haas]

Works: 2007020611
Fails: 2007020704 

(Both testcases)

I do not see any enhancements in current nighly (20070614)

Comment 8

[Jean-Marc Desperrier]

Confirming the regression windows, reducing it to be between 2007020617 and 2007020702.

This proves almost 100% it's a regression from bug 177805 :
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1170755000&maxdate=1170852000

Vladimir, I'll take advantage of you apparently watching this bug closely to ask if you can check if the similar "scrolling+lines" bug 384143 is really caused by your patch in bug 379834.

Comment 9

[Jonathan Haas]

Attachment: Testcase 3

Notice that the problem happens only when the control is moved exactly 1/2 pixel, so it may be some strange rounding problem.

I'm pretty sure that all testcases describe the same bug.

Comment 10

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

Attachment: screenshot of various native control rounding

So here's a screenshot of the html code that's in the screenshot; the red div in each case is 50px wide.

From left to right, we have 1.9 + my patch, 1.9 on trunk, and 1.8

current 1.9 trunk looks the same as 1.8, but I think that both are wrong.  Native widgets can't be anything other than aligned to whole pixels.  So when we have a rect like x: 0.2 y: 0.2 width: 50 height: 50, that widget would ideally be from x=0.2 to x=50.2.  But since it can't, we have to decide how to round.

One option is to round the corners; so we'd round 0.2 to 0, and 50.2 to 50.  We'd have a 50px wide widget starting at 0px.  This is pretty natural, and is what happens on the trunk and on 1.8.  However, code in the view manager that translates coordinates from view coords to widget coords uses ScaleRoundOut -- so it will always round outward.  The above rectangle ends up at x=0 to x=51 (because 50.2 rounded "out" is 51).  So we end up with a 51px wide element when you specified 50px, which I'm not really happy with.

Comment 11

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

Attachment: potential fix

So here's the patch; it fixes the testcases, but it does have the issue of an element at 0.2 with width 50px ends up being rendered at 0 with a width of 51px.  It basically makes the native rendering code convert from gfxRects to native (integer-aligned) rects using RoundOut instead of rounding corners.

roc, what do you think?

Comment 12

[Eli Friedman]

(In reply to comment #10)
> However, code in the view manager that
> translates coordinates from view coords to widget coords uses ScaleRoundOut --
> so it will always round outward.

What code are you referring to?  The viewmanager uses ScaleRoundOut in some invalidation code, but nowhere else as far as I can tell.  It looks like the bug is in DrawWidgetBackground.

Comment 13

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

What Eli said ... the rounding used by invalidation should not be an issue, as long as it includes the area we actually draw for the widget, which ScaleRoundOut should...

Comment 14

[Eli Friedman]

Attachment: Patch

vlad had the right idea in that he caught the rounding mistakes in gfxWindowsNativeDrawing, but he missed the issues in nsNativeThemeWin.

Comment 15

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

+  gfxFloat p2a = (gfxFloat)dc->AppUnitsPerDevPixel();

Use constructor-style casts.

+  tr.pos.x = aRect.x / p2a;
+  tr.pos.y = aRect.y / p2a;
+  tr.size.width  = ((aRect.width + aRect.x) / p2a) - tr.pos.x;
+  tr.size.height = ((aRect.height + aRect.y) / p2a) - tr.pos.y;

Add a method to gfxRect to do this? ScaleInverse or something. Also I don't think you have to do the "scale X/YMost" trick here, because we're not rounding.

+    roundedRect.Round();

I know you didn't write it, but gfxRect::Round() really needs some documentation...

+    rout.left   = (LONG) roundedRect.X();

Constructor-style casts again.

Comment 16

[Eli Friedman]

Attachment: Patch v2

Updated to comments.

Comment 17

[Eli Friedman]

Checked in.

Comment 19

[Jean-Marc Desperrier]

Confirming as fixed in latest nightly

Comment 20

[Jonathan Haas]

There are still some cases with the same repainting problems. Reopening...

Comment 21

[Jonathan Haas]

Attachment: testcase 4

See Testcase.

Comment 22

[Eli Friedman]

My guess is that we're rounding relative to the wrong translation... I'm not sure how to get the root translation in gfxWindowsNativeDrawing, though.  (Expanding the clip rect by a pixel "works", but isn't really the right fix...)

Comment 23

[Eli Friedman]

Attachment: Additional patch

I'm not completely sure what's going on, but this fixes it.  This patch is trivially correct because conservatively estimating the height/width.  There might be a better approach, though...

Comment 24

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

Comment on attachment 275903 [details] [diff] [review]
Additional patch

Good enough for now -- can you add a comment above those lines pointing at this bug as an explanation for the + 1's, and a notice that if someone is motivated, they should dig deeper for a root cause? :)

Comment 25

[Eli Friedman]

Checked in.  Please file additional bugs for any remaining issues.

Comment 26

[Jonathan Haas]

Testcase 4 still shows the error for me using 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082111 Minefield/3.0a8pre

Comment 27

[Eli Friedman]

Oops, I guess that's what happens when I have changes all over my tree.  Additional patch to really fix this coming up.

Comment 28

[Eli Friedman]

Attachment: Additional patch 2

Okay, this fix ought to do it.

Comment 29

[Martijn Wargers (dead)]

Vlad, could you review "Additional patch 2"?

Comment 30

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

Comment on attachment 277612 [details] [diff] [review]
Additional patch 2

Erm, can you explain why this fixes it?

Comment 31

[Tony Chung [:tchung]]

Hi, can you please update the target milestone for this bug?   Thanks.

Comment 32

[Reed Loden [:reed]]

Eli, can you answer vlad's question?

Comment 33

[Jonathan Haas]

Eli doesn't seem to answer, but I think this fixes it, because, it doesn't clip the control background anymore (which probably result in rounding errors), so the clipping hopefully depends now on some working code that clips everything else, too.

Unfortunately I'm not sure about this and I'm not Eli of course, but It's really stupid that this bug with a patch isn't fixed for so a long time. So please apply the patch or let somebody else review this stuff.

Comment 34

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

Eli's inactive. Vlad, you'll have to figure it out on your own.

Comment 35

[Eli Friedman]

Ah, sorry, I haven't been reading my bugmail carefully... I wrote this patch a while ago, here's the idea, roughly: we aren't creating the right clip rect, and since we don't actually need to pass in a clip rect, this can be fixed by just passing null.

I'm not completely sure whether this is fix is masking a real issue, though.

Comment 36

[Eli Friedman]

A better fix than the one I posted would be to change http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsNativeThemeWin.cpp&rev=3.127&mark=1278#1278 to use a new TransformToNativeRectRoundOut() method.  (Untested because I'm building on Linux at the moment, but I'm 99% sure it'll work.)

That said, there appears to be a fundamental issue here: essentially, anything calling the Thebes round() methods (like gfxContext::UserToDevicePixelSnapped and gfxWindowsNativeDrawing::TransformToNativeRect) is too sensitive to small rounding errors in the transform matrix (really small, just one or two bits of the mantissa), which causes Thebes to round different ways when we round coordinates in different spots.  This comes about when we do translations: essentially, (a+b)/p2a is not equivalent to a/p2a+b/p2a when dealing with floats.  Specifically, this difference is visible when we're rounding a coordinate that is supposed to have a fractional part equal to 0.5, but ends up with a value slightly less because the intermediates aren't exactly representable.  Of course, it's difficult to come up with testcases because the results are sensitive exactly how the floats are rounded.

For the testcase attached here, we end up with a clip rect that's one pixel smaller than it should be, causing rendering artifacts.  Clip rects are relatively easy to fix by just being a bit more conservative; however, this could easily manifest in other places where rounding is used, like different boxes being misaligned by a pixel.

What needs to happen, I think, is anything that rounds to pixel centers needs to be aware of the current value of AppUnitsPerDevPixel, and force the value to the nearest app-unit before rounding it.  (This is effective because the rounding errors in question are extremely small relative to the size of an app unit.)

Another possibility is to eliminate every use of translation where we care about pixel-precision, and only do pixel rounding for the identity matrix; this fixes the issue because we don't use inexact intermediates in unsafe ways.  However, this approach seems more fragile and difficult to enforce.  The advantage is that it requires only very minor architectural changes, and can be done incrementally.

Comment 37

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

Thanks for that analysis Eli.

In layout we have eliminated almost all translations already to make it possible to find pixel boundaries (in most cases) using AppUnitsToDevPixels-like calculations. It's not that difficult to enforce ... we just make sure there aren't calls to Translate or similar.

Comment 38

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

eli/roc, given comment #36, what do you guys think is the right fix here?  We could take the patches as-is for the immediate fix and then do the translation analysis in the future, or should we try to do that now?

Comment 39

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

Finding out who's doing the translation and eliminating it should actually be easy to do.

Comment 40

[Jonathan Haas]

WFM now using Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100124 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Possibly fixed by Bug 526394