510979 - OnPaintImageDDraw16 repaints the entire screen on every call

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.1

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 394901 [details] [diff] [review]
patch v.1

I think we should only be setting mRegion on WINCE.

Maybe we should just implement GetRandomRgn in terms of the mRegion that we are setting in invalidate.  In this way, our code between wince and win32 would look more similar.

What do you think?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

adds function GetRegionToPaint to nsWindow

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.3

addresses comments from doug on irc

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 395893 [details] [diff] [review]
patch v.3



The biggest point is that -- I am not sure if I would use nsWindow::GetRegionToPaint for all platforms.  I liked your patch v.1 better -- it was missing some of the comments, and correctness, but was clearer what we are painting in each of the OnPaint calls.  I would like another windows peer to look at this too.  vlad maybe?


If he is happy, here is the remaining comments I have:

the return type of:

nsCOMPtr<nsIRegion> nsWindow::GetRegionToPaint(..)

Should just be nsIRegion.  Although using nsCOMPtr is correct, it is uncommon in our source code.  The more common pattern is to just use the raw type without the smart pointer decoration.


Comment what this is for:

+#ifdef WINCE
+  mInvalidatedRegion = do_CreateInstance(kRegionCID);


AddRECTToRegion -> AddRectToRegion reads better.  I do not think Rect is a type, so it is probably just as good.


Otherwise looks fine.


Vlad, what do you think of this new method for getting the region to paint?

Comment 5

[Doug Turner (:dougt)]

in IRC, vlad mentioned that if mInvalidatedRegion was ever not empty and a dialog (or similar OS dialog) is hidden in front of the gecko window, we will not repaint that area.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 395893 [details] [diff] [review]
patch v.3

After talking this over with vlad, it is probably better to draw faster on windows mobile and have an edge case where by sometime there could be an OS window that causes us to skip a drop.

could you post a new patch that has such a comment in GetRegionToPaint() and then make sure that the region handling is Windows Mobile only.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.4

Has the comment Doug asked for

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fennecmark results before

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fennecmark results after

Fennecmark shows a it took 55% of the time to pan 11384 pixels (1.8x speed up) with this patch versus without it

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 396948 [details] [diff] [review]
patch v.4

The mInvalidatedRegion should all be ifdef only for Windows Mobile -- not WINCE.  We do not want other platforms (e.g. Fennec on CE) having to pay the cost of dealing with this management.

(this is going to cause GetRegionToPaint is going to get more complex, i think)


+  HRESULT                 PaintRectImageDDraw16(RECT, nsPaintEvent*);

Please add a variable name to be consistent with the rest of the declarations

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.5

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 397033 [details] [diff] [review]
patch v.5

add variable names to:

+  HRESULT                 PaintRectImageDDraw16(RECT, nsPaintEvent*);


Align:
+  paintRgn = ::CreateRectRgn(ps.rcPaint.left, ps.rcPaint.top,
+                                  ps.rcPaint.right, ps.rcPaint.bottom);

fix and r+

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/82e2e7d9c18f

Comment 14

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

+    // XXX: we may not recieve invalidates when an OS dialog obsures out window 
+    // and dismisses, so mInvalidatedRegion may not be complete

Isn't this a major flaw in this approach?

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #14)
> +    // XXX: we may not recieve invalidates when an OS dialog obsures out
> window 
> +    // and dismisses, so mInvalidatedRegion may not be complete
> 
> Isn't this a major flaw in this approach?

I don't see this happening while testing on a device.  I've tested by popping another window over Fennec and by popping a message box up over Fennec.  In both cases I see no drawing artifacts. 

In the case of the message box, I see a repaint of (0,0), (480,330) which would be the bounding rect that includes the title bar that popped in plus the message box. In the case of the full screen window I see (0,0), (480,640), which would a be a full screen repaint.

Comment 16

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

It is, though even if it happens it's acceptable for Fennec (given that it's fullscreen and if it does happen, a simple pan would refresh).

Comment 17

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

The code looks like this:

+  if (!mInvalidatedRegion->IsEmpty()) {  
+    // XXX: we may not recieve invalidates when an OS dialog obsures out window 
+    // and dismisses, so mInvalidatedRegion may not be complete
+    paintRgnWin = mInvalidatedRegion.forget();
+    mInvalidatedRegion = do_CreateInstance(kRegionCID);
+    mInvalidatedRegion->Init(); 
+    return paintRgnWin;
+  }
+# endif
+  paintRgn = ::CreateRectRgn(ps.rcPaint.left, ps.rcPaint.top,
+                                  ps.rcPaint.right, ps.rcPaint.bottom);

So if mInvalidatedRegion is nonempty then we ignore the OS dirty area. So try a test where you have a constantly updating animation and pop up a window on top of that, then dismiss it. I bet sometimes the area under the window will not be repainted correctly.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: screen cast of popping window over animated page

screen cast of test that roc asked for

Comment 19

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

Maybe we can't reproduce it in this testcase, but it seems to me that it can never be correct to ignore the OS-provided dirty rect. If you have an argument that it is correct, please make it.

I think Vlad is arguing in comment #16 that this does introduce a bug, but it's worth taking in exchange for increased painting performance. If that's true, fine, but we need to track this new bug somehow.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

I also think that's what Vlad is arguing in comment #16 and I tend to agree.  I would also add that in my testing designed to demonstrate this potential bug, I haven't been able to do so.

Comment 21

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

Yeah, that's basically what I'm saying -- we have a similar argument on linux, with the XSHM stuff.  Basically, we have code paths that will not be 100% correct in all cases, but offer significant performance wins; apps can opt-in to use those paths as necessary, with them being aware of the tradeoffs/pitfalls.  In the future ideally we'll get to The One Fast Path and get rid of all this junk.  Maybe OpenGL will solve everything :)

Comment 22

[Stuart Parmenter]

Vlad: is there a reason you aren't using this on CE?

Comment 23

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

It seems to me the Xshm bug can actually be fixed, whereas here I have no idea.

Knowingly introducing nondeterministic, user-visible bugs is really bad. If this manifests in an unexpected way, or someone tries to triage the problem who isn't aware of this bug, then lots of time will be wasted, and if it was my time I'd be pretty upset.

Comment 24

[Doug Turner (:dougt)]

Maybe we can leave the WM_PAINT handler alone, and instead have the widget's invalidate calls do the painting directly (when aIsSynchronous is true).  This would allow us to directly use the set of regions that need to be painting and avoid the conservative one RECT to rule them all paint structure that windows gives us.  Even when aIsSynchronous is false, we could have some internal state that reminds us to paint the set of regions after some time.

This will preserve the WM_PAINT semantic of painting whatever windows tells us to paint, and allow us to paint only what is invalidated (by us) directly without involving windows messages.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/94d90ddad6e2