Closed Bug 558467 Opened 14 years ago Closed 14 years ago

Implement nsIMemoryReporter for D2D cache(s)

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: robarnold, Assigned: bas.schouten)

References

(Blocks 1 open bug, )

Details

Attachments

(4 files, 2 obsolete files)

[15:36:13] <shaver> rob will file it, and Bas will fix it!
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.
Blocks: DarkMatter
I dunno what the d2d caches are, but this adds a very simple guesstimate at d2d memory used by surfaces.
Attachment #450243 - Flags: review?(joe)
And, of course, it's wrong. s/8/4.
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?
Attachment #450243 - Flags: review?(joe) → review-
Attachment #450367 - Flags: review?(joe) → review+
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.
This will make cairo record a guess at the D2D image surface cache memory usage.
Attachment #454896 - Flags: review?(jmuizelaar)
Something that shouldn't be there slipped in.
Attachment #454896 - Attachment is obsolete: true
Attachment #454898 - Flags: review?(jmuizelaar)
Attachment #454896 - Flags: review?(jmuizelaar)
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.
Attachment #454898 - Flags: review?(jmuizelaar) → review-
I think a separate function is an excellent idea.
Attachment #454898 - Attachment is obsolete: true
Attachment #454934 - Flags: review?(jmuizelaar)
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.
Attachment #454934 - Flags: review?(jmuizelaar) → review+
Attachment #454902 - Flags: review?(jmuizelaar) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: