Closed
Bug 736716
Opened 13 years ago
Closed 13 years ago
remove unused and broken tiling code
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: gal, Assigned: gal)
References
Details
Attachments
(1 file, 1 obsolete file)
14.46 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 2•13 years ago
|
||
I have a pretty easy way to reproduce this, happy to test a patch.
Assignee | ||
Updated•13 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Updated•13 years ago
|
Summary: ImageLayerOGL tiling code looks broken → ImageLayerOGL tiling code uses initinitialized mTileSourceRect (mUseTileSourceRect is false)
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Comment 4•13 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•13 years ago
|
Attachment #606852 -
Flags: review?(joe)
Assignee | ||
Updated•13 years ago
|
Summary: ImageLayerOGL tiling code uses initinitialized mTileSourceRect (mUseTileSourceRect is false) → remove unused and broken tiling code
Assignee | ||
Updated•13 years ago
|
Attachment #606852 -
Attachment is obsolete: true
Attachment #606852 -
Flags: review?(joe)
Assignee | ||
Updated•13 years ago
|
Attachment #606904 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 6•13 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•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/8191d6cd869d
Assignee | ||
Comment 9•13 years ago
|
||
Fixed the bad style nit fix: http://hg.mozilla.org/integration/mozilla-inbound/rev/f797fa3447ac
Assignee | ||
Comment 10•13 years ago
|
||
Backed out. Time to try server this. http://hg.mozilla.org/integration/mozilla-inbound/rev/62e6af03fa55
Comment 11•13 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•13 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•13 years ago
|
||
Forth time is the charm http://hg.mozilla.org/integration/mozilla-inbound/rev/2fb48dfe4959
Comment 14•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Updated•13 years ago
|
status-firefox13:
--- → affected
status-firefox14:
--- → fixed
Updated•11 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•