GL layers cause too much painting

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: bzbarsky, Assigned: mattwoodrow)

Tracking

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Assignee

Comment 5

9 years ago
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
Assignee

Comment 10

9 years ago
Fixed review suggestions.

Carrying forward r=joe
Attachment #503977 - Attachment is obsolete: true
Attachment #504175 - Flags: review+
Assignee

Comment 11

9 years ago
Rebased against tip for landing.

Carrying forward r=joe
Attachment #504175 - Attachment is obsolete: true
Attachment #504591 - Flags: review+
Assignee

Updated

9 years ago
Blocks: 593733
Assignee

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/31b46f48f714
Status: NEW → RESOLVED
Closed: 9 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.