628566 - Implement basic tiling of ImageLayerOGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Fennec won't start up with GL compositing unless this is fixed or the checkerboard background is disabled.

Currently we can assume we're only tiling images with power-of-two dimensions, so a simple implementation of the tiling API using GL_REPEAT can be done.  For this bug we probably want to start saving the checkerboard layer created by bug 593310 so that we don't churn texture memory unnecessarily.

Comment 2

[Florian Hänel [:heeen]]

Is there a problem with GL_REPEATing NPOT Textures? I'd assume most hardware could handle that.

I'd say for size==tileSrcRect, just draw one big quad with gl_repeat, this would be the most common fast path. 

Then there's srcRect != tileRect:

* We could implement this in the fragment shader, I'm thinking of a very naive implementation ala sample2D(image, vec2(pos.x % rect.width + rect.height... - trading rendering speed against memory savings
* Render several quads for each repetition.
* prepare a temporary texture that tiles correctly trading memory for rendering speed.

Do we even support the tileSrcRect being bigger than the image that is supposed to tile?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #2)
> Is there a problem with GL_REPEATing NPOT Textures? I'd assume most hardware
> could handle that.

Yes, most can, but some HW can't, so we can't rely on it.  See http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#55 .

> Do we even support the tileSrcRect being bigger than the image that is supposed
> to tile?

See the comments: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#658 .  "No".

Comment 4

[Florian Hänel [:heeen]]

I've been hacking at this and have it somewhat working, but I'm kind of confused by what it is supposed to look like - I expected the checkerboard to behave like the content basically - scale and pan as a placeholder to give the user a sense of proportion.
It seems it doesn't scale at all and panning is kind of odd.

Comment 5

[Florian Hänel [:heeen]]

Another thing: The checkerboard layer seems to be recreated on every pan or zoom change, this seems kind of wasteful to me. Wouldn't it be better to upload the checkerboard once and then manipulate the image layer in-place?

Comment 6

[Florian Hänel [:heeen]]

Attachment: work in progress

Here's a work in progress for this I'd like to get some input on. I had to change the format to argb since it wouldn't show up on the first try. I have to investigate this later.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> I've been hacking at this and have it somewhat working, but I'm kind of
> confused by what it is supposed to look like - I expected the checkerboard to
> behave like the content basically - scale and pan as a placeholder to give the
> user a sense of proportion.
> It seems it doesn't scale at all and panning is kind of odd.

You can see what it's supposed to look like by disabling GL.

(In reply to comment #5)
> Another thing: The checkerboard layer seems to be recreated on every pan or
> zoom change, this seems kind of wasteful to me. Wouldn't it be better to upload
> the checkerboard once and then manipulate the image layer in-place?

For GL, yeah probably, see comment 0.  This doesn't really matter with basic layers.

Comment 8

[Florian Hänel [:heeen]]

Attachment: WIP - added support for npot textures and tilesrcrect

I added support for tiling subrects and gpus that don't tile npot textures.

Comment 9

[Florian Hänel [:heeen]]

Attachment: wip - fixes some unpainted areas when panning

Comment 10

[Florian Hänel [:heeen]]

Attachment: ready for first round of review

I think this is ready for a first round of review. I had to change the checkerboard to ARGB, since my platform doesn't like that we first allocate the texture with ARGB in InitTexture and then do TexSubImage with RGB565 in UploadSurface. I think that should be addressed in a new bug.

Anyways, you can play with different checkerboard sizes with the code provided.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 518345 [details] [diff] [review]
ready for first round of review

Hi Florian, it would be better for someone who knows GL to review this ;).  Not sure who is more appropriate.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 518345 [details] [diff] [review]
ready for first round of review

Couple of more general comments:

Is there any reason you're drawing triangles rather than quads? Quads would require fewer vertices.

Be sure to always put a space after open-comments //, between operators a = b + c, between if ( and its opening parens, etc.

Line up arguments on new lines:

doSomeStuff(someVeryLongArgumentListThatGoesToTheNextLine,
            anotherArgumentForRealzGuys, etc);

>diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp

>-    program->SetLayerQuadRect(nsIntRect(0, 0,
>-                                        cairoImage->mSize.width,
>-                                        cairoImage->mSize.height));
>+    // set a identity transform, we use pixel coordinates below
>+    program->SetLayerQuadRect(nsIntRect(0, 0, 1, 1));

Can you explain how this unit square gets translated into the final image size?

>     program->SetLayerTransform(GetEffectiveTransform());
>     program->SetLayerOpacity(GetEffectiveOpacity());
>     program->SetRenderOffset(aOffset);
>     program->SetTextureUnit(0);
> 
>-    mOGLManager->BindAndDrawQuad(program);
>+    std::vector<vert_coord> verts(6);
>+    std::vector<tex_coord> texc(6);

Unfortunately using std::vector is a no-no in Mozilla code. I suggest using nsTArray.

>+    nsIntRect rect = GetVisibleRegion().GetBounds();
>+
>+    bool tileIsWholeImage = mTileSourceRect == nsIntRect(0,0,iwidth,iheight);
>+    bool imageIsPowerOfTwo = ((iwidth & (iwidth - 1)) == 0 && (iheight & (iheight - 1)) == 0);
>+    bool canDoNPOT =
>+          (gl()->IsExtensionSupported(GLContext::ARB_texture_non_power_of_two) ||
>+          gl()->IsExtensionSupported(GLContext::OES_texture_npot));

nit: line up the gl() calls vertically

>+        // we need to anchor the repeating texture approtiately

nit: "appropriately"

>+        //round rect position down to multiples if texture size

nit: "of" texture size.

>diff --git a/gfx/layers/opengl/LayerManagerOGLShaders.txt b/gfx/layers/opengl/LayerManagerOGLShaders.txt

>+varying mediump vec2 vTexCoord;

>+varying mediump vec2 vTexCoord;

Is mediump supported on non-ES GL? Also, I thought we put a #pragma above all our shaders specifying medium precision.

Basically: "Why is this necessary?"

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp
>@@ -53,16 +53,19 @@ typedef nsContentView::ViewConfig ViewCo
> using namespace mozilla::layers;
> 
> namespace mozilla {
> namespace layout {
> 
> typedef FrameMetrics::ViewID ViewID;
> typedef RenderFrameParent::ViewMap ViewMap;
> 
>+// RefPtr doesn't seem to make sense for statics
>+mozilla::layers::ImageContainer* sCheckerboard=0;

= nsnull

I'm pretty sure that you're going to run into leak detection problems here when this lands on mozilla-central. You're going to have to clean it up on xpcom shutdown. (It might be a good idea to just use an nsRefPtr anyways.)
 
> already_AddRefed<gfxASurface>
> GetBackgroundImage()

>+  const unsigned int size = 32;
>+  const unsigned int checkersize = 16;
>+  static unsigned int data[size*size];
>+  static bool initialized = false;
>+  if(!initialized) {
>+    initialized=true;
>+    for(unsigned int y = 0; y < size; y++)
>+      for(unsigned int x = 0; x < size; x++) {
>+        if(((x / checkersize) & 1) xor ((y / checkersize) & 1))

Do MSVC or other compilers handle specifying logical xor this way? Might be better to use ^^

Also, I think size is going to have to be a #define - unless we can rely on the ability to use non-literal constants for array sizes.

Finally, I wonder if there's a more straightforward way of writing this condition.

Comment 13

[Matt Woodrow (:mattwoodrow)]

> 
> Unfortunately using std::vector is a no-no in Mozilla code. I suggest using
> nsTArray.
> 

We have GLContext::RectTriangles for handling sets of vertex/texture coordinates already. I'd prefer if we use this.

Comment 14

[Florian Hänel [:heeen]]

Attachment: fixed nits

Comment 15

[Joe Drew (not getting mail)]

Comment on attachment 521517 [details] [diff] [review]
fixed nits

Overall, looking good. A few more changes and I think this will be ready to go.

A friendly reminded from my earlier general comments. There are still a number of examples of these.

Be sure to always put a space after open-comments //, between operators a = b + c, between if ( and its opening parens, etc.

Line up arguments on new lines:

doSomeStuff(someVeryLongArgumentListThatGoesToTheNextLine,
            anotherArgumentForRealzGuys, etc);

>diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp

>@@ -436,34 +436,121 @@ ImageLayerOGL::RenderLayer(int,

>+        //round out size to acomodate

accommodate

And this should probably specify more details about how & why we're rounding below.

>+        rect.width = (rect.width / iwidth + 2) * iwidth;
>+        rect.height = (rect.height / iheight + 2) * iheight;


>+        // tesselate the visible region with tiles of subrect size

>+                //when we already tesselate, we might as well save on overdraw here

tessellate

>+    gl()->fVertexAttribPointer(vertAttribIndex, 2,
>+            LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0,
>+            &triangleBuffer.vertexCoords[0].x);
>+
>+    gl()->fVertexAttribPointer(texCoordAttribIndex, 2,
>+            LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0,
>+             &triangleBuffer.texCoords[0].u);

Can you explain why you're uploading 2 vertex attribs starting at x (and u)? 

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp

>+// RefPtr doesn't seem to make sense for statics
>+nsRefPtr<ImageContainer> sCheckerboard = nsnull;

This checkerboard will still need to be deallocated at XPCOM shutdown time. (With an nsRefPtr, you can deallocate just by setting to nsnull.) See https://wiki.mozilla.org/XPCOM_Shutdown for some more information on this. Specifically, you want to register an observer for the "xpcom-shutdown" notification topic: https://developer.mozilla.org/en/Observer_Notifications

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 521517 [details] [diff] [review]
fixed nits


> 
>+    cairoImage->SetTiling(mUseTileSourceRect);

Need to call MakeCurrent after this as the tiling call may change the current context.


>+void CairoImageOGL::SetTiling(bool aTiling)
>+{
>+  if(aTiling == mTiling)
>+      return;
>+  mozilla::gl::GLContext *gl = mTexture.GetGLContext();
>+  gl->fActiveTexture(LOCAL_GL_TEXTURE0);
>+  gl->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture.GetTextureID());
>+  mTiling=aTiling;
>+
>+  if(aTiling) {
>+    gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_REPEAT);
>+    gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_REPEAT);
>+  } else {
>+    gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE);
>+    gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE);
>+  }
>+}

Need to call MakeCurrent here too as the gl context of the texture will be different to the one use for rendering.
Obviously this pair of changes isn't technically needed as you can set the wrap modes with any context, but it gets caught by our gl debug system.


Running with MOZ_GL_DEBUG in your environment will abort if a non-current GLContext* is used.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 521517 [details] [diff] [review]
fixed nits


>+
>+    bool tileIsWholeImage = mTileSourceRect == nsIntRect(0,0,iwidth,iheight);

Shouldn't this be:

bool tileIsWholeImage = mTileSourceRect == nsIntRect(0,0,iwidth,iheight) || !mUseTileSourceRect;

Otherwise when we don't have a tiled image (and mTileSourceRect is 0,0,0,0), we hit the tesselation path with theight == 0 and infinite loop.

Comment 18

[Florian Hänel [:heeen]]

Attachment: fixed more nits

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: stack trace leading to GL_INVALID_FRAMEBUFFER_OPERATION

When I run with this patch on a Nexus S, I get the "Framebuffer not complete!" assertion in SetBlitFramebufferForDestTexture, and the next glDrawArrays call generates GL_INVALID_FRAMEBUFFER_OPERATION, see attached backtrace.

Comment 21

[Joe Drew (not getting mail)]

(In reply to comment #15)
> >+    gl()->fVertexAttribPointer(vertAttribIndex, 2,
> >+            LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0,
> >+            &triangleBuffer.vertexCoords[0].x);
> >+
> >+    gl()->fVertexAttribPointer(texCoordAttribIndex, 2,
> >+            LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0,
> >+             &triangleBuffer.texCoords[0].u);
> 
> Can you explain why you're uploading 2 vertex attribs starting at x (and u)? 

I'd still like an answer to this :)

Comment 22

[Joe Drew (not getting mail)]

(In reply to comment #21)
> I'd still like an answer to this :)

Belay that! Matt answered - we're saying there's a set of attributes of size 2 starting at that pointer, and because of the way it's laid out in memory we'll just keep reading from there.

One thing that both he & I would like, for code cleanliness, is to have convenience functions on the triangle buffer class that return those pointers. Also, another convenience function to say how many elements it contains.

Matt also expressed interest in some way of not hard-coding 2, but I'm not as adamant about that.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 526687 [details] [diff] [review]
fixed more nits

Review of attachment 526687 [details] [diff] [review]:

::: gfx/thebes/GLContext.h
@@ +813,5 @@
+        typedef struct { GLfloat u,v; } tex_coord;
+
+        // default is 4 rectangles, each made up of 2 triangles (3 coord vertices each)
+        nsAutoTArray<vert_coord, 6> vertexCoords;
+        nsAutoTArray<tex_coord, 6>  texCoords;

These should probably be private too :)

Comment 24

[Joe Drew (not getting mail)]

Comment on attachment 526687 [details] [diff] [review]
fixed more nits

Review of attachment 526687 [details] [diff] [review]:

I'm quite happy with this. You still need to address checkerboard deallocation and the review comments in comment 22 and comment 23, but this is very close.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +548,5 @@
+            &triangleBuffer.vertexCoords[0].x);
+
+    gl()->fVertexAttribPointer(texCoordAttribIndex, 2,
+            LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0,
+             &triangleBuffer.texCoords[0].u);

These fVertexAttribPointer calls should have their arguments indented until 1 column past (.

::: layout/ipc/RenderFrameParent.cpp
@@ +59,5 @@
 typedef RenderFrameParent::ViewMap ViewMap;
 
+// RefPtr doesn't seem to make sense for statics
+nsRefPtr<ImageContainer> sCheckerboard = nsnull;
+

The checkerboard allocation/deallocation is the only thing in my mind that stops us from being able to commit this patch as-is. As you note, I was wrong - this doesn't *need* to be an nsRefPtr, though it does make things a little simpler.

@@ +404,5 @@
+    for (unsigned int y = 0; y < BOARDSIZE; y++) {
+      for (unsigned int x = 0; x < BOARDSIZE; x++) {
+        bool col_odd = (x / CHECKERSIZE) & 1;
+        bool row_odd = (y / CHECKERSIZE) & 1;
+        if (col_odd ^ row_odd) { //xor

Do you want logical or bitwise xor here? I guess they're the same considering you're operating on a single-bit thing, but I think you want to use ^^ instead.

@@ +484,5 @@
+    nsRefPtr<Image> img = sCheckerboard->CreateImage(fmts, 1);
+    CairoImage::Data data = { bgImage.get(), bgImageSize };
+    static_cast<CairoImage*>(img.get())->SetData(data);
+    sCheckerboard->SetCurrentImage(img);
+  }

In here, we should also add an event listener for xpcom-shutdown. See the end of comment 15 for more details.

Comment 25

[Florian Hänel [:heeen]]

Attachment: fixed nits

fixed nits. There's no such thing as a logical xor operator.

Comment 26

[Joe Drew (not getting mail)]

(In reply to comment #25)
> There's no such thing as a logical xor operator.

Turns out I'm just stupid! :)

Comment 27

[Joe Drew (not getting mail)]

Comment on attachment 530563 [details] [diff] [review]
fixed nits

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

Looks great. Let's get this in!

Some very small nits, below, should be addressed before pushing it though. (I don't need to review this patch again before pushing, though!)

::: layout/ipc/RenderFrameParent.cpp
@@ +57,5 @@
>  
>  typedef FrameMetrics::ViewID ViewID;
>  typedef RenderFrameParent::ViewMap ViewMap;
>  
> +// RefPtr doesn't seem to make sense for statics

You should probably remove this comment.

@@ +59,5 @@
>  typedef RenderFrameParent::ViewMap ViewMap;
>  
> +// RefPtr doesn't seem to make sense for statics
> +nsRefPtr<ImageContainer> sCheckerboard = nsnull;
> +CheckerBoardPatternDeleter sCheckerBoardCleanup;

Because the observer service holds a strong reference to its observers, you don't need an instance of this to be stored here at all. (See below.)

@@ +69,5 @@
> +                                    const char* aTopic,
> +                                    const PRUnichar* aData)
> +{
> +  if (!strcmp(aTopic, "xpcom-shutdown")) {
> +    sCheckerboard = 0;

This should be nsnull instead of 0.

@@ +503,5 @@
> +      mozilla::services::GetObserverService();
> +    if (!observerService) {
> +      return;
> +    }
> +    nsresult rv = observerService->AddObserver(&sCheckerBoardCleanup, "xpcom-shutdown", PR_FALSE);

What you can do here is say 

  nsCOMPtr<nsISupports> cleanup = new CheckerBoardPatternDeleter();

then pass cleanup to the observer service. Since the observer service will hold on to the reference until it doesn't need to anymore, you don't need to keep track of the deleter at all.

::: layout/ipc/RenderFrameParent.h
@@ +122,5 @@
> +{
> +public:
> +  NS_DECL_NSIOBSERVER
> +  NS_DECL_ISUPPORTS
> +};

This (and the #include of nsIObserver.h) should be moved to the cpp file, since we don't need to refer to it elsewhere.

Comment 28

[Florian Hänel [:heeen]]

Attachment: fixed last nits

ready for check in I suppose

Comment 29

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/7642608ac4a5