Last Comment Bug 736716 - remove unused and broken tiling code
: remove unused and broken tiling code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on: 738158
Blocks: 628566
  Show dependency treegraph
 
Reported: 2012-03-17 03:45 PDT by Andreas Gal :gal
Modified: 2013-12-10 10:00 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed


Attachments
patch (1.46 KB, patch)
2012-03-17 04:02 PDT, Andreas Gal :gal
no flags Details | Diff | Review
patch (14.46 KB, patch)
2012-03-17 14:42 PDT, Andreas Gal :gal
cjones.bugs: review+
Details | Diff | Review

Description Andreas Gal :gal 2012-03-17 03:45:02 PDT
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) {
Comment 1 Andreas Gal :gal 2012-03-17 03:45:49 PDT
I think this was introduced with bug 628566. I can't find a test so I am not sure this ever worked.
Comment 2 Andreas Gal :gal 2012-03-17 03:46:43 PDT
I have a pretty easy way to reproduce this, happy to test a patch.
Comment 3 Andreas Gal :gal 2012-03-17 04:02:32 PDT
Created attachment 606852 [details] [diff] [review]
patch
Comment 4 Andreas Gal :gal 2012-03-17 04:03:45 PDT
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.
Comment 5 Andreas Gal :gal 2012-03-17 14:42:55 PDT
Created attachment 606904 [details] [diff] [review]
patch
Comment 6 Andreas Gal :gal 2012-03-17 14:53:02 PDT
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 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-17 14:57:53 PDT
Comment on attachment 606904 [details] [diff] [review]
patch

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;

float([blah])

>+    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);

float([blah])
Comment 9 Andreas Gal :gal 2012-03-17 15:07:47 PDT
Fixed the bad style nit fix:

http://hg.mozilla.org/integration/mozilla-inbound/rev/f797fa3447ac
Comment 10 Andreas Gal :gal 2012-03-17 15:21:03 PDT
Backed out. Time to try server this.

http://hg.mozilla.org/integration/mozilla-inbound/rev/62e6af03fa55
Comment 11 Mozilla RelEng Bot 2012-03-17 16:47:17 PDT
Try run for d7bb632f74fc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d7bb632f74fc
Results (out of 8 total builds):
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/agal@mozilla.com-d7bb632f74fc
Comment 12 Mozilla RelEng Bot 2012-03-17 20:46:49 PDT
Try run for d7da069415da is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d7da069415da
Results (out of 135 total builds):
    success: 111
    warnings: 17
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/agal@mozilla.com-d7da069415da
Comment 13 Andreas Gal :gal 2012-03-18 03:17:34 PDT
Forth time is the charm

http://hg.mozilla.org/integration/mozilla-inbound/rev/2fb48dfe4959

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