532344 - only issue one NS_PAINT event for each WM_PAINT on WinMo

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

Attachment: WIP patch

As it is we issue one NS_PAINT for each rect in a complex region, even though the event takes care of the region in one go.

Comment 1

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

According to fennecmark, this is a small panning speed win (9797px/27305ms vs. 8957px/25730ms). It consistently halves the max (and initial) panning lag from 20ms to 9ms.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 415580 [details] [diff] [review]
WIP patch

+  pixman_image_t *srcPixmanImage = NULL;
+  pixman_image_t *dstPixmanImage = NULL;


use nsnull instead of NULL.


+  gfxIntSize surfaceSize;      

I think we can declare this below when we set it to gfxIntSize(brw, bra).


-    printf("nothing to paint\n");
-    goto cleanup;
+    NS_WARNING("nothing to paint\n");


i do not think having nothing to paint is a warning.  Lets just remove the printf.


 #ifdef DEBUG
       DDError("CreateSurface renderer", hr);
+#else
+      NS_ERROR("CreateSurface renderer failed");
 #endif


I do not think that we need both.  NS_ERROR is fine by me.



-    PaintRectImageDDraw16(r, &event);
+  {
+    nsRefPtr<gfxContext> thebesContext;


Why the opening { ?  do we need to scope this for any reason?



+    //memset(sSharedSurfaceData.get(), 0, WORDSSIZE(surfaceSize) * 4);
+

lets remove this.


+    nsRefPtr<gfxImageSurface> targetSurfaceImage = new gfxImageSurface(sSharedSurfaceData.get(),
+                                             surfaceSize,
+                                             surfaceSize.width * 4,
+                                             gfxASurface::ImageFormatRGB24);


The white space isn't aligned.  make sure the params line up.


declare this where we need it:

+    nsEventStatus eventStatus = nsEventStatus_eIgnore;

Also, the patch looks like the nsEventStatus is declared in a scope that the call to DispatchWindowEvent doesn't have access to.  it is probably just the way diff works, but double check that we are using the right eventStatus.

+    //surfaceSize = gfxIntSize(rects->mRects[i].width, rects->mRects[i].height);


drop

paintRgnWin->GetRects(&rects);

can rects

+    // Convert RGB24 -> RGB565

This is the wrong comment for create_bits()


+      rcPaint.left = rects->mRects[i].x - brx;
+      rcPaint.top = rects->mRects[i].y - bry;
+      rcPaint.right = rcPaint.left + rects->mRects[i].width;
+      rcPaint.bottom = rcPaint.top + rects->mRects[i].height;


nit: lets keep the rect-> on the same side of the operator for consistency.


+      hr = glpDDSecondary->Lock(0, &gDDSDSecondary, DDLOCK_WAITNOTBUSY | DDLOCK_DISCARD, 0);  /* should we wait here? */


Dropping a paint would be bad.  lets wait.

Also, lets move this outside the for-loop.  there is no reason to lock/unlock multiple times.


+       pixman_image_unref(dstPixmanImage);
+       dstPixmanImage = NULL;
+      hr = glpDDSecondary->Unlock(0);


whitespace is off.  nsnull instead of NULL.  move unlock outside of loop.


+#ifdef DEBUG
+        DDError("Failed to unlock renderer", hr);
+#else
+      NS_ERROR("Failed to unlock renderer");
+#endif


Just NS_ERROR.


Could you explain why we need to Union over the mInvalidatedRegion?


+        if (hr ==  DDERR_INVALIDPARAMS) {

I do not understand why we have to get the Sip involved.  



 cleanup:
- 
...
   return result;

+error:
+  mInvalidatedRegion->Union(*paintRgnWin.get());
+  goto cleanup;


How about if result != NS_OK, you do the error case.  we maybe even get rid of cleanup.

Comment 3

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

Attachment: patch v.2

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 422128 [details] [diff] [review]
patch v.2

Move mPainting below the variable declarations and above BeginPaint()

Move gfxIntSize lower, right above where you init the height/width

do we need to test mInvalidatedRegion?  GetRegionToPaint creates it, but if aForceFullRepaint is always passed, it will be null.

You should move the create_bits stuff outside of the loop.  The loop should just have composite inside of it.

Not needed, right?
+      SetLastError(0);


I am sure you don't want this left in:
+        printf("error mapping points\n");


You probably want to have a note above mInvalidatedRegion->Union on why we can depend on this failure option.  Also, do we want to set to result accordingly.

Instead of just looping on DDERR_WASSTILLDRAWING, do you want to try once, doing it async, then do it sync?  I'd wonder if there is any perf difference

Comment 5

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

Attachment: patch v.3

Comment 6

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

(In reply to comment #4)
> do we need to test mInvalidatedRegion?  GetRegionToPaint creates it, but if
> aForceFullRepaint is always passed, it will be null.

not sure I follow here. aForceRullRepaint is always set to false here. What are you suggesting?

> Not needed, right?
> +      SetLastError(0);

actually it is, see the return value section here http://msdn.microsoft.com/en-us/library/dd145046%28VS.85%29.aspx.

I added a comment since its not obvious

Comment 7

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

pushed http://hg.mozilla.org/mozilla-central/rev/7de69a6e5983

Comment 8

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

followed up with http://hg.mozilla.org/mozilla-central/rev/6ea1b12adda1 to fix a wince burning. basically ifdef'd the re-invalidation to WINCE_WINDOWS_MOBILE

Comment 9

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

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8754ad119a62 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/92f62d49d4f9