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.

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.

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.

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

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.

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.

unbitrotted after bug 618628, comments not addressed

Fixed review suggestions. Carrying forward r=joe

Rebased against tip for landing. Carrying forward r=joe