make single-pixel optimization release surface memory

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
Graphics
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: vlad, Assigned: Joe Drew (not getting mail))

Tracking

({memory-footprint})

Trunk
mozilla1.9beta5
x86
All
memory-footprint
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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.
Flags: blocking1.9+
Keywords: footprint
Priority: -- → P3
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.

Updated

10 years ago
Priority: P3 → P2
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Assignee: vladimir → joe
Status: ASSIGNED → NEW
(Assignee)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
Created attachment 304604 [details]
SVG with a 100% size image filter
(Assignee)

Comment 4

10 years ago
Created attachment 304605 [details]
single-colour PNG
eek, is SVG code using Paint() to draw images?
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 8

10 years ago
Created attachment 305666 [details] [diff] [review]
release surfaces in single-pixel optimization case, v1

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.
Attachment #305666 - Flags: review?
(Assignee)

Comment 9

10 years ago
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.
Attachment #305666 - Flags: review? → review?(vladimir)
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)
Attachment #305666 - Flags: review?(vladimir) → review-
(Assignee)

Comment 11

10 years ago
Created attachment 305795 [details] [diff] [review]
release surfaces in single-pixel optimization case, v2

This addresses all of Vlad's comments, and it also removes the bad including of nsThebesImage.h in the SVG code.
Attachment #305666 - Attachment is obsolete: true
Attachment #305795 - Flags: review?(vladimir)
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?
Attachment #305795 - Flags: review?(vladimir) → review+
(Assignee)

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
Created attachment 305861 [details] [diff] [review]
release surfaces in single-pixel optimization case, v3

Updated with Linux compilation fixes.
Attachment #305795 - Attachment is obsolete: true

Updated

10 years ago
Flags: tracking1.9+ → blocking1.9+
Status: NEW → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 16

10 years ago
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.
(Assignee)

Comment 18

10 years ago
Created attachment 307994 [details] [diff] [review]
remove ThebesPattern

Sorry it took more time than anticipated -- i think i have the plague
Attachment #307994 - Flags: review?(vladimir)
Comment on attachment 307994 [details] [diff] [review]
remove ThebesPattern

Looks good, thanks!
Attachment #307994 - Flags: review?(vladimir) → review+

Updated

10 years ago
Depends on: 421780
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9beta5

Updated

10 years ago
Depends on: 421524
You need to log in before you can comment on or make changes to this bug.