Last Comment Bug 607687 - Fennec should take care about Texture MAX size HW limitation
: Fennec should take care about Texture MAX size HW limitation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM MeeGo
: -- normal (vote)
: ---
Assigned To: Florian Hänel [:heeen]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 671986
Blocks: opengl-mobile 619056
  Show dependency treegraph
 
Reported: 2010-10-27 11:14 PDT by Oleg Romashin (:romaxa)
Modified: 2011-11-26 04:12 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Low level MAX_TEXTURE_SIZE check (8.96 KB, patch)
2010-11-05 07:30 PDT, Oleg Romashin (:romaxa)
vladimir: feedback-
Details | Diff | Splinter Review
Max texture size API (2.17 KB, patch)
2010-12-13 04:21 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ShadowBufferOGL multi texture (8.76 KB, patch)
2010-12-13 04:24 PST, Oleg Romashin (:romaxa)
cjones.bugs: feedback-
Details | Diff | Splinter Review
Updated to trunk (8.49 KB, patch)
2011-03-14 16:35 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Implementation moved a little bit down (11.83 KB, patch)
2011-03-15 08:00 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
different approach - WIP (20.20 KB, patch)
2011-03-15 09:35 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
WIP - tiling textureimage (29.69 KB, patch)
2011-03-16 09:45 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
multi texture image aka tiledTextureImage proof of concept (30.35 KB, patch)
2011-03-22 06:37 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
rebased (on top of bug 628566), but gives compile error (30.11 KB, patch)
2011-04-29 13:35 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
implement TiledTextureImage::BindTexture (3.18 KB, patch)
2011-05-03 05:58 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
implemented tiled texture blit (38.96 KB, patch)
2011-05-16 08:22 PDT, Florian Hänel [:heeen]
joe: review+
Details | Diff | Splinter Review
fixed nits (43.03 KB, patch)
2011-05-30 06:31 PDT, Florian Hänel [:heeen]
joe: review+
Details | Diff | Splinter Review
interdiff from previous r+ patch (16.23 KB, patch)
2011-05-30 15:01 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
same patch with headers... ready to import (43.23 KB, patch)
2011-06-04 08:01 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Interdiff Fix for Mac failure + compile warnings (6.27 KB, patch)
2011-06-09 22:15 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Updated to trunk + mac orange fix + compile warnings combined (45.52 KB, patch)
2011-06-10 06:59 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Updated to trunk again (35.55 KB, patch)
2011-06-28 16:09 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Updated to trunk, fixed GLX compilation (41.03 KB, patch)
2011-06-29 18:22 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fixed GLX rendering (possibly Mac too) (41.22 KB, patch)
2011-06-30 16:58 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fixed compile warning, init list (41.22 KB, patch)
2011-07-01 11:09 PDT, Oleg Romashin (:romaxa)
jmuizelaar: review+
Details | Diff | Splinter Review
fixed chris' nits (45.56 KB, patch)
2011-07-04 09:23 PDT, Florian Hänel [:heeen]
jmuizelaar: review+
Details | Diff | Splinter Review
rebased yet again (45.97 KB, patch)
2011-07-06 05:44 PDT, Florian Hänel [:heeen]
florian.haenel: review+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2010-10-27 11:14:31 PDT
Problem is that with SGX drivers on maemo platform we have texture size limitation... and maximum texture size is 2000x2000...

So we have problem here with possibility to have bigger offscreen texture.

in portrait mode we have screen size ~800px, and 2000-800/2 = 600, so 600px is maximum offscreen size we can get with single ThebesLayer texture.
And this is the problem, because if we have fast kinetic scrolling then we going to see checkerboard backround very often...

I see 2 options here:
1) teach layout operate with 2 thebes layers
2) hack ThebesBuffer texture and paint function in order build ThebesBuffer with 2 or more textures.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-10-27 13:48:23 PDT
I vastly prefer option #2.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2010-10-27 18:27:55 PDT
yeah, I do too; will require drawing in a loop and some logic for callers to actually use the TextureImages sanely.
Comment 3 Oleg Romashin (:romaxa) 2010-11-05 07:30:08 PDT
Created attachment 488461 [details] [diff] [review]
Low level MAX_TEXTURE_SIZE check
Comment 4 Oleg Romashin (:romaxa) 2010-11-05 07:47:26 PDT
Comment on attachment 488461 [details] [diff] [review]
Low level MAX_TEXTURE_SIZE check

does it make sense to check texture size before each upload and textureImage creation?
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2010-11-08 12:47:05 PST
Comment on attachment 488461 [details] [diff] [review]
Low level MAX_TEXTURE_SIZE check

I don't think so -- we need the MaxTextureSize getter and a check function on GLContext, but it doesn't make sense to check before upload -- GL itself should be doing that and ignoring the upload if it's out of range.  Code upstream should be doing the checking and making decisions based on the max texture size.

You could add NS_ASSERTs, though, but we could also do those just by wrapping the TexImage2D and RenderbufferStorage functions.
Comment 6 Oleg Romashin (:romaxa) 2010-11-11 09:32:33 PST
Now I'm trying to make working support for ThebesBuffer (remote fennec viewport) be bigger than Max texture size.

And basically we have 2 options here:
1) Low level:
  Make TextureImage class handling bigger size, and create internally textures array, return multiple GFX context for callbac(state...) function e.t.c...
2) High level - just hack quickly ShadowThebesLayerOGL (Upload, RenderTo) and make it works for cached offscreen remote ThebesBuffer only

Any suggestions and ideas here?
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-11 12:54:21 PST
Doing it at the lowest possible level sounds better to me, but I'm not sure. Vlad?
Comment 8 Vladimir Vukicevic [:vlad] [:vladv] 2010-11-11 13:47:15 PST
Well, you can do it either as part of TextureImage or as part of the thebes layer buffer code.  I think doing it as part of the thebes layer buffer code might be cleaner, because you're going to need to call the thebes callback multiple times.  But now that I write that, it means you'll have to maintain N different texture images.  .... but then you have to make actually drawing the texture images quite complex, since you can't really fudge texcoords to be from 0.0 to 1.0 across multiple texture images.

So there's a lot of work here, but I think the "cleanest" would be to do it as part of the *Buffer code in ThebesLayerOGL, have it manage multiple TextureImages, and go from there.

This doesn't resolve the issue of creating too-big textures/renderbuffers for FBOs, though.
Comment 9 Oleg Romashin (:romaxa) 2010-12-13 04:21:57 PST
Created attachment 497241 [details] [diff] [review]
Max texture size API
Comment 10 Oleg Romashin (:romaxa) 2010-12-13 04:24:21 PST
Created attachment 497242 [details] [diff] [review]
ShadowBufferOGL multi texture

This is simple trick around ShadowBufferOGL which makes textures grid if ShadowBufferOGL size is bigger than Max HW limit
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-12-14 11:22:25 PST
Here's the problem as I understand it
 - (canvas|image)layer can in theory be sized arbitrarily
 - we control the size of thebeslayer
 - drivers impose a max texture size of mw x mh
 - it's possible that (canvas|image)layer or thebeslayer could exceed W x H
 - I heard (from jbos?) that the n900 imposes a limit on the number of textures, mt (I recall "21").  That doesn't make a lot of sense to me, but if true is something we need to consider.

So, we need to know mw x mh (and mt if it exists).
 - 2000 x 2000 on the n900 et al. (mt == 21?)
 - what is it on android drivers?
 - 4000 x 4000 with D2D (IIRC)

Currently in fennec, we don't set a displayport larger than 2000x1000, and don't have any immediate plans to do so (we have room to make scrolling smarter, first).  So the texture-size limit isn't an immediate problem.

IIUC, we have the same texture-size problem with (canvas|image)layer on mobile/GL as we do with desktop/D2D: content can create a 5000x5000 canvas, say.  How do we handle this in D2D?
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-12-14 11:28:59 PST
Comment on attachment 497242 [details] [diff] [review]
ShadowBufferOGL multi texture

This basic idea looks mostly OK.  However, this patch isn't going to help us atm.  The first place we would likely want to use this is for canvas.  So, I think we should push tiling into a lower level, as a new class wrapping existing TextureImage (say, TiledTextureImage).

(It's fine that this patch doesn't handle the "hard case" where we're redrawing a tiled thebeslayer inside layers/opengl, for which we'd have to make the thebeslayercallback multiple times, though other solutions are possible.)
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-12-14 11:30:27 PST
(Sorry, meant to CC bas with the group above.)
Comment 14 Matt Woodrow (:mattwoodrow) 2010-12-14 11:30:40 PST
This is similar to bug 615741, though more general.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-12-14 11:56:40 PST
Hmm, yes.  I like Rob's idea for canvas/image.
Comment 16 Oleg Romashin (:romaxa) 2011-03-14 16:35:59 PDT
Created attachment 519270 [details] [diff] [review]
Updated to trunk

Updated to trunk, still does the work.
Comment 17 Oleg Romashin (:romaxa) 2011-03-14 18:08:46 PDT
I think we can just create TiledTextureImage class which contain list of TextureImage's and has some methods easy to use from ThebesLayerOGL.
so we can just wrap most functionality of current patch into single class, don't keep it ThebesLayer specific, and allow to use from other places.
Comment 18 Florian Hänel [:heeen] 2011-03-15 07:02:52 PDT
I started hacking at this to abstract the implementation detail of one or many textures out of TextureImage into BasicTextureImage and TiledTextureImage respectively.

As for painting, the following question arises:
Does it make sense to do updates tiled as well? eg:
 
BeginUpdate:
  return gfxSurface sized for first tile
NextUpdate:
  return gfxSurface for next tile, null if none
Endupdate:
  finish update

OR:

BeginUpdate:
  return gfxASurface as required
EndUpdate:
  do tiled upload from big gfxASurface

There may be limits in offscreen surfaces in addition to texture limits, eg 4k×4k on some platforms, but I couldn't confirm this yet. The X11 protocol allows a theoretical size of 32k×32k, which should be enough for anybody for the forseeable future.
Comment 19 Oleg Romashin (:romaxa) 2011-03-15 08:00:25 PDT
Created attachment 519407 [details] [diff] [review]
Implementation moved a little bit down

Here is simple wrapper class which just wrapping existing functionality
Comment 20 Florian Hänel [:heeen] 2011-03-15 09:35:29 PDT
Created attachment 519426 [details] [diff] [review]
different approach - WIP

After trying to abstract tiled textures into the TiledTextureImage, I found some more issues. For the simple case of just one Texture and Program, TextureImage can take care of drawing itself, repeatedly for each tile if necessary; so the caller doesn't need to know how the TextureImage is represented internally, it just prepares the program parameters.

But I noticed ThebesLayerOGL uses multiple passes with multiple textures (TextureOnWhite...?), where it would need to know that each of the images has multiple textures internally and would have to iterate over these to set both textures to bind them to the program.

To give you an idea, here's what I have been hacking together.

This is where the abstraction leaks and things start to get nasty. I'm not sure this is the right layer to abstract this.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-15 12:35:33 PDT
mattwoodrow and jrmuizel are your best bets for feedback here I suspect
Comment 22 Matt Woodrow (:mattwoodrow) 2011-03-15 17:44:42 PDT
I don't really like the idea of having NextUpdate(), it'd be a lot cleaner to keep the texture size limitations abstracted within TextureImage.

We could run into a problem with this if we ever start using cairo-gl for content rendering as these gfxASurfaces will have the same size limit. I don't think we should worry about this here though.

As for handling component alpha, maybe we need to tell the texture image the region we want to draw (visible region) and have it return sets of texture coordinates and the texture to bind when drawing them. We can then leave the actual drawing code inside ThebesLayerBufferOGL.

Something along the lines of:

mTexImage->BeginDrawing(GetEffectiveVisibleRegion());
mTexImageOnWhite->BeginDrawing(GetEffectiveVisibleRegion());

nsRegion toDraw;

while (mTexImage->GetNextDrawRegion(toDraw))
{
  nsRegion temp;
  NS_ASSERTION(mTexImageOnWhite->GetNextDrawRegion(temp) && temp == toDraw);

  program->SetBlackTextureUnit(mTexImage->GetDrawTexture());
  program->SetWhiteTextureUnit(mTexImageOnWhite->GetDrawTexture());

  //Draw toDraw normally!
}

Will think about this some more though.
Comment 23 Florian Hänel [:heeen] 2011-03-16 09:45:57 PDT
Created attachment 519687 [details] [diff] [review]
WIP - tiling textureimage

Here's some food for thought.
This works for canvas already, but not yet Image or Thebes. I hardcoded a max texture size of 128 to test this with a 1024 sized canvas. It seems to work reasonably well.
Comment 24 Florian Hänel [:heeen] 2011-03-22 06:37:18 PDT
Created attachment 520904 [details] [diff] [review]
multi texture image aka tiledTextureImage proof of concept

Here's my proof of concept for tiling textureimages. Theres still one or two points where I'm missing an implementation, i.e. this Blit function - see my comment in the patch.

Otherwise this seems to work to some extent, but better try it for yourself.
I left some code in the shader to visualize texture seams and left the texture size at 256² for your testing convenience.
Comment 25 Oleg Romashin (:romaxa) 2011-04-02 10:41:58 PDT
Comment on attachment 520904 [details] [diff] [review]
multi texture image aka tiledTextureImage proof of concept


>-  return aGl->CreateTextureImage(aSize, aContentType, wrapMode);
>+//  return aGl->CreateTextureImage(aSize, aContentType, wrapMode);
>+  TextureImage* t=new gl::TiledTextureImage(aGl, aSize, aContentType); //more elegant?
>+  NS_ADDREF(t);
>+  return t;

I guess u can use here nsRefPtr
>+  nsRefPtr<TextureImage> t = new gl::TiledTextureImage(aGl, aSize, aContentType);
>+  return t.forget();


>+    SetBlitFramebufferForDestTexture(textureID);
>+    printf("TODO - BLIT NOT IMPLEMENTED CORRECTLY!\n");

Can we avoid these calls at all?


About 256 size... can we use maximum texture size value here ?

Matt do you have any comments on this approach?
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2011-04-26 07:50:24 PDT
Unfortunately, this patch doesn't apply anymore.
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2011-04-29 13:35:14 PDT
Created attachment 529188 [details] [diff] [review]
rebased (on top of bug 628566), but gives compile error

This is the same patch rebased on top of mozilla-central + the patch in 628566.

However it's not compiling (Android) because TiledTextureImage is pure virtual because TextureImage::BindTexture(GLenum) is.

Here are the compile errors that I get:

/home/bjacob/mozilla-central/gfx/layers/opengl/ThebesLayerOGL.cpp: In function 'already_AddRefed<mozilla::gl::TextureImage> mozilla::layers::CreateClampOrRepeatTextureImage(mozilla::gl::GLContext*, const nsIntSize&, gfxASurface::gfxContentType, PRUint32)':
/home/bjacob/mozilla-central/gfx/layers/opengl/ThebesLayerOGL.cpp:76: error: cannot allocate an object of abstract type 'mozilla::gl::TiledTextureImage'
../../dist/include/GLContext.h:390: note:   because the following virtual functions are pure within 'mozilla::gl::TiledTextureImage':
../../dist/include/GLContext.h:219: note:       virtual void mozilla::gl::TextureImage::BindTexture(GLenum)
/home/bjacob/mozilla-central/gfx/layers/opengl/CanvasLayerOGL.cpp: In member function 'virtual void mozilla::layers::ShadowCanvasLayerOGL::Initialize(const mozilla::layers::CanvasLayer::Data&)':
/home/bjacob/mozilla-central/gfx/layers/opengl/CanvasLayerOGL.cpp:304: error: cannot allocate an object of abstract type 'mozilla::gl::TiledTextureImage'
../../dist/include/GLContext.h:390: note:   because the following virtual functions are pure within 'mozilla::gl::TiledTextureImage':
../../dist/include/GLContext.h:219: note:       virtual void mozilla::gl::TextureImage::BindTexture(GLenum)
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2011-04-29 13:36:45 PDT
Dammit --- there are two patches here. I had applied only the second one, 'multi texture image aka tiledTextureImage proof of concept'.
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2011-04-29 13:43:04 PDT
The first patch doesn't apply at all anymore. For example, this hunk on ThebesLayerOGL.cpp:

@@ -791,31 +792,33 @@ public:
     NS_RUNTIMEABORT("can't BeginPaint for a shadow layer");
     return PaintState();
   }
 
   void
   CreateTexture(ContentType aType, const nsIntSize& aSize)
   {
     NS_ASSERTION(gfxASurface::CONTENT_ALPHA != aType,"ThebesBuffer has color");
-
-    mTexImage = CreateClampOrRepeatTextureImage(gl(), aSize, aType);
+    mTiledTex = new TiledTextureImage(gl(), aSize, aType);
   }
 
There is no longer a CreateTexture method in this file.
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2011-05-03 05:58:00 PDT
Created attachment 529689 [details] [diff] [review]
implement TiledTextureImage::BindTexture

This patch provides the missing impl of TiledTextureImage::BindTexture:

 void
+TiledTextureImage::BindTexture(GLenum aTextureUnit)
+{
+    mImages[mCurrentImage]->BindTexture(aTextureUnit);
+}

It also does some junk (implements TextureImage::BindTexture as NS_ABORT()), that's a leftover from debugging I was doing, no time to clean that up now, sorry.
Comment 31 Florian Hänel [:heeen] 2011-05-16 08:22:53 PDT
Created attachment 532652 [details] [diff] [review]
implemented tiled texture blit

This is now feature complete. I implemented the missing multi-texture blit function and tested it successfully.
The only point I'm not 100% confident in is the scaling between different sized blit rectangles. I use nsIntRect::ScaleRoundOut which, as the name implies, rounds out to int boundaries, but since we calculate texture coordinates based on those rounded rects we should be in the clear I suppose.
Comment 32 Florian Hänel [:heeen] 2011-05-16 08:23:48 PDT
by the way, this bug lives as part of a GLES layers patchset at https://hg.mozilla.org/users/florian.haenel_heeen.de/GLES_layers until accepted
Comment 33 Joe Drew (not getting mail) 2011-05-27 13:09:55 PDT
Comment on attachment 532652 [details] [diff] [review]
implemented tiled texture blit

Review of attachment 532652 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I'd like to take another look at this once you've addressed the review comments, but overall I'm quite pleased.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +1039,5 @@
> +    GLuint textureID;
> +    nsIntRect textureRect;
> +    gl()->fActiveTexture(LOCAL_GL_TEXTURE0);
> +    mTexImage->BeginRender(textureID, textureRect);
> +      do {

indentation weirdness here

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +268,5 @@
>        renderRegion = &visibleRegion;
>      }
> +    mTexImage->BeginRender(textureID, textureRect);
> +    if (mTexImageOnWhite) {
> +        nsIntRect textureRectWhite;

Please add a comment (and maybe an assertion?) that textureRectWhite should be the same as textureRect.

@@ +272,5 @@
> +        nsIntRect textureRectWhite;
> +        mTexImageOnWhite->BeginRender(textureIDWhite, textureRectWhite);
> +    }
> +    nsIntRegion region=mLayer->GetEffectiveVisibleRegion();
> +    nsIntPoint origin=GetOriginOffset();

Please add whitespace before & after the = operator here (and throughout).

@@ +273,5 @@
> +        mTexImageOnWhite->BeginRender(textureIDWhite, textureRectWhite);
> +    }
> +    nsIntRegion region=mLayer->GetEffectiveVisibleRegion();
> +    nsIntPoint origin=GetOriginOffset();
> +    region.MoveBy(-origin);           // transform into TexImage space

Can you elaborate on this comment, saying why we're transforming into TexImage space (i.e., because we're going to intersect the drawing region with each texture's region below, and they have to be in the same space)?

@@ +291,5 @@
> +      nsIntRegionRectIterator iter(subregion);
> +      while (const nsIntRect *iterRect = iter.Next()) {
> +        nsIntRect regionRect = *iterRect;  // one rectangle of this texture's region
> +        nsIntRect screenRect = regionRect; // transform back from texture resolution
> +        screenRect.ScaleRoundOut(1.0f / xres, 1.0f / yres); // to screen resolution

It's probably easier to understand this comment if it's put above the block of code rather than to the right of it (because it applies to multiple lines).

::: gfx/thebes/GLContext.cpp
@@ +656,5 @@
>  {
>  }
>  
>  bool 
> +BasicTextureImage::DirectUpdate(gfxASurface *aSurf, const nsIntRegion& aRegion, const nsIntPoint& aFrom)

For default value parameters like aFrom, I like specifying what their default value is in a comment:

void foo(int a, int b /* = 3 */)

Can you do something similar throughout?

@@ +701,5 @@
>  }
>  
> +TiledTextureImage::TiledTextureImage(GLContext* aGL,
> +        nsIntSize aSize, TextureImage::ContentType aContentType,
> +        PRBool aUseNearestFilter)

Please vertically align the parameters here under GLContext.

@@ +702,5 @@
>  
> +TiledTextureImage::TiledTextureImage(GLContext* aGL,
> +        nsIntSize aSize, TextureImage::ContentType aContentType,
> +        PRBool aUseNearestFilter)
> +    :TextureImage(aSize, LOCAL_GL_CLAMP_TO_EDGE, aContentType, aUseNearestFilter)

Please add a space before TextureImage here.

@@ +720,5 @@
> +bool 
> +TiledTextureImage::DirectUpdate(gfxASurface *aSurf, const nsIntRegion& aRegion, const nsIntPoint& aFrom)
> +{
> +    PRBool result = PR_TRUE;
> +    for(unsigned i = 0; i < mImages.Length(); i++) {

Can you add a space between for and ( here, and throughout your patch?

@@ +731,5 @@
> +        tileRegion.MoveBy(-xPos, -yPos); // translate into tile local space
> +        result &= mImages[i]->DirectUpdate(
> +                aSurf,
> +                tileRegion,
> +                aFrom + nsIntPoint(xPos, yPos));

Here I think aSurf will fit on the previous line, and the rest of the parameters should be vertically aligned under it.

@@ +753,5 @@
> +            aRegion.MoveBy(-xPos, -yPos);   // adjust for tile offset
> +            nsRefPtr<gfxASurface> surface = mImages[i]->BeginUpdate(aRegion); // forward it
> +            aRegion.MoveBy(xPos, yPos);     // caller expects container space
> +            mUpdateSurface = nsnull;        // don't have a temp surface
> +            mCurrentImage = i;              // which image to EndUpdate

Can you put your comments above the lines to which they apply here?

@@ +793,5 @@
> +        if(subregion.IsEmpty())
> +            continue;
> +        subregion.MoveBy(-xPos, -yPos); // Tile-local space
> +        // copy tile from temp surface
> +        gfxASurface* surf = mImages[i]->BeginUpdate(subregion);

Can you check if using DirectUpdate on each of the tiles here would be faster?

@@ +840,5 @@
> + * adding or discarding tiles, but do we want this?
> + */
> +void TiledTextureImage::Resize(const nsIntSize& aSize)
> +{
> +    mSize=aSize;

Please don't resize if mSize == aSize. (You'll need to be careful when calling Resize from the constructor, though - make sure mSize isn't set!)

@@ +843,5 @@
> +{
> +    mSize=aSize;
> +    mImages.Clear();
> +    mColumns = (aSize.width  + mTileSize - 1) / mTileSize;
> +    mRows    = (aSize.height + mTileSize - 1) / mTileSize;

Please add a comment above this that says "Calculate number of columns and rows required, rounding up" or something to that effect.

@@ +849,5 @@
> +    for(unsigned int row = 0; row < mRows; row++) {
> +      for(unsigned int col = 0; col < mColumns; col++) {
> +          nsIntSize size( // use tilesize first, then the remainder
> +                  (col+1) * mTileSize > aSize.width  ? aSize.width  % mTileSize : mTileSize,
> +                  (row+1) * mTileSize > aSize.height ? aSize.height % mTileSize : mTileSize);

I personally am not a huge fan of the ternary operator. IMO it's clearer to have a separate calculation:

int tileWidth, tileHeight;
if ((col + 1) * mTileSize > mSize.width)
  tileWidth = mSize.width % mTileSize;
else
  tileWidth = mTileSize;
// etc

However I do not insist upon this.

@@ +850,5 @@
> +      for(unsigned int col = 0; col < mColumns; col++) {
> +          nsIntSize size( // use tilesize first, then the remainder
> +                  (col+1) * mTileSize > aSize.width  ? aSize.width  % mTileSize : mTileSize,
> +                  (row+1) * mTileSize > aSize.height ? aSize.height % mTileSize : mTileSize);
> +          nsRefPtr<TextureImage> i =

Maybe call this "tex" or "elem" rather than "i", because i is an overloaded name.

@@ +852,5 @@
> +                  (col+1) * mTileSize > aSize.width  ? aSize.width  % mTileSize : mTileSize,
> +                  (row+1) * mTileSize > aSize.height ? aSize.height % mTileSize : mTileSize);
> +          nsRefPtr<TextureImage> i =
> +                  mGL->CreateTextureImage(size, mContentType, LOCAL_GL_CLAMP_TO_EDGE, mUseNearestFilter);
> +          mImages.AppendElement(i);

Given how many hoops you need to jump through "emulating" a 2D array with a 1D array, wouldn't it be easier to have an array of arrays (2D array) in this class?

@@ +1344,5 @@
> +    nsIntSize size;
> +    nsIntSize a=aDst->GetSize();
> +    nsIntSize b=aSrc->GetSize();
> +    size.width  = a.width  + b.width;
> +    size.height = a.height > b.height ? a.height : b.height;

NS_MAX(a.height, b.height)

@@ +1347,5 @@
> +    size.width  = a.width  + b.width;
> +    size.height = a.height > b.height ? a.height : b.height;
> +
> +    nsRefPtr<gfxASurface> ds = gfxPlatform::GetPlatform()->
> +        CreateOffscreenSurface(size, gfxASurface::ContentFromFormat(gfxASurface::ImageFormatRGB24));

I don't think this is actually used, is it?
Comment 34 Florian Hänel [:heeen] 2011-05-27 14:36:55 PDT
Just FYI, I noticed this breaks building for Mac OS X, but the fix is pretty straight forward. I also have to check out some building errors I got from this try server run http://tbpl.mozilla.org/?tree=Try&rev=678639cd8911 that may have resulted from this or the other patches in that patchqueue.
Comment 35 Florian Hänel [:heeen] 2011-05-30 06:31:10 PDT
Created attachment 536077 [details] [diff] [review]
fixed nits

> Can you check if using DirectUpdate on each of the tiles here would be faster?
we can only use DirectUpdate when the rect to update == tile rect. should we check if the current update region is exactly the tile size and do DirectUpdate instead? And I'm not sure how to test that it is really faster afterwards. I propose leaving this as a optimization in another bug.
Comment 36 Florian Hänel [:heeen] 2011-05-30 06:33:25 PDT
Try server runs, one is after I fixed the mac issue, the other before.
http://tbpl.mozilla.org/?tree=Try&rev=90efe6c57ae8
http://tbpl.mozilla.org/?tree=Try&rev=96e65f972b97
Comment 37 Oleg Romashin (:romaxa) 2011-05-30 15:01:08 PDT
Created attachment 536183 [details] [diff] [review]
interdiff from previous r+ patch
Comment 38 Dão Gottwald [:dao] 2011-06-04 05:36:05 PDT
Please attach a patch with the author (Florian Hänel? this bug has no assignee) and a commit message added for check-in.
Comment 39 Oleg Romashin (:romaxa) 2011-06-04 08:01:00 PDT
Created attachment 537349 [details] [diff] [review]
same patch with headers... ready to import
Comment 40 Oleg Romashin (:romaxa) 2011-06-04 09:50:33 PDT
I see some weird assertion on Mac builds with this patch:
http://tinderbox.mozilla.org/showlog.cgi?tree=Try&errorparser=unittest&logfile=1307203494.1307204008.14859.gz&buildtime=1307203494&buildname=Rev3%20MacOSX%20Snow%20Leopard%2010.6.2%20try%20debug%20test%20crashtest&fulltext=1#err1
***************
###!!! ASSERTION: Framebuffer not complete!: 'status == LOCAL_GL_FRAMEBUFFER_COMPLETE', file /builds/slave/try-osx64-dbg/build/gfx/thebes/GLContext.cpp, line 2021
mozilla::gl::GLContext::SetBlitFramebufferForDestTexture [gfx/thebes/GLContext.cpp:2023]
mozilla::gl::GLContext::BlitTextureImage [gfx/thebes/GLContext.cpp:1457]
mozilla::layers::BasicBufferOGL::BeginPaint [gfx/layers/opengl/ThebesLayerOGL.cpp:654]
mozilla::layers::ThebesLayerOGL::RenderLayer [gfx/layers/opengl/ThebesLayerOGL.cpp:837]
mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerOGL> [gfx/layers/opengl/ContainerLayerOGL.cpp:246]
mozilla::layers::ContainerLayerOGL::RenderLayer [gfx/layers/opengl/ContainerLayerOGL.cpp:339]
mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerOGL> [gfx/layers/opengl/ContainerLayerOGL.cpp:246]
mozilla::layers::ContainerLayerOGL::RenderLayer [gfx/layers/opengl/ContainerLayerOGL.cpp:339]
mozilla::layers::LayerManagerOGL::Render [gfx/layers/opengl/LayerManagerOGL.cpp:728]
mozilla::layers::LayerManagerOGL::EndTransaction [gfx/layers/opengl/LayerManagerOGL.cpp:427]
nsDisplayList::PaintForFrame [layout/base/nsDisplayList.cpp:630]
nsDisplayList::PaintRoot [layout/base/nsDisplayList.cpp:538]
nsLayoutUtils::PaintFrame [layout/base/nsLayoutUtils.cpp:1640]
PresShell::Paint [layout/base/nsPresShell.cpp:6253]
nsViewManager::RenderViews [view/src/nsViewManager.cpp:444]
nsViewManager::Refresh [view/src/nsViewManager.cpp:420]
nsViewManager::DispatchEvent [view/src/nsViewManager.cpp:922]
HandleEvent [view/src/nsView.cpp:160]
***************
Comment 41 Oleg Romashin (:romaxa) 2011-06-04 10:02:45 PDT
I'm wondering should not we do some changes in GLContextProviderCGL.mm? related to new multitexture implementation?
Comment 42 Oleg Romashin (:romaxa) 2011-06-09 22:15:37 PDT
Created attachment 538437 [details] [diff] [review]
Interdiff Fix for Mac failure + compile warnings
Comment 43 Oleg Romashin (:romaxa) 2011-06-09 23:24:59 PDT
let see if try server happy with updated patch
http://tbpl.mozilla.org/?tree=Try&rev=4c66a5a66215
Comment 44 Oleg Romashin (:romaxa) 2011-06-10 06:59:06 PDT
Created attachment 538503 [details] [diff] [review]
Updated to trunk + mac orange fix + compile warnings combined
Comment 45 Oleg Romashin (:romaxa) 2011-06-11 12:09:24 PDT
Still something is wrong... always getting error in
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/635373-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/635373-2.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/635373-3.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/scrolling/uncovering-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/scrolling/uncovering-2.html | image comparison (==)

on Mac build..
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1307762451.1307764159.4776.gz&fulltext=1#err0

I compiled the same patch, build on my old Mac Mini, and it does not fail, all tests pass without any problems

Any ideas what it could be and how to fix it?
Comment 46 Oleg Romashin (:romaxa) 2011-06-16 14:55:33 PDT
I was testing this patch with GLX backend (gtk build)
and without patch build works fine, with patch it crashes, because mShaderType not initalized in TiledTexture.
even with that stuff fixed I see only artifacts and broken rendering on GLX with this patch
Comment 47 Florian Hänel [:heeen] 2011-06-17 06:36:34 PDT
I did some debugging and found that it is the backingsurface path that makes this fail. It just doesn't work with SurfaceBufferOGL. If you force it into the BasicBuffer Path I at least get gui elements working, but then it crashes like this:

0xb7fe0832 in free () at /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/memory/jemalloc/jemalloc.c:6035
6035            UTRACE(ptr, 0, 0);
(gdb) bt
#0  0xb7fe0832 in free () at /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/memory/jemalloc/jemalloc.c:6035
#1  0xb544e0cf in raise () from /lib/libc.so.6
#2  0xb544f9e7 in abort () from /lib/libc.so.6
#3  0xb7f9eee9 in mozalloc_abort (msg=0xbfffc234 "###!!! ABORT: X_GLXVendorPrivate: GLXBadPixmap; sync; id=0x0: file /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/toolkit/xre/nsX11ErrorHandler.cpp, line 199")
    at /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:76
#4  0xb7f9eeb8 in moz_free (ptr=0xb4330044) at /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/memory/mozalloc/mozalloc.cpp:95
#5  0xb6977d75 in Abort (aMsg=<value optimized out>) at /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/xpcom/base/nsDebugImpl.cpp:388
#6  NS_DebugBreak_P (aMsg=<value optimized out>) at /scratchbox/users/fhae/home/fhae/upstream/mozilla-central/xpcom/base/nsDebugImpl.cpp:375
#7  0xb7eceff4 in ?? () from dist/bin/libxul.so

not sure where to go from there.

Since it is not necessary for GLX or CGL anyways, I say we only use the TiledTextureImage for the EGL case and leave GLX as they are.
The only question is how to cleanly implement this distinction between EGL and GLX. Maybe we should just generate TiledTexImg in ContextProviderEGL::createTextureImage and inside TiledTextureImage create TextureImageEGL?
Comment 48 Joe Drew (not getting mail) 2011-06-21 09:30:48 PDT
Comment on attachment 538503 [details] [diff] [review]
Updated to trunk + mac orange fix + compile warnings combined

I'm just going to clear the review request until we get the EGL <-> non-EGL issues resolved. :)
Comment 49 Oleg Romashin (:romaxa) 2011-06-23 03:03:30 PDT
> Since it is not necessary for GLX or CGL anyways, I say we only use the
> TiledTextureImage for the EGL case and leave GLX as they are.

I think we can do that, but should create some bug about non-EGL TiledImage enabler...
Comment 50 Jeff Muizelaar [:jrmuizel] 2011-06-24 13:31:54 PDT
Comment on attachment 538503 [details] [diff] [review]
Updated to trunk + mac orange fix + compile warnings combined

This needs to be rebased against mozilla-central again because of the layer resolutions changes that happened.
Comment 51 Chris Lord [:cwiiis] 2011-06-28 16:09:08 PDT
Created attachment 542651 [details] [diff] [review]
Updated to trunk again

This is (hopefully) the latest patch, updated to today's mozilla-central. The rebase was quite awkward, so assuming I've not messed it up (it builds and works for me), hopefully this'll save someone some effort.
Comment 52 Oleg Romashin (:romaxa) 2011-06-29 18:22:59 PDT
Created attachment 543024 [details] [diff] [review]
Updated to trunk, fixed GLX compilation

Tested this patch on Qt fennec, Harmattan, works fine
Checked on desktop Firefox GTK/GLX and without patch it works fine, with patch on scroll I see black/not-updated thebes layer content...

Tested desktop Firefox Qt/EGL on device and scrolling works fine, Thebes Layer updated correctly... 
Seems something else is missing in GLX provider... 
btw, GLXprovider is very similar to EGL (not using BasicTextureImage).
Comment 53 Florian Hänel [:heeen] 2011-06-30 04:00:18 PDT
I notice some black areas on harmattan as well, must be the refactored rendering code I think.
Comment 54 Florian Hänel [:heeen] 2011-06-30 04:47:06 PDT
It seems only chrome pages are affected, but it's so rare I can't really reproduce it
Comment 55 Oleg Romashin (:romaxa) 2011-06-30 16:58:25 PDT
Created attachment 543307 [details] [diff] [review]
Fixed GLX rendering (possibly Mac too)

Fixed GLX rendering, by adding visiblerect offset to textureRect when intersecting visible region...
Comment 56 Oleg Romashin (:romaxa) 2011-06-30 17:16:28 PDT
try build is here:
http://tbpl.mozilla.org/?tree=Try&rev=903fa83c53e4
Comment 57 Jeff Muizelaar [:jrmuizel] 2011-07-01 10:38:07 PDT
I get some warnings with this patch:
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.h: In constructor ‘mozilla::gl::TiledTextureImage::TiledTextureImage(mozilla::gl::GLContext*, nsIntSize, gfxASurface::gfxContentType, PRBool)’:
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.h:419: warning: ‘mozilla::gl::TiledTextureImage::mTextureState’ will be initialized after
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.h:409: warning:   ‘bool mozilla::gl::TiledTextureImage::mInUpdate’
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.cpp:707: warning:   when initialized here
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.h:409: warning: ‘mozilla::gl::TiledTextureImage::mInUpdate’ will be initialized after
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.h:407: warning:   ‘unsigned int mozilla::gl::TiledTextureImage::mCurrentImage’
/Users/jrmuizel/source/mozilla-central/gfx/thebes/GLContext.cpp:707: warning:   when initialized here
Comment 58 Oleg Romashin (:romaxa) 2011-07-01 11:05:30 PDT
Another try... don't see any permanent failures
http://tbpl.mozilla.org/?tree=Try&rev=3f543417b3e0
Comment 59 Oleg Romashin (:romaxa) 2011-07-01 11:09:32 PDT
Created attachment 543476 [details] [diff] [review]
Fixed compile warning, init list
Comment 60 Jeff Muizelaar [:jrmuizel] 2011-07-01 11:18:28 PDT
(In reply to comment #55)
> Created attachment 543307 [details] [diff] [review] [review]
> Fixed GLX rendering (possibly Mac too)
> 
> Fixed GLX rendering, by adding visiblerect offset to textureRect when
> intersecting visible region...

This does fix the Mac blackness problem I was seeing.
Comment 61 Chris Lord [:cwiiis] 2011-07-01 17:34:53 PDT
Seems that last breaks updating on some sites.

If I go to engadget.com, you see the site briefly, then it paints over it in white - if you scroll down faster enough, you can see the buffer outside of the visible rect has painted, but then it gets clipped when the buffer is updated.
Comment 62 Chris Lord [:cwiiis] 2011-07-01 18:35:07 PDT
Apologies, seems I get blank Engadget.com regardless of the MoveBy change. Something in this patch has changed and broken that for me, but I don't know what yet.
Comment 63 Chris Lord [:cwiiis] 2011-07-04 03:10:58 PDT
Comment on attachment 543476 [details] [diff] [review]
Fixed compile warning, init list

Review of attachment 543476 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the two comments I've made, looks good and works for me on both desktop and android.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +269,1 @@
>  

Missing "mTexImage->BeginTileIteration();" here

::: gfx/thebes/GLContext.cpp
@@ +740,5 @@
> +    for (unsigned i = 0; i < mImages.Length(); i++) {
> +        unsigned int xPos = (i % mColumns) * mTileSize;
> +        unsigned int yPos = (i / mColumns) * mTileSize;
> +        nsIntRegion tileRegion;
> +        tileRegion.And(aRegion, nsIntRect(nsIntPoint(xPos,yPos), mImages[i]->GetSize())); // intersect with tile

region should be used here instead of aRegion, I assume?
Comment 64 Jeff Muizelaar [:jrmuizel] 2011-07-04 08:29:36 PDT
Comment on attachment 543476 [details] [diff] [review]
Fixed compile warning, init list

The warning fixes look fine.
Comment 65 Florian Hänel [:heeen] 2011-07-04 09:23:27 PDT
Created attachment 543794 [details] [diff] [review]
fixed chris' nits
Comment 66 Jeff Muizelaar [:jrmuizel] 2011-07-05 21:18:10 PDT
Comment on attachment 543794 [details] [diff] [review]
fixed chris' nits

Review of attachment 543794 [details] [diff] [review]:
-----------------------------------------------------------------

Joe gave an oral r+ to this tonight. Provided it passes tests, and doesn't regress performance this should be ready to go.
Comment 67 Chris Lord [:cwiiis] 2011-07-06 01:47:07 PDT
It can always be fixed later, but I've unfortunately noticed that scrolling sub-frames seem to be broken with this patch on android (e.g. the desktop site for Google Reader).
Comment 68 Oleg Romashin (:romaxa) 2011-07-06 01:56:47 PDT
> sub-frames seem to be broken with this patch on android (e.g. the desktop
> site for Google Reader).

does it work without this patch? I'm wondering if anything works at all without this patch and HW accel enabled...
Comment 69 Oleg Romashin (:romaxa) 2011-07-06 01:58:13 PDT
Latest try build for attached patch
http://tbpl.mozilla.org/?tree=Try&rev=d8ad716f2e43
Comment 70 Oleg Romashin (:romaxa) 2011-07-06 02:06:01 PDT
also tested this patch on Nokia device, and frames scrolling works fine
Comment 71 Florian Hänel [:heeen] 2011-07-06 02:15:57 PDT
Chris - can you add the following lines to your $profile/prefs.js file and test without the patch?

user_pref("toolkit.browser.cacheRatioHeight", 1000);
user_pref("toolkit.browser.cacheRatioWidth", 1000);

these are 3000 and 2000 by default (mobile/app/mobile.js). It lower the values until the Texture Images fit within thw 2048 limit. It depends on your screen resolution.

I found some weird draw bugs related to background filling if you scroll around the settings pane, but they were also happening without any patches applied.
Comment 72 Chris Lord [:cwiiis] 2011-07-06 02:35:39 PDT
Sorry, I take my comment back - the problem I was seeing occurs with or without the patch.
Comment 73 Florian Hänel [:heeen] 2011-07-06 05:44:42 PDT
Created attachment 544211 [details] [diff] [review]
rebased yet again
Comment 74 Oleg Romashin (:romaxa) 2011-07-06 14:03:51 PDT
http://hg.mozilla.org/mozilla-central/rev/709b5696199b
Comment 75 neil@parkwaycc.co.uk 2011-11-26 04:12:24 PST
Comment on attachment 544211 [details] [diff] [review]
rebased yet again

>+  namespace layers {
>+    class LayerManagerOGL;
>+    class ColorTextureLayerProgram;
>+  };
My gcc doesn't like this semicolon...

Note You need to log in before you can comment on or make changes to this bug.