Closed Bug 736716 Opened 9 years ago Closed 9 years ago

remove unused and broken tiling code


(Core :: Graphics, defect)

Not set



Tracking Status
firefox13 --- affected
firefox14 --- fixed


(Reporter: gal, Assigned: gal)




(1 file, 1 obsolete file)

The Nexus S doesn't support NPOT textures and we are trying to use tiling, however, I don't think mTileSourceRect is ever initialized.

As a result twidth is 0 here:

        unsigned int twidth = mTileSourceRect.width;
        unsigned int theight = mTileSourceRect.height;

And as a result, this loop here is going to take a _really_ long time:

        // tesselate the visible region with tiles of subrect size                                                                                    
        for (int y = rect.y; y < rect.y + rect.height;  y += theight) {
            for (int x = rect.x; x < rect.x + rect.width; x += twidth) {
I think this was introduced with bug 628566. I can't find a test so I am not sure this ever worked.
Blocks: 628566
OS: Mac OS X → All
Hardware: x86 → All
I have a pretty easy way to reproduce this, happy to test a patch.
tracking-fennec: --- → ?
Summary: ImageLayerOGL tiling code looks broken → ImageLayerOGL tiling code uses initinitialized mTileSourceRect (mUseTileSourceRect is false)
Attached patch patch (obsolete) — Splinter Review
This fixes my test case, but video and webgl are still broken. I assume that has both to do with no NPOT texture support as well but in a different place. If you grant review, can you please also land the patch for me? If there is a better fix, please feel to steal. I don't know this code at all.
Attachment #606852 - Flags: review?(joe)
Summary: ImageLayerOGL tiling code uses initinitialized mTileSourceRect (mUseTileSourceRect is false) → remove unused and broken tiling code
Attached patch patchSplinter Review
Assignee: nobody → gal
Attachment #606852 - Attachment is obsolete: true
Attachment #606852 - Flags: review?(joe)
Attachment #606904 - Flags: review?(jones.chris.g)
A little explanation: looks like we don't actually use GL_REPEAT anywhere any more, and GLES 2.0 guarantees that we support NPOT textures as long we are in clamp and not GL_REPEAT mode, so we can whole sale nuke this code.
Comment on attachment 606904 [details] [diff] [review]

A little more explanantion: I added this code to support lightspeed
checkerboard drawing for GL (eventually, I only added basic layer
support).  We removed that in XUL fennec in favor of drawing the CSS
background color.  So this code is unused now.

This would be a really nice optimization for background-repeat, e.g.,
but we can re-add something similar then, when we have the next real
use case in mind.

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

>+    // we need to anchor the repeating texture appropriately
>+    // otherwise it will start from the region border instead
>+    // of the layer origin. This is the offset into the texture
>+    // that the region border represents

This comment is confusing.  Remove references to repeating.

>+    float tex_offset_u = (float)(rect.x % iwidth) / iwidth;
>+    float tex_offset_v = (float)(rect.y % iheight) / iheight;


>+    triangleBuffer.addRect(rect.x, rect.y,
>+                           rect.x + rect.width, rect.y + rect.height,
>+                           tex_offset_u, tex_offset_v,
>+                           tex_offset_u + (float)rect.width / (float)iwidth,
>+                           tex_offset_v + (float)rect.height / (float)iheight);

Attachment #606904 - Flags: review?(jones.chris.g) → review+
Backed out. Time to try server this.
Try run for d7bb632f74fc is complete.
Detailed breakdown of the results available here:
Results (out of 8 total builds):
    failure: 8
Builds (or logs if builds failed) available at:
Try run for d7da069415da is complete.
Detailed breakdown of the results available here:
Results (out of 135 total builds):
    success: 111
    warnings: 17
    failure: 7
Builds (or logs if builds failed) available at:
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.