Note: There are a few cases of duplicates in user autocompletion which are being worked on.

remove unused and broken tiling code

RESOLVED FIXED in Firefox 14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 affected, firefox14 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

14.46 KB, patch
cjones
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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) {
(Assignee)

Comment 1

5 years ago
I think this was introduced with bug 628566. I can't find a test so I am not sure this ever worked.
Blocks: 628566
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

5 years ago
I have a pretty easy way to reproduce this, happy to test a patch.
(Assignee)

Updated

5 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

5 years ago
Summary: ImageLayerOGL tiling code looks broken → ImageLayerOGL tiling code uses initinitialized mTileSourceRect (mUseTileSourceRect is false)
(Assignee)

Comment 3

5 years ago
Created attachment 606852 [details] [diff] [review]
patch
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #606852 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Summary: ImageLayerOGL tiling code uses initinitialized mTileSourceRect (mUseTileSourceRect is false) → remove unused and broken tiling code
(Assignee)

Comment 5

5 years ago
Created attachment 606904 [details] [diff] [review]
patch
Assignee: nobody → gal
(Assignee)

Updated

5 years ago
Attachment #606852 - Attachment is obsolete: true
Attachment #606852 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Attachment #606904 - Flags: review?(jones.chris.g)
(Assignee)

Comment 6

5 years ago
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]
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])
Attachment #606904 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8191d6cd869d
(Assignee)

Comment 9

5 years ago
Fixed the bad style nit fix:

http://hg.mozilla.org/integration/mozilla-inbound/rev/f797fa3447ac
(Assignee)

Comment 10

5 years ago
Backed out. Time to try server this.

http://hg.mozilla.org/integration/mozilla-inbound/rev/62e6af03fa55

Comment 11

5 years ago
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

5 years ago
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
(Assignee)

Comment 13

5 years ago
Forth time is the charm

http://hg.mozilla.org/integration/mozilla-inbound/rev/2fb48dfe4959
https://hg.mozilla.org/mozilla-central/rev/8191d6cd869d
https://hg.mozilla.org/mozilla-central/rev/f797fa3447ac
https://hg.mozilla.org/mozilla-central/rev/62e6af03fa55
https://hg.mozilla.org/mozilla-central/rev/2fb48dfe4959
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 738158
status-firefox13: --- → affected
status-firefox14: --- → fixed
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.