295737 - Tiling very tall images is slow

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[Simon Fraser [no longer active]]

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.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Image for testcase

Comment 2

[Simon Fraser [no longer active]]

Attachment: Testcaes

Comment 3

[Simon Fraser [no longer active]]

Another testcase: http://www.mindvision.com/vise_x.asp

Comment 4

[Josh Aas]

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.

Comment 5

[Simon Fraser [no longer active]]

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.

Comment 6

[Simon Fraser [no longer active]]

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.

Comment 7

[Simon Fraser [no longer active]]

Note to self: test bug 113561 when we're done here.

Comment 8

[Simon Fraser [no longer active]]

Attachment: Image for next testcase

Comment 9

[Simon Fraser [no longer active]]

Attachment: Image tiling testcase

Comment 10

[Simon Fraser [no longer active]]

Attachment: Patch to fix up Quartz image tiling

This patch needs testing on 10.2 and 10.3.

See nsRenderingContextMac::DrawTil() for a #define to uncomment to get timing
data.

Comment 11

[Simon Fraser [no longer active]]

Attachment: Quartz image tiling patch v. 1.1

This patch fixes a "garbage pixels" issue in Camino caused by a failure to
clear the bitmap context.

Comment 12

[jhp (no longer active)]

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.

Comment 13

[Simon Fraser [no longer active]]

> 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.

Comment 14

[Simon Fraser [no longer active]]

Attachment: Better patch

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.

Comment 15

[jhp (no longer active)]

> 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

[jhp (no longer active)]

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.

Comment 17

[Simon Fraser [no longer active]]

(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.

Comment 18

[Simon Fraser [no longer active]]

Test camino build at
http://www.smfr.org/downloads/camino/Camino_20050808_quartz_tiling.zip

Comment 19

[Brian Ryner (not reading)]

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.

Comment 20

[Josh Aas]

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.

Comment 21

[Simon Fraser [no longer active]]

I'm fine with removing the pre-CGTiling code; we have CVS after all. Javier?

Comment 22

[Simon Fraser [no longer active]]

Comment on attachment 191996 [details] [diff] [review]
Better patch

Seeking approval on this big perf win. Has been tested on 10.2 -> 10.4.

Comment 23

[jhp (no longer active)]

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.

Comment 24

[Simon Fraser [no longer active]]

Ping for 1.8b4 approval (since we want this for camino 1.0).

Comment 25

[Simon Fraser [no longer active]]

Checked in on branch and trunk, with the #ifdefs. We can remove unused code once
we're sure this is regression-free.

Comment 26

[Boris Zbarsky [:bzbarsky]]

This apparently caused bug 

Comment 27

[Darin Fisher]

Boris was referring to bug 313062 :)