After the fix for bug 371867 lands, we can free up memory by releasing all the surfaces. However, some things (clipboard, drag and drop) are going to want a Surface -- what we can do is recreate a surface in GetSurface and return a ref that we don't own, as long as it's understood that modifying the surface returned by GetSurface won't get you an image you can modify. Alternately, we can force those callers to do LockImagePixels/UnlockImagePixels.

Some more description here... Right now, there is nsThebesImage::ThebesSurface(). Some code expects to be able to get the actual surface under a nsThebesImage by calling GetSurface/ThebesSurface, usually so that it can encode it (clipboard, drag and drop). Most code, though, just wants to call it so that it can get something it can use as a rendering source. So, I would introduce a ThebesPattern() call, that would return a gfxPattern -- since a pattern can either be a surface pattern or a solid pattern, and is always read-only, this would encapsulate the optimization nicely. For the code that needs the actual image data, if we have the single-pixel optimization stuff in place, we can then just create a new gfxImageSurface, fill it with the right color, and hand it back. The caller would just release it when it's done with it (via refcounting). First off I would identify all the consumers of ThebesSurface()/GetSurface() and see what they want to do with the resulting surface.

I've run across at least one problem with using patterns instead of surfaces, and it comes up in an SVG testcase I'll attach. Since single-colour patterns don't have any sizes (unlike image-based patterns), they won't be clipped or resized when we use them instead of the surface. This seems to be a Cairo limitation.

eek, is SVG code using Paint() to draw images?

Yeah. Both <image> and the feImage filter use nsSVGUtils::CompositeSurfaceMatrix, which just calls Paint(). http://mxr.mozilla.org/firefox/source/layout/svg/base/src/nsSVGUtils.cpp#1580

Ok, especially given that they already have a Save()/Restore() around there, the easiest thing to do would be to set a clip rectangle before calling paint, to [0,0,width,height] of the image.

This patch implements the support required to release surfaces (and enables the release-surfaces code too). There are undoubtedly ugly bits, including the need to downcast to nsThebesImage when we need to access ThebesPattern. Maybe there should be an nsIImage::GetPattern()? Other notes: - This patch includes totally untested, uncompiled code in widget/src/gtk2/nsImageToPixbuf. Linux devs requested. :) - I never found any obvious clipboard or drag and drop code; certainly nothing that calls GetSurface or ThebesSurface. Drag and drop definitely still works on Mac, though, and it appears that cut and paste do (though I only ever got things like Word to paste in the _path_ to a single-colour image). - Firefox definitely releases the memory; without this patch, a 4096x4096 single-colour image makes Firefox grow to 100+ MB; with it, less than 50.

Comment on attachment 305666 [details] [diff] [review] release surfaces in single-pixel optimization case, v1 Whoops - I edited out all the CVS diff unknown files, but apparently forgot to save the file before adding the attachment.

Comment on attachment 305666 [details] [diff] [review] release surfaces in single-pixel optimization case, v1 >Index: content/canvas/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/content/canvas/src/Makefile.in,v >retrieving revision 1.15 >diff -u -8 -p -r1.15 Makefile.in >--- content/canvas/src/Makefile.in 18 Feb 2008 08:50:09 -0000 1.15 >+++ content/canvas/src/Makefile.in 26 Feb 2008 03:14:38 -0000 >@@ -80,10 +80,11 @@ CPPSRCS += nsCanvasRenderingContext2D.c > endif > > # we don't want the shared lib, but we want to force the creation of a static lib. > FORCE_STATIC_LIB = 1 > > include $(topsrcdir)/config/rules.mk > > CXXFLAGS += $(MOZ_CAIRO_CFLAGS) $(TK_CFLAGS) >+INCLUDES += -I$(topsrcdir)/gfx/src/thebes > > DEFINES += -D_IMPL_NS_LAYOUT Add a GetPattern method to nsIImage; don't reach in to gfx's private bits here :) >Index: content/canvas/src/nsCanvasRenderingContext2D.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v >retrieving revision 1.118 >diff -u -8 -p -r1.118 nsCanvasRenderingContext2D.cpp >--- content/canvas/src/nsCanvasRenderingContext2D.cpp 21 Feb 2008 08:37:30 -0000 1.118 >+++ content/canvas/src/nsCanvasRenderingContext2D.cpp 26 Feb 2008 03:14:38 -0000 >@@ -58,16 +58,17 @@ > #include "imgIContainer.h" > #include "gfxIImageFrame.h" > #include "nsIDOMHTMLCanvasElement.h" > #include "nsICanvasElement.h" > #include "nsIDOMHTMLImageElement.h" > #include "nsIImageLoadingContent.h" > #include "nsIInterfaceRequestorUtils.h" > #include "nsIImage.h" >+#include "nsThebesImage.h" > #include "nsIFrame.h" > #include "nsDOMError.h" > #include "nsIScriptError.h" > > #include "nsICSSParser.h" > #include "nsICSSStyleRule.h" > #include "nsStyleSet.h" > >@@ -2202,38 +2203,53 @@ nsCanvasRenderingContext2D::CairoSurface > > if (!imgContainer) > return NS_ERROR_NOT_AVAILABLE; > > nsCOMPtr<gfxIImageFrame> frame; > rv = imgContainer->GetCurrentFrame(getter_AddRefs(frame)); > NS_ENSURE_SUCCESS(rv, rv); > >- nsCOMPtr<nsIImage> img(do_GetInterface(frame)); >+ nsCOMPtr<nsIImage> imgiface(do_GetInterface(frame)); >+ nsThebesImage *img = static_cast<nsThebesImage *>(imgiface.get()); (see above) > > PRInt32 imgWidth, imgHeight; > rv = frame->GetWidth(&imgWidth); > rv |= frame->GetHeight(&imgHeight); > if (NS_FAILED(rv)) > return NS_ERROR_FAILURE; > > if (widthOut) > *widthOut = imgWidth; > if (heightOut) > *heightOut = imgHeight; > >- nsRefPtr<gfxASurface> gfxsurf; >- rv = img->GetSurface(getter_AddRefs(gfxsurf)); >- NS_ENSURE_SUCCESS(rv, rv); >+ nsRefPtr<gfxPattern> gfxpattern = img->ThebesPattern(); >+ nsRefPtr<gfxASurface> gfxsurf = gfxpattern->GetSurface(); > >- *aCairoSurface = gfxsurf->CairoSurface(); >- cairo_surface_reference (*aCairoSurface); >- *imgData = nsnull; >+ if (gfxsurf) { >+ *aCairoSurface = gfxsurf->CairoSurface(); >+ cairo_surface_reference (*aCairoSurface); >+ *imgData = nsnull; > >- return NS_OK; >+ return NS_OK; >+ } else { >+ gfxsurf = new gfxImageSurface (gfxIntSize(imgWidth, imgHeight), gfxASurface::ImageFormatARGB32); >+ nsRefPtr<gfxContext> ctx = new gfxContext(gfxsurf); >+ >+ ctx->SetOperator(gfxContext::OPERATOR_SOURCE); >+ ctx->SetPattern(gfxpattern); >+ ctx->Paint(); >+ >+ *aCairoSurface = gfxsurf->CairoSurface(); >+ cairo_surface_reference (*aCairoSurface); >+ *imgData = nsnull; >+ >+ return NS_OK; >+ } Pull out the common code here -- e.g. if (!gfxsurf) { gfxSurf = new gfxImage.... ... ctx->Paint(); } *aCairoSurface = gfxsurf->... ... return NS_OK; >Index: widget/src/gtk2/nsImageToPixbuf.h >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/gtk2/nsImageToPixbuf.h,v >retrieving revision 1.3 >diff -u -8 -p -r1.3 nsImageToPixbuf.h >--- widget/src/gtk2/nsImageToPixbuf.h 16 Jun 2007 02:13:55 -0000 1.3 >+++ widget/src/gtk2/nsImageToPixbuf.h 26 Feb 2008 03:14:44 -0000 >@@ -36,28 +36,34 @@ > * ***** END LICENSE BLOCK ***** */ > > #ifndef NSIMAGETOPIXBUF_H_ > #define NSIMAGETOPIXBUF_H_ > > #include "nsIImageToPixbuf.h" > > class gfxASurface; >+class gfxPattern; >+class gfxImageSurface; > > class nsImageToPixbuf : public nsIImageToPixbuf { > public: > NS_DECL_ISUPPORTS > NS_IMETHOD_(GdkPixbuf*) ConvertImageToPixbuf(nsIImage* aImage); > > // Friendlier version of ConvertImageToPixbuf for callers inside of > // widget > static GdkPixbuf* ImageToPixbuf(nsIImage* aImage); > static GdkPixbuf* SurfaceToPixbuf(gfxASurface* aSurface, > PRInt32 aWidth, PRInt32 aHeight); >+ static GdkPixbuf* PatternToPixbuf(gfxPattern* aPattern, >+ PRInt32 aWidth, PRInt32 aHeight); > private: >+ static GdkPixbuf *ImgSurfaceToPixbuf(nsRefPtr<gfxImageSurface>, >+ PRInt32 aWidth, PRInt32 aHeight); ^ the first param can't (shouldn't) be nsRefPtr<gfxImageSurface>; just gfxImageSurface*. Also, minor nit, watch the position of the * -- usually just try to follow convention in whatever file you're in (so GdkPixbuf* for the return type)

This addresses all of Vlad's comments, and it also removes the bad including of nsThebesImage.h in the SVG code.

Comment on attachment 305795 [details] [diff] [review] release surfaces in single-pixel optimization case, v2 Looks great, get it on the try server to make sure it builds cleanly everywhere? Also, did you do reftests and all that fun testing stuff?

Tryserver is in progress; I ran all sorts of tests, including the reftests and the Mochitests, and all were exactly the same before and after.

Updated with Linux compilation fixes.

Checked in! Checking in content/canvas/src/nsCanvasRenderingContext2D.cpp; /cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v <-- nsCanvasRenderingContext2D.cpp new revision: 1.119; previous revision: 1.118 done Checking in content/svg/content/src/Makefile.in; /cvsroot/mozilla/content/svg/content/src/Makefile.in,v <-- Makefile.in new revision: 1.74; previous revision: 1.73 done Checking in content/svg/content/src/nsSVGFilters.cpp; /cvsroot/mozilla/content/svg/content/src/nsSVGFilters.cpp,v <-- nsSVGFilters.cpp new revision: 1.67; previous revision: 1.66 done Checking in gfx/public/nsIImage.h; /cvsroot/mozilla/gfx/public/nsIImage.h,v <-- nsIImage.h new revision: 1.35; previous revision: 1.34 done Checking in gfx/src/thebes/nsThebesImage.cpp; /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v <-- nsThebesImage.cpp new revision: 1.77; previous revision: 1.76 done Checking in gfx/src/thebes/nsThebesImage.h; /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.h,v <-- nsThebesImage.h new revision: 1.26; previous revision: 1.25 done Checking in gfx/thebes/public/gfxPattern.h; /cvsroot/mozilla/gfx/thebes/public/gfxPattern.h,v <-- gfxPattern.h new revision: 1.15; previous revision: 1.14 done Checking in gfx/thebes/src/gfxPattern.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxPattern.cpp,v <-- gfxPattern.cpp new revision: 1.7; previous revision: 1.6 done Checking in layout/svg/base/src/Makefile.in; /cvsroot/mozilla/layout/svg/base/src/Makefile.in,v <-- Makefile.in new revision: 1.68; previous revision: 1.67 done Checking in layout/svg/base/src/nsSVGImageFrame.cpp; /cvsroot/mozilla/layout/svg/base/src/nsSVGImageFrame.cpp,v <-- nsSVGImageFrame.cpp new revision: 1.56; previous revision: 1.55 done Checking in layout/svg/base/src/nsSVGUtils.cpp; /cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.cpp,v <-- nsSVGUtils.cpp new revision: 1.101; previous revision: 1.100 done Checking in layout/svg/base/src/nsSVGUtils.h; /cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.h,v <-- nsSVGUtils.h new revision: 1.66; previous revision: 1.65 done Checking in modules/libpr0n/src/imgContainer.cpp; /cvsroot/mozilla/modules/libpr0n/src/imgContainer.cpp,v <-- imgContainer.cpp new revision: 1.66; previous revision: 1.65 done Checking in modules/libpr0n/src/imgTools.cpp; /cvsroot/mozilla/modules/libpr0n/src/imgTools.cpp,v <-- imgTools.cpp new revision: 1.4; previous revision: 1.3 done Checking in widget/src/gtk2/nsImageToPixbuf.cpp; /cvsroot/mozilla/widget/src/gtk2/nsImageToPixbuf.cpp,v <-- nsImageToPixbuf.cpp new revision: 1.9; previous revision: 1.8 done Checking in widget/src/gtk2/nsImageToPixbuf.h; /cvsroot/mozilla/widget/src/gtk2/nsImageToPixbuf.h,v <-- nsImageToPixbuf.h new revision: 1.4; previous revision: 1.3 done

Get rid of ThebesPattern from nsThebesImage -- we don't have any other functions like that that return new unrefed objects

Yeah, that was my bad, didn't catch that in review. It should either return already_AddRefed<gfxPattern> and do an addref inside it, or, as stuart says, we should just get rid of it and put the code inside GetPattern (since noone else calls it anyway). Joe, can you do a quick followup patch and just attach it here? Poke me tomorrow to check in.

Sorry it took more time than anticipated -- i think i have the plague

Comment on attachment 307994 [details] [diff] [review] remove ThebesPattern Looks good, thanks!

Checking in gfx/src/thebes/nsThebesImage.h; /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.h,v <-- nsThebesImage.h new revision: 1.30; previous revision: 1.29 done