Closed
Bug 295737
Opened 19 years ago
Closed 19 years ago
Tiling very tall images is slow
Categories
(Core Graveyard :: GFX: Mac, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: sfraser_bugs, Assigned: sfraser_bugs)
References
()
Details
(Keywords: fixed1.8, perf)
Attachments
(5 files, 2 obsolete files)
1.14 KB,
image/gif
|
Details | |
426 bytes,
text/html
|
Details | |
320 bytes,
image/png
|
Details | |
1.24 KB,
text/html
|
Details | |
11.14 KB,
patch
|
jhpedemonte
:
review+
bryner
:
superreview+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
Many pages have narrow but very tall backgroung images (usually for a color gradation). Our image tiling code seems to perform poorly on these, making page drawing very slow. The URL provides a good example, made worse by the fact that there's a flash plugin that is constantly redrawing, forcing us to keep redrawing the background.
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
Assignee | ||
Comment 3•19 years ago
|
||
Another testcase: http://www.mindvision.com/vise_x.asp
In gfx/src/mac/nsImageMac.cpp, the USE_CGPATTERN_TILING option might work here if we can fix it up. If we can make it work, we can remove the option and make it work standard as we no longer support Mac OS X 10.1.x.
Assignee | ||
Comment 5•19 years ago
|
||
The USE_CGPATTERN_TILING code doesn't work out of the box, alas. We're spending all the time in DrawTileQuickly(), doing the manual tiling with memcpy(). It's so bad for big images because the code currently builds up a tile which is at least the height of the image, even if we're only drawing a small part of it. So, in the testcase, it's tiling 800 x 30,000 pixels.
Assignee | ||
Comment 6•19 years ago
|
||
With the CG tiling code limping, I can see that it's way faster. With the testcase, time taken for one tiling of approx 800x600px: Current code smfr optimized code CGTiling Testcase on bug: 748ms 25ms 11ms Other testcase: 24ms 20ms 4ms so it's well worth the effort to get it going. I just wonder how stable it will be on 10.2.
Assignee: joshmoz → sfraser_bugs
Assignee | ||
Comment 7•19 years ago
|
||
Note to self: test bug 113561 when we're done here.
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•19 years ago
|
||
Assignee | ||
Comment 9•19 years ago
|
||
Assignee | ||
Comment 10•19 years ago
|
||
This patch needs testing on 10.2 and 10.3. See nsRenderingContextMac::DrawTil() for a #define to uncomment to get timing data.
Attachment #191937 -
Flags: review?(jhpedemonte)
Assignee | ||
Comment 11•19 years ago
|
||
This patch fixes a "garbage pixels" issue in Camino caused by a failure to clear the bitmap context.
Assignee | ||
Updated•19 years ago
|
Attachment #191937 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #191937 -
Flags: review?(jhpedemonte)
Assignee | ||
Updated•19 years ago
|
Attachment #191941 -
Flags: review?(jhpedemonte)
Comment 12•19 years ago
|
||
Comment on attachment 191941 [details] [diff] [review] Quartz image tiling patch v. 1.1 + PRUint32 tiledCols = (aTileRect.width + aSXOffset + mWidth - 1) / mWidth; + PRUint32 tiledRows = (aTileRect.height + aSYOffset + mHeight - 1) / mHeight; + + // The tiling that we do below can be expensive for large background + // images, for which we have few tiles. Plus, the SlowTile call eventually + // calls the Quartz drawing functions, which can take advantage of hardware + // acceleration. So if we have few tiles, we just call SlowTile. + const PRUint32 kTilingCopyThreshold = 16; + if (tiledCols * tiledRows < kTilingCopyThreshold) { + return SlowTile(aContext, aSurface, aSXOffset, aSYOffset, 0, 0, aTileRect); + } + This code is also in |DrawTileQuickly()|, but with a threshold of 64. We should probably only do it in one place. + CGrafPtr destPort; + rv = surface->GetGrafPtr(&destPort); + Rect portRect; + ::GetPortBounds(destPort, &portRect); Are these used anywhere? + ::CGImageRelease(tiledImage); + PR_Free(bitmap); + } The |PR_Free(bitmap)| should be outside that block. Nit: For consistency, make all of the CG calls start with "::". Nit: Please try to keep lines to 80 chars or less. Good job on getting this to work. Works great in my testing. With the above fixes, r=jhpedemonte.
Attachment #191941 -
Flags: review?(jhpedemonte) → review+
Assignee | ||
Comment 13•19 years ago
|
||
> Good job on getting this to work. Works great in my testing. With the above
> fixes, r=jhpedemonte.
Thanks for the review! Did you test on 10.2?
I'm trying to track down why I have to do the extra blit in Camino; posting on
the Quartz list.
Assignee | ||
Comment 14•19 years ago
|
||
OK, I figured out why it didn't work in Camino (fixed by adding call to CGContextSetPatternPhase()). So now we're fast everywhere! I moved the Quartz tiling code into a new method for clarify, and tidied up the #ifdefs a bit. The new method is still built if you turn off USE_CGPATTERN_TILING, but I think that's fine (and keeps the code building). I also remove the SlowTile() fallback from DrawTileQuickly(), but #ifdeffed the threshold lower for Quartz tiling. This patch is a huge win. On the first testcase, the time taken to tile goes from about 740ms to 6ms on my machine.
Attachment #191941 -
Attachment is obsolete: true
Attachment #191996 -
Flags: review?(jhpedemonte)
Comment 15•19 years ago
|
||
> Thanks for the review! Did you test on 10.2?
No, but I was able to test on 10.3 (with patch v1.1), and noticed no issues.
Don't have 10.2.
Comment 16•19 years ago
|
||
Comment on attachment 191996 [details] [diff] [review] Better patch + const PRUint32 kTilingCopyThreshold = 16; // CG tiling is so much more efficient With CG being so efficient, is there any case where |SlowTile()| will be faster? The only one I can think of is when the background image is bigger than the window. Have you done any performance testing in this regard? The two nits from my previous comments still apply. With that fixed, r=me.
Attachment #191996 -
Flags: review?(jhpedemonte) → review+
Assignee | ||
Comment 17•19 years ago
|
||
(In reply to comment #16) > (From update of attachment 191996 [details] [diff] [review] [edit]) > + const PRUint32 kTilingCopyThreshold = 16; // CG tiling is so much more > efficient > > With CG being so efficient, is there any case where |SlowTile()| will be > faster? The only one I can think of is when the background image is bigger > than the window. Have you done any performance testing in this regard? Yes, I did some measuring here. FF often draws a single tile (e.g. for the tab widget, and the search bar background), and SlowTile was faster than doing Quartz tiling. I arrived at that 16-tile threshold empirically. > The two nits from my previous comments still apply. Will clean up on checkin.
Assignee | ||
Comment 18•19 years ago
|
||
Test camino build at http://www.smfr.org/downloads/camino/Camino_20050808_quartz_tiling.zip
Assignee | ||
Updated•19 years ago
|
Attachment #191996 -
Flags: superreview?(bryner)
Comment 19•19 years ago
|
||
Comment on attachment 191996 [details] [diff] [review] Better patch >--- nsImageMac.cpp 23 Jun 2005 00:59:00 -0000 1.79 >+++ nsImageMac.cpp 8 Aug 2005 19:19:34 -0000 >+// MacOSX 10.2 supports CGPattern, which does image/pattern tiling for us, and >+// is much faster than doing it ourselves. >+#define USE_CGPATTERN_TILING > Why bother having an #ifdef at all? Is there a reason someone would want to turn this off? Looks good to me otherwise, sr=bryner.
Attachment #191996 -
Flags: superreview?(bryner) → superreview+
Comment 20•19 years ago
|
||
If this patch might land for FF 1.5, I suggest we leave the ifdefs in so we can easily revert the bahavior if we need to because of regressions.
Assignee | ||
Comment 21•19 years ago
|
||
I'm fine with removing the pre-CGTiling code; we have CVS after all. Javier?
Assignee | ||
Comment 22•19 years ago
|
||
Comment on attachment 191996 [details] [diff] [review] Better patch Seeking approval on this big perf win. Has been tested on 10.2 -> 10.4.
Attachment #191996 -
Flags: approval1.8b4?
Comment 23•19 years ago
|
||
I'm fine with removing the #ifdefs. The only reason they were in there to begin with was that I had written the CG tiling code, but never got it working fully. Now that it's working, no reason to keep it around.
Blocks: 299569
Assignee | ||
Comment 24•19 years ago
|
||
Ping for 1.8b4 approval (since we want this for camino 1.0).
Updated•19 years ago
|
Attachment #191996 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 25•19 years ago
|
||
Checked in on branch and trunk, with the #ifdefs. We can remove unused code once we're sure this is regression-free.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 27•18 years ago
|
||
Boris was referring to bug 313062 :)
Updated•15 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•