563701 - add memory reporters for imglib

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add memory reporters for images, canvas, surfaces

This patch adds memory reporters for gfx surfaces, canvas, and imglib.

Comment 1

[Joe Drew (not getting mail)]

Comment on attachment 443385 [details] [diff] [review]
add memory reporters for images, canvas, surfaces

>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp

>+/* Memory reporter stuff */
>+static nsIMemoryReporter *gCanvasMemoryReporter = 0;

= nsnull?

>+    if (surface) {
>+        if (gCanvasMemoryReporter == 0) {
>+            gCanvasMemoryReporter = new NS_MEMORY_REPORTER_NAME(CanvasMemory);
>+            NS_RegisterMemoryReporter(gCanvasMemoryReporter);
>+        }

It might be overkill, but this could be done from an XPCOM initialization function, which would at least clean up SetDimensions().

>diff --git a/gfx/thebes/src/gfxASurface.cpp b/gfx/thebes/src/gfxASurface.cpp

>+class SurfaceMemoryReporter :

>+    NS_IMETHOD GetPath(char **memoryPath) {
>+        *memoryPath = (char*) malloc(256);
>+        strcpy(*memoryPath, "gfx/surface/");
>+        strcat(*memoryPath, gfxASurface::SurfaceNameForType(mType));

Well, that kind of sucks. Couldn't you use nsCString stuff?

>diff --git a/gfx/thebes/src/gfxWindowsSurface.cpp b/gfx/thebes/src/gfxWindowsSurface.cpp

>@@ -74,8 +74,13 @@ gfxWindowsSurface::gfxWindowsSurface(con

>+    int bytesPerPixel = imageFormat == gfxASurface::ImageFormatRGB24 ? 3 : 4;

bytesPerPixel is 4 when the format is gfxASurface::ImageFormatRGB24. And putting brackets in there would make it easier to read, too. :)

>@@ -90,8 +95,13 @@ gfxWindowsSurface::gfxWindowsSurface(HDC

>+    int bytesPerPixel = imageFormat == gfxASurface::ImageFormatRGB24 ? 3 : 4;

Same comment as above.

>diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp
>--- a/modules/libpr0n/src/imgFrame.cpp
>+++ b/modules/libpr0n/src/imgFrame.cpp
>@@ -774,10 +774,49 @@ PRUint32 imgFrame::GetImageBytesPerRow()
> 
> PRUint32 imgFrame::GetImageDataLength() const
> {
>-  if (mImageSurface)
>-    return mImageSurface->Stride() * mSize.height;
>-  else
>-    return mSize.width * mSize.height;
>+  PRUint32 size = 0;
>+  PRUint32 rsize = 0;

In this function, size seems to be calculated by hand, and rsize by asking the surface in question. We should always ask the surface in question, never calculate it ourselves, and not have two separate variables for this.

>+  PRBool addImageSurface = PR_TRUE;

This boolean seems not to be used.

>+  if (rsize < size) {
>+    fprintf(stderr, "rsize: %d size: %d!\n", rsize, size);
>+    fflush(stderr);
>+  }

Please remove debugging printf.

>+  // fall back to pessimistic size
>+  if (size == 0) {
>+    size = mSize.width * mSize.height * 4;
>+    rsize = mSize.width * mSize.height * 4;
>+  }

Did you run into this at all? I'd prefer not to have it if possible. Feel free to add an NS_ABORT_IF_FALSE, though.

>diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp

> #include "imgLoader.h"
>+#include "imgContainer.h"
>+#undef LoadImage

Please add a windows.h mention here.
 
>+class imgMemoryReporter :
>+  public nsIMemoryReporter
>+{
>+public:
>+#define CHROME_BIT  (1<<0)
>+#define CONTENT_BIT (0<<0)
>+#define USED_BIT    (1<<1)
>+#define UNUSED_BIT  (0<<1)
>+#define RAW_BIT     (1<<2)
>+#define UNCOMPRESSED_BIT (0<<2)

I really dislike these #defines - they seem sort of unclean - but more than that, 0 << anything = 0. Maybe you should just use PR_BIT? I don't think we can just say static const PRUInt8 CHROME_BIT = PR_BIT(0), can we?

Also, each of these should have defining comments.

>+  NS_IMETHOD GetPath(char **memoryPath) {
>+    if (mType == ChromeUsedRaw) {
>+      *memoryPath = strdup("imglib/images/chrome/used/raw");

Same wish for using nsCString instead.

>+  NS_IMETHOD GetMemoryUsed(PRInt64 *memoryUsed) {
>+    EnumArg arg = { mType, 0 };

EnumArg should just have a constructor that takes a type as its argument. And the function's { open brace should be on its own line here.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated, v2

Here's an updated patch.  The size/rsize stuff was a leftover that I thought I removed, but the removal ended up in the wrong patch, oops.

The strdup() stuff is unfortunately here to stay for now, due to the way the xpcom interfaces are defined.  I talked with bsmedberg yesterday on how to fix that, though it would take a good bit of work (need to switch to nsString everywhere, and find a way to store literal strings so that the data is shared appropriately).  It's doable, it'll just be a followup.

Comment 3

[Joe Drew (not getting mail)]

I'll get to this review early next week.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated, v3

This puts back GetImageDataLength (oops) and adds a new method for estimated image frame size.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 445810 [details] [diff] [review]
updated, v3


>diff --git a/gfx/thebes/public/gfxASurface.h b/gfx/thebes/public/gfxASurface.h

>+    /**
>+     * Same as above, but use current surface type.  In addition,
>+     * the bytes will be accumulated until RecordMemoryFreed is called,
>+     * in which case the value that was recorded for this surface will
>+     * be freed.
>+     */
>+    void RecordMemoryUsed(PRInt32 aBytes);
>+    void RecordMemoryFreed();

"...use current surface type as returned from GetType(). In addition..."

Also note that RecordMemoryFreed will be called from the destructor if it's not called before then.


>diff --git a/gfx/thebes/src/gfxPlatform.cpp b/gfx/thebes/src/gfxPlatform.cpp
>--- a/gfx/thebes/src/gfxPlatform.cpp
>+++ b/gfx/thebes/src/gfxPlatform.cpp
>@@ -167,7 +167,6 @@ static const char *gPrefLangNames[] = {
>     "x-user-def"
> };
> 
>-
> gfxPlatform*
> gfxPlatform::GetPlatform()
> {

Unrelated change - please drop.

wooo r+ otherwise

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in with review comments addressed, plus the palette change/fix as discussed on irc.

http://hg.mozilla.org/mozilla-central/rev/852ed6c76feb

Comment 7

[[e-mail]]

(In reply to comment #2)
> The strdup() stuff is unfortunately here to stay for now, due to the way the
> xpcom interfaces are defined.
In particular, XPCOM expects out string parameters to have been allocated using NS_Alloc because the caller expects to be able to NS_Free them. (You only get away with using strdup because of moz_malloc.)