59392 - warnings when handling exposes on linux from new view manager code

Status: NEW

(Core :: Web Painting, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

There are warnings when exposing areas on Linux.  For example when using the
mouse wheel to scroll I see warnings like this:

XXX: Using final transparent rect, x=5490, y=615, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=810, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=990, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=1185, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=1365, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=1545, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=1740, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=1920, width=16, height=1006
XXX: Using final transparent rect, x=5490, y=2115, width=16, height=1006

Comment 1

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

I definitely don't see this on Windows.

Do you see a 1 pixel wide, 67 pixel high gray line anywhere on the right-hand 
side of your window? :-) It looks like there could be an off-by-one error 
somewhere that's making a frame not extend quite far enough to the right to 
cover the whole area. It could be a bug in some code or it could be bad CSS.

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: screen magnification

Comment 3

[Christopher Blizzard (:blizzard)]

I've attached a screenshot.  It's of the bottom of the slider bar in a
scrollbar.  What you see is the upper right hand corner of another window over
the open area underneath the scrollbar.

I can generate those warnings by obscuring the pixel that's the overlapping
window has just obscured.  If I move it one pixel to the left and move it up and
down, no warnings are generated.  It sounds like an off by one error.  This does
not happen on the left or top of the window, only on the bottom and the right.

Comment 4

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

What theme is that?

I'm not sure if I correctly understand your explanation. Could you take a 
large magnified screenshot without the overlapping window and then manually 
highlight the pixels that cause the warnings? :-)

Comment 5

[Christopher Blizzard (:blizzard)]

I am lame.  I haven't done anything with this bug.

Comment 6

[Brian Ryner (not reading)]

So, a large number of these seem to be due to roundoff errors in pixel <--> twip
conversions.  I've found at least one 1-twip-off error and one 1-pixel-off
error. Will attach a patch once I've done some more testing/investigation.

CC'ing evaugahan -- eric, didn't you say there was a recommended way that ALL
pixel/twip conversion should be happening so that we don't introduce roundoff
errors?

Comment 7

[Brian Ryner (not reading)]

Attachment: patch to fix ceiling and RoundExclusive rounding

Comment 8

[Brian Ryner (not reading)]

As you can see by the patch, nsUnitConversion.h has far too many 9's on the
rounding constants.  gcc rounds these internally to 0.5 or 1.0, respectively,
meaning that numbers would get rounded up an entire extra unit in the ceiling
function.

This doesn't fix everything though.  In particular, I found the following case
that occurs when ceiling rounding is used, in the form of nsRect::ScaleRoundOut:

p2t=15.0
t2p=0.0666667

ceil(100 pixels * 15.0 twips/pixel) = ceil(1500 twips) = 1500 twips
ceil (1500 twips * 0.0666667 pixels/twip) = ceil(100.00005 pixels) = 101 pixels

To make things worse, this new value may be converted back to twips, which would
put us off by a full 15 twips.

I would propose changing all instances to use NSIntPixelsToTwips and
NSTwipsToIntPixels.  These use rounding instead of ceiling functions, which
would eliminate errors like the one above.

Comment 9

[Brian Ryner (not reading)]

Attachment: very rough patch

Comment 10

[Brian Ryner (not reading)]

Ok, that patch has a lot of ugly debugging output (for me, anyway) and shouldn't
be checked in.  The reason I attached it is so I could point out that with these
two patches applied, painting in mail compose gets really messed up.  In
particular, except for the first character, nothing paints in the message area
until I cause an expose event.

Here's the debugging output generated from doing this (note that I have some
additional debugging output in nsWindow):

Typing first character:

dirtyRegion->Union(8,8,0,13)
dirtyRegion->Union(8,8,7,14)
dirtyRegion->Union(8,21,21,14)
dirtyRegion->Union(8,21,21,14)
dirtyRegion->Union(8,34,84,14)
dirtyRegion->Union(8,34,84,14)
dirtyRegion->Union(8,47,105,14)
dirtyRegion->Union(8,47,105,14)
dirtyRegion->Union(8,60,0,13)
dirtyRegion->Union(8,73,0,13)
dirtyRegion->Union(8,8,432,78)
nsWindow::InvalidateRegion:  (8,8,432,78)
got rect bounding box: (8,8,432,78)
native damagerect = (8,8,432,78)
scaled damagerect = (120,120,6480,1170)
setting mTmpRgn to damagerect: (120,120,6480,1170)
mOpaqueRgn =  (120,120,6480,1170)
new mTmpRgn =
got bounding box: (0,0,0,0)

Typing second character:
dirtyRegion->Union(8,8,14,14)
dirtyRegion->Union(8,8,14,14)
dirtyRegion->Union(8,21,21,14)
dirtyRegion->Union(8,21,21,14)
dirtyRegion->Union(8,34,84,14)
dirtyRegion->Union(8,34,84,14)
dirtyRegion->Union(8,47,105,14)
dirtyRegion->Union(8,47,105,14)
dirtyRegion->Union(8,60,0,13)
dirtyRegion->Union(8,73,0,13)

As you can see, no Invalidate seems to ever be done for typing the second
character, but I'm not quite sure why.  roc, any ideas?

Comment 11

[Brian Ryner (not reading)]

found out what's going on here, nsRegionGTK is not being smart about empty
regions.  Filed as bug 68642

Comment 12

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

Indeed, the use of ScaleRoundOut is causing problems because it rounds up an
extra twip. A lot of the warnings (but not all) go away with this patch:

Index: nsViewManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/view/src/nsViewManager.cpp,v
retrieving revision 3.170
diff -u -r3.170 nsViewManager.cpp
--- nsViewManager.cpp	2001/02/01 05:59:59	3.170
+++ nsViewManager.cpp	2001/02/13 22:17:52
@@ -1811,6 +1811,7 @@
                 view->GetBounds(viewrect);
                 viewrect.x = viewrect.y = 0;
                 varea = (float)viewrect.width * viewrect.height;
+                damrect.IntersectRect(damrect, viewrect);

                 if (varea > 0.0000001f)
                        {

This trims off any part of the damage rect that is slopping over the edge of the
view that was damaged.

Comment 13

[Brian Ryner (not reading)]

So, do you think we should change the ScaleRoundOut()'s to something else?

Comment 14

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

ScaleRoundOut is only used in the view manager right now. As far as I can tell, 
the other uses of ScaleRoundOut in the view manager are not problematic.

Comment 15

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

bryner:  Regarding the second patch, how many 9's do we want?  In my install of
gcc3, the generated std_limits.h gives |epsilon| (which the C++ standard,
section 18.2.1.2, clause 19, says is "the difference between 1 and the least
value greater than 1 that is representable") for a float as around 1.19e-7.  I
think this means that your constant will work correctly up to around 100, but
will it then start failing again, or not?  I'm not sure.  Who would know?

Did you test that the function works correctly for larger inputs?  What are our
requirements for it, anyway?

Comment 16

[Brian Ryner (not reading)]

dbaron: I just tested this ceiling function on a variety of large numbers.  I
was never able to produce a scenario where it either failed to round up or
incorrectly rounded up.

Comment 17

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 18

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.