Closed Bug 382458 Opened 14 years ago Closed 11 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: jonathan_haas, Assigned: sharparrow1)

References

Details

(Keywords: regression, testcase)

Attachments

(8 files, 2 obsolete files)

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
Attached file Testcase
Keywords: regression, testcase
Attached file Another Testcase
I did not checked it but I would bet that this regressed because of the "fix the use of units".
Flags: blocking1.9?
Blocks: 376124
why this blocks 376124?
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.
No longer blocks: 376124
Depends on: 376124
Assignee: nobody → vladimir
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9? → blocking1.9+
Version: unspecified → Trunk
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.
Works: 2007020611
Fails: 2007020704 

(Both testcases)

I do not see any enhancements in current nighly (20070614)
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha6
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.
Blocks: pixels
No longer depends on: 376124
Attached file 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.
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.
Attached patch potential fix (obsolete) — Splinter Review
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?
Attachment #269500 - Flags: review?(roc)
Attachment #269499 - Attachment is patch: false
Attachment #269499 - Attachment mime type: text/plain → image/png
(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.
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...
Attached patch Patch (obsolete) — Splinter Review
vlad had the right idea in that he caught the rounding mistakes in gfxWindowsNativeDrawing, but he missed the issues in nsNativeThemeWin.
Assignee: vladimir → sharparrow1
Attachment #269500 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #269792 - Flags: review?(roc)
Attachment #269500 - Flags: review?(roc)
Blocks: 375172
Blocks: 385906
+  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.
Attached patch Patch v2Splinter Review
Updated to comments.
Attachment #269792 - Attachment is obsolete: true
Attachment #269977 - Flags: review?(roc)
Attachment #269792 - Flags: review?(roc)
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Confirming as fixed in latest nightly
There are still some cases with the same repainting problems. Reopening...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file testcase 4
See Testcase.
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...)
Summary: repainting problems with not pixel aligned controls and active visual style → repainting problems with not pixel aligned controls and active visual style (native theme painting)
Attached patch Additional patchSplinter Review
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...
Attachment #275903 - Flags: review?(vladimir)
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? :)
Attachment #275903 - Flags: review?(vladimir) → review+
Checked in.  Please file additional bugs for any remaining issues.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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
Oops, I guess that's what happens when I have changes all over my tree.  Additional patch to really fix this coming up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Okay, this fix ought to do it.
Attachment #277612 - Flags: review?(vladimir)
Vlad, could you review "Additional patch 2"?
Comment on attachment 277612 [details] [diff] [review]
Additional patch 2

Erm, can you explain why this fixes it?
Hi, can you please update the target milestone for this bug?   Thanks.
Target Milestone: mozilla1.9alpha6 → ---
Eli, can you answer vlad's question?
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-
Priority: P2 → --
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.
Eli's inactive. Vlad, you'll have to figure it out on your own.
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.
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.
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.
Flags: blocking1.9.1?
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?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P2
Finding out who's doing the translation and eliminating it should actually be easy to do.
Product: Core → Core Graveyard
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
Status: REOPENED → RESOLVED
Closed: 14 years ago11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.