Comment 60 for bug 217908

(From update of attachment 330932)
Some comments on the patch, and then overall comments at the end...

>Index: ./gfx/src/thebes/nsThebesImage.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v
>retrieving revision 1.86
>diff -u -8 -p -B -b -r1.86 nsThebesImage.cpp
>--- ./gfx/src/thebes/nsThebesImage.cpp 28 Apr 2008 21:27:05 -0000 1.86
>+++ ./gfx/src/thebes/nsThebesImage.cpp 22 Jul 2008 20:36:07 -0000
>@@ -43,48 +44,68 @@
> #include "gfxPattern.h"
>
> #include "gfxPlatform.h"
>
> #include "prenv.h"
>
> static PRBool gDisableOptimize = PR_FALSE;
>
>+#ifdef XP_UNIX
>+static gfxFloat gImageThreshold = 0.5;

This variable is oddly named; I'm not sure what it means, especially
not from the name. gImagePreScaleFactor? Though that's not quite how
it's used, see below.

> nsresult
> nsThebesImage::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth, nsMaskRequirements aMaskRequirements)
> {
> mWidth = aWidth;
> mHeight = aHeight;
>
>@@ -206,29 +227,28 @@ PRInt32
> nsThebesImage::GetAlphaLineStride()
> {
> return (mAlphaDepth > 0) ? mStride : 0;
> }
>
> void
> nsThebesImage::ImageUpdated(nsIDeviceContext *aContext, PRUint8 aFlags, nsRect *aUpdateRect)
> {
>+ mImageIncomplete = aFlags & nsImageUpdateFlags_ImageIncomplete;

You only modified the PNG decoder to pass down ImageIncomplete; why is
this info needed at all? You should be able to tell whether the image
is complete or not via the same mechanism used in GetIsImageComplete.

> nsresult
> nsThebesImage::Optimize(nsIDeviceContext* aContext)
> {
> if (gDisableOptimize)
> return NS_OK;
>
>@@ -326,21 +346,26 @@ nsThebesImage::Optimize(nsIDeviceContext
>
> #ifdef XP_MACOSX
> if (mQuartzSurface) {
> mQuartzSurface->Flush();
> mOptSurface = mQuartzSurface;
> }
> #endif
>
>+#ifndef XP_UNIX
> if (mOptSurface == nsnull)
> mOptSurface = gfxPlatform::GetPlatform()->OptimizeImage(mImageSurface, mFormat);
>+#endif
>+

If you want to do this, we should just early-return from Optimize
after the 1x1 check, instead of adding the extra ifdefs. But I'm not
sure it's a good idea, see below.

>@@ -482,24 +507,108 @@ nsThebesImage::Draw(nsIRenderingContext
> return NS_ERROR_FAILURE;
>
> // Expand the subimageRect to place its edges on integer coordinates.
> // Basically, if we're allowed to sample part of a pixel we can
> // sample the whole pixel.
> subimageRect.RoundOut();
>
> nsRefPtr<gfxPattern> pat;
>+#ifdef XP_UNIX
>+ // We optimize the image surface to it's visible size and use scaling algorithms
>+ // to reduce X memory consumption, bug 395260. We bypass 1x1 rectangles to avoid useless operations.
>+ if (destRect.size != gfxSize(1.0, 1.0) &&
>+ (mWidth > (destRect.size.width / gImageThreshold) ||
>+ mHeight > (destRect.size.height / gImageThreshold) ||
>+ xscale < (1.0 * gImageThreshold) || yscale < (1.0 * gImageThreshold) ||
>+ xscale > (1.0 / gImageThreshold) || yscale > (1.0 / gImageThreshold))) {

I don't understand this check at all; why isn't this just checking if
the xscale is less then gImageThreshold? Upscaling shouldn't go
through this path, you'll just end up creating a bigger temporary
surface than the original...

>+ gfxIntSize tinySize(NS_lroundf(destRect.size.width), NS_lroundf(destRect.size.height));
>+ GdkPixbuf* pixbuf = gdk_pixbuf_new_from_data(mImageSurface->Data(), GDK_COLORSPACE_RGB,
>+ PR_TRUE, 8, mWidth, mHeight,
>+ mStride, nsnull, nsnull);

All this scaling stuff should be done using thebes, not gdk_pixbuf;
there's no reason why all this code can't be shared on other platforms
as well if it helps.

>+ if (!pixbuf)
>+ return NS_ERROR_FAILURE;
>+
>+ GdkPixbuf* tinyPix = pixbuf;
[...]
>+
>+ nsRefPtr<gfxASurface> tmpSurf = gfxPlatform::GetPlatform()->CreateOffscreenSurface(tinySize, mFormat);

There's no reason to do this -- if you're just creating a new scaled
surface each time, just let cairo take care of getting it in whatever
format's useful for the platform. Optimizing to an offscreen surface
is only useful when you keep that image around.

[...]
>+ } else if (!mOptSurface) {
>+ // Sends pixmaps to X
>+ if (GetIsImageComplete()) {
>+ mOptSurface = gfxPlatform::GetPlatform()->OptimizeImage(mImageSurface, mFormat);
>+ } else {
>+ gfxPlatform::GetPlatform()->OptimizeImage(mImageSurface, mFormat);
>+ }

That second call does a bunch of work and then leaks the return. Why does it need to be called at all?

>--- ./modules/libpref/src/init/all.js 2 May 2008 06:27:27 -0000 3.758
>+++ ./modules/libpref/src/init/all.js 22 Jul 2008 20:36:11 -0000
>@@ -97,16 +97,19 @@ pref("browser.display.show_image_placeho
>+#ifdef XP_UNIX
>+pref("browser.gdk_image_interpolation_threshold", 50);
>+#endif

This shouldn't be present to 50%; it should be set to whatever the
value is for "don't touch anything", and then it can be tweaked from
there.

So overall, I think the idea is a good one, but I think the
implementation is flawed. Doing a pre-scale on each Draw invocation
might help with X memory consumption, but it'll be horrible for
performance.

A better approach would be to keep track of the maximum image size
that was requested in Draw. If it's bigger than the last max size,
then re-decode if needed and optimize to that size (up to the actual
image size). If a smaller size is requested, just draw whatever size
you currenly have scaled down. That would be generally useful on all
platforms, and shouldn't need any heuristics or significant pre-scaling.