Closed Bug 295737 Opened 19 years ago Closed 19 years ago

Tiling very tall images is slow

Categories

(Core Graveyard :: GFX: Mac, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

References

()

Details

(Keywords: fixed1.8, perf)

Attachments

(5 files, 2 obsolete files)

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.
Attached image Image for testcase
Attached file Testcaes
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.
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.
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
Note to self: test bug 113561 when we're done here.
Status: NEW → ASSIGNED
Attached image Image for next testcase
Attached file Image tiling testcase
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)
Attached patch Quartz image tiling patch v. 1.1 (obsolete) — Splinter Review
This patch fixes a "garbage pixels" issue in Camino caused by a failure to
clear the bitmap context.
Attachment #191937 - Attachment is obsolete: true
Attachment #191937 - Flags: review?(jhpedemonte)
Attachment #191941 - Flags: review?(jhpedemonte)
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+
> 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.
Attached patch Better patchSplinter Review
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)
> 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 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+
(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.
Attachment #191996 - Flags: superreview?(bryner)
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+
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.
I'm fine with removing the pre-CGTiling code; we have CVS after all. Javier?
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?
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: 303464
Ping for 1.8b4 approval (since we want this for camino 1.0).
Attachment #191996 - Flags: approval1.8b4? → approval1.8b4+
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
Keywords: fixed1.8
This apparently caused bug 
Depends on: 313062
Boris was referring to bug 313062 :)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: