995880 - VolatileBuffer non-heap memory use by images is not reported

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Also, I noticed the else was removed in the same patch, was that intentional? I restored it in this patch.

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 8405984 [details] [diff] [review]
patch

Review of attachment 8405984 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgFrame.cpp
@@ +927,5 @@
>  #endif
>  #ifdef XP_MACOSX
>    if (mQuartzSurface && aLocation == gfxMemoryLocation::IN_PROCESS_HEAP) {
>      n += aMallocSizeOf(mQuartzSurface);
> +  } else

This was in fact intentional. mQuartzSurface always wraps a mImageSurface and never really owns its data. We have to add the mImageSurface to count everything.

@@ +947,5 @@
>    }
>  
> +  if (mVBuf && aLocation == gfxMemoryLocation::IN_PROCESS_NONHEAP) {
> +    n += mVBuf->NonHeapSizeOfExcludingThis();
> +  }

Thanks - I really can't remember why I didn't just do this in the first place..

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Michael Wu [:mwu] from comment #2)
> ::: image/src/imgFrame.cpp
> @@ +927,5 @@
> >  #endif
> >  #ifdef XP_MACOSX
> >    if (mQuartzSurface && aLocation == gfxMemoryLocation::IN_PROCESS_HEAP) {
> >      n += aMallocSizeOf(mQuartzSurface);
> > +  } else
> 
> This was in fact intentional. mQuartzSurface always wraps a mImageSurface
> and never really owns its data. We have to add the mImageSurface to count
> everything.

Okay, removed that bit.

Comment 4

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9cda4dc752e

Comment 5

[Timothy Nikkel (:tnikkel)]

We should get this everywhere bug 962670 is, which I think is 30 and 1.3T, otherwise doing any sort of work with image memory there will be severely hampered.

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8405984 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 962670
User impact if declined: memory used by images will be dark matter in about:memory, ie we won't know what is using the memory used by images
Testing completed (on m-c, etc.): just landed m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 7

[Timothy Nikkel (:tnikkel)]

I don't know how to deal with just 1.3T (and not 1.3).

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9cda4dc752e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fdfcb379dda

(In reply to Timothy Nikkel (:tn) from comment #7)
> I don't know how to deal with just 1.3T (and not 1.3).

Request 1.3T blocking status (done :)..) and then land away once it has it.

Comment 10

[James Zhang (Spreadtrum)]

Fabrice, do we need land it on v1.3t?

Comment 11

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a3ac908e6f68

Comment 12

[[:fabrice] Fabrice Desré]

and backed out from 1.3t because I messed up the uplift:
 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/0b141ffd3b27

Comment 13

[[:fabrice] Fabrice Desré]

re-landed:
 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e08ab6f71cd7