558467 - Implement nsIMemoryReporter for D2D cache(s)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Rob Arnold [:robarnold]]

[15:36:13] <shaver> rob will file it, and Bas will fix it!

Comment 1

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

This might also depend on bug 563701, where gfx*Surface is instrumented in a generic way, depending on what caches we're talking about here.

Comment 2

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

Attachment: simple surface memory

I dunno what the d2d caches are, but this adds a very simple guesstimate at d2d memory used by surfaces.

Comment 3

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

And, of course, it's wrong. s/8/4.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 450243 [details] [diff] [review]
simple surface memory

I don't think we actually have 8 bytes per pixel, do we?

Also, don't we have to RecordMemoryFreed?

Comment 5

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

Attachment: s/8/4/

Comment 6

[Bas Schouten (:bas.schouten)]

Please note that this is not exactly the cache ment in the bug! (it's still useful though!). The cache that we need to record as well is the one maintained internally by the D2D backend. It maintains copies in VRAM of image surfaces used as sources.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Expose D2D Cache usage from Cairo

This will make cairo record a guess at the D2D image surface cache memory usage.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Expose D2D Cache usage from Cairo v2

Something that shouldn't be there slipped in.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Create an nsIMemoryReporter to report D2D cache usage

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 454898 [details] [diff] [review]
Part 1: Expose D2D Cache usage from Cairo v2

>     if (!--existingBitmap->refs) {
>-	delete existingBitmap;
>+	D2D1_SIZE_U size = existingBitmap->bitmap->GetPixelSize();
>+	int Bpp = existingBitmap->bitmap->GetPixelFormat().format == DXGI_FORMAT_A8_UNORM ? 1 : 4;
>+	cache_usage -= size.width * size.height * Bpp;

How about using a function for computing the size of a bitmap.
Also be careful about mixing tabs and spaces.

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Expose D2D Cache usage from Cairo v3

I think a separate function is an excellent idea.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 454934 [details] [diff] [review]
Part 1: Expose D2D Cache usage from Cairo v3

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-private.h b/gfx/cairo/cairo/src/cairo-d2d-private.h
>--- a/gfx/cairo/cairo/src/cairo-d2d-private.h
>+++ b/gfx/cairo/cairo/src/cairo-d2d-private.h
>@@ -130,17 +130,17 @@ public:
>     static ID2D1Factory *Instance()
>     {
> 	if (!mFactoryInstance) {
> 	    D2D1CreateFactoryFunc createD2DFactory = (D2D1CreateFactoryFunc)
> 		GetProcAddress(LoadLibraryW(L"d2d1.dll"), "D2D1CreateFactory");
> 	    if (createD2DFactory) {
> 		D2D1_FACTORY_OPTIONS options;
> #ifdef DEBUG
>-		options.debugLevel = D2D1_DEBUG_LEVEL_INFORMATION;
>+		options.debugLevel = D2D1_DEBUG_LEVEL_NONE;
> #else
> 		options.debugLevel = D2D1_DEBUG_LEVEL_NONE;
> #endif
> 		createD2DFactory(
> 		    D2D1_FACTORY_TYPE_SINGLE_THREADED,
> 		    __uuidof(ID2D1Factory),
> 		    &options,
> 		    (void**)&mFactoryInstance);

Wrong change?


>diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>@@ -48,16 +48,18 @@ extern "C" {
> // Required for using placement new.
> #include <new>
> 
> ID2D1Factory *D2DSurfFactory::mFactoryInstance = NULL;
> ID3D10Device1 *D3D10Factory::mDeviceInstance = NULL;
> 
> #define CAIRO_INT_STATUS_SUCCESS (cairo_int_status_t)CAIRO_STATUS_SUCCESS
> 
>+static int cache_usage = 0;
>+

It doesn't matter for us but this should perhaps be a cairo_atomic_int_t
so we're threadsafe. If you don't want to look into it now, just add a comment.


> 
>+static int _d2d_compute_bitmap_mem_size(ID2D1Bitmap *bitmap)
>+{
>+    D2D1_SIZE_U size = bitmap->GetPixelSize();
>+    int Bpp = bitmap->GetPixelFormat().format == DXGI_FORMAT_A8_UNORM ? 1 : 4;
>+    return size.width * size.height * Bpp;
>+}

I'd prefer that you use a better name then Bpp because it's easily confused with
bits per pixel. There's only two uses so bytes_per_pixel or similar is probably fine.


>+
>+/**
>+ * Get an estimate of the amount of (video) RAM which is currently in use by the D2D
>+ * internal image surface cache.
>+ */
>+int cairo_d2d_get_cache_usage();

calling it cairo_d2d_get_image_surface_cache_usage() or something more verbose
is probably better because it describes which cache is being described and there probably wont ever be very many users.

Comment 13

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/4498d202b847.
Pushed http://hg.mozilla.org/mozilla-central/rev/d40d82f51b58.