621778 - GL layers cause too much painting

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 585258 comment 45.  The short of it is that BasicTextureImage::BeginUpdate replaces the dirty region with its bounding rect, which can cause a huge amount of extra painting in the common case of a page updating two or three completely distinct areas (e.g. an fps counter and whatever it's actually animating).

If we just don't want to deal with regions that are too complicated, we can simplify the region, but I don't think we should be simplifying to one rect.

Comment 1

[Joe Drew (not getting mail)]

This doesn't need to block the release. I agree that it's sub-optimal, but this is the sort of thing we could ship in the next release, since fixing it has the possibility of being a bit involved.

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK.  It just leads to really (pathologically; think O(N^2) instead of O(N)) bad behavior in some cases....  If fixing this is too involved, I can see punting this; should it block 2.x?

Comment 3

[Joe Drew (not getting mail)]

Yeah, maybe.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Upload texture data in regions

Pushed to try server as http://hg.mozilla.org/try/pushloghtml?changeset=336606709ba9

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Upload texture data in regions v2

Updated docs for UploadSurfaceToTexture since the meaning of the src/dest offset stuff swapped. This better matches the way we use the code and makes everything cleaner.

Comment 6

[Oleg Romashin (:romaxa)]

Does it make sense do multiupload for Begin/End-Update implementation too?

Comment 7

[Markus Stange [:mstange]]

+#ifdef USE_GLES2
+    if (imageSurface->Stride() != iterRect.width * pixelSize) {
+      // Not using the whole row of texture data and GLES doesn't 
+      // support GL_UNPACK_ROW_LENGTH. We need to upload each row
+      // separately.
+      if (!textureInited) {
+        fTexImage2D(LOCAL_GL_TEXTURE_2D,
+                    0,
+                    internalformat,
+                    iterRect.width,
+                    iterRect.height,
+                    0,
+                    format,
+                    type,
+                    NULL);
+      }
+
+      for (int h = 0; h < iterRect.height; h++) {
+        fTexSubImage2D(LOCAL_GL_TEXTURE_2D,
+                       0,
+                       iterRect.x,
+                       iterRect.y+h,
+                       iterRect.width,

This needs to be "iterRect->" instead of "iterRect.".

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 501256 [details] [diff] [review]
Upload texture data in regions v2

>diff --git a/gfx/thebes/GLContext.cpp b/gfx/thebes/GLContext.cpp

>     // the basic impl can only upload updates to rectangles
>-    aRegion = nsIntRegion(mUpdateRect);
>+    aRegion = mUpdateRegion;

That comment no longer applies, does it? :)
 
>@@ -1349,71 +1364,83 @@ GLContext::UploadSurfaceToTexture(gfxASu

>+  nsIntRegionRectIterator iter(paintRegion);
>+  nsIntPoint topLeft = paintRegion.GetBounds().TopLeft();
>+  const nsIntRect *iterRect;
>+
>+  while ((iterRect = iter.Next())) {
>+
>+      unsigned char *rectData = 
>+        data + DataOffset(imageSurface, iterRect->TopLeft() - topLeft);

It took me a long while to work out why |iterRect->TopLeft() - topLeft| was necessary. A comment with a little text diagram could help a lot with that.

Comment 9

[Markus Stange [:mstange]]

Attachment: Upload texture data in regions v2

unbitrotted after bug 618628, comments not addressed

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Upload texture data in regions v3

Fixed review suggestions.

Carrying forward r=joe

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Upload texture data in regions v4

Rebased against tip for landing.

Carrying forward r=joe

Comment 12

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/mozilla-central/rev/31b46f48f714