Closed Bug 621778 Opened 11 years ago Closed 11 years ago

GL layers cause too much painting


(Core :: Graphics, defect)

Not set



Tracking Status
blocking2.0 --- .x+


(Reporter: bzbarsky, Assigned: mattwoodrow)




(1 file, 4 obsolete files)

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.
blocking2.0: --- → ?
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.
blocking2.0: ? → -
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?
Yeah, maybe.
blocking2.0: - → .x
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.
Attachment #501254 - Attachment is obsolete: true
Attachment #501256 - Flags: review?(joe)
Attachment #501254 - Flags: review?(joe)
Does it make sense do multiupload for Begin/End-Update implementation too?
+#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 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.
Attachment #501256 - Flags: review?(joe) → review+
unbitrotted after bug 618628, comments not addressed
Attachment #501256 - Attachment is obsolete: true
Fixed review suggestions.

Carrying forward r=joe
Attachment #503977 - Attachment is obsolete: true
Attachment #504175 - Flags: review+
Rebased against tip for landing.

Carrying forward r=joe
Attachment #504175 - Attachment is obsolete: true
Attachment #504591 - Flags: review+
Blocks: 593733
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 629016
Depends on: 632781
Depends on: 641250
Assignee: nobody → matt.woodrow+bugzilla
You need to log in before you can comment on or make changes to this bug.