Last Comment Bug 672755 - Add memory reporters for media code
: Add memory reporters for media code
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Matthew Gregan [:kinetik]
:
:
Mentors:
Depends on: 675630
Blocks: DarkMatter 657115
  Show dependency treegraph
 
Reported: 2011-07-20 03:52 PDT by Matthew Gregan [:kinetik]
Modified: 2011-08-01 08:20 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0 (13.48 KB, patch)
2011-07-20 19:37 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v1 (18.41 KB, patch)
2011-07-20 22:03 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Splinter Review
patch v2 (18.73 KB, patch)
2011-07-21 20:01 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v3 (18.75 KB, patch)
2011-07-21 20:18 PDT, Matthew Gregan [:kinetik]
joe: review+
Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2011-07-20 03:52:14 PDT
I'll attach a patch here shortly.
Comment 1 Matthew Gregan [:kinetik] 2011-07-20 19:37:34 PDT
Created attachment 547313 [details] [diff] [review]
patch v0

There's not a great way to work out how big the allocation for each decoded video frame is, so I'm estimating it based on the frame dimensions.

This covers the single main use of memory in the media code.  It's probably worth tracking the size of the Ogg and WebM packet queues too.

It might be worth converting this over to the multireporter interface so that memory is attributed to specific URLs.
Comment 2 Timothy B. Terriberry (:derf) 2011-07-20 19:54:40 PDT
Comment on attachment 547313 [details] [diff] [review]
patch v0

>+  v->mSize = aBuffer.mPlanes[0].mStride * aBuffer.mPlanes[0].mHeight;
>+  v->mSize += aBuffer.mPlanes[1].mStride * aBuffer.mPlanes[1].mHeight;
>+  v->mSize += aBuffer.mPlanes[2].mStride * aBuffer.mPlanes[2].mHeight;
>+
>   videoImage->SetData(data); // Copies buffer

Wouldn't it make more sense to ask the videoImage how big the buffer it copied the data into is? That's what actually gets stored in the queue. The original YCbCrBuffer refers to data internal to the decoder, which generally rotates among a small, fixed set of buffers (3 for Theora, at least 4 for VP8). That size is constant over the life of the decoder (and also certainly not the only source of memory usage by the decoder itself).
Comment 3 Matthew Gregan [:kinetik] 2011-07-20 22:03:24 PDT
Created attachment 547322 [details] [diff] [review]
patch v1

This version extends PlanarYCbCrImage's interface so that it's possible to query the buffer size it actually used.
Comment 4 Matthew Gregan [:kinetik] 2011-07-20 22:26:05 PDT
Comment on attachment 547322 [details] [diff] [review]
patch v1

Chris for the content/media bit and Bas for the gfx/layers bit.
Comment 5 Nicholas Nethercote [:njn] 2011-07-20 23:35:23 PDT
Comment on attachment 547322 [details] [diff] [review]
patch v1

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

Thanks for doing this!

::: content/media/nsMediaDecoder.cpp
@@ +313,5 @@
> +
> +MediaMemoryReporter* MediaMemoryReporter::sUniqueInstance;
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(MediaDecodedVideoMemory,
> +                             "explicit/media/decoded/video",

Maybe just "explicit/media-decoded/video", or "explicit/decoded-media/video", or "explicit/media/decoded-video"?  There's not much point making the tree deeper than it needs to be, it just creates extra lines in about:memory that aren't useful. Likewise for the audio reporter.

(And if you add more reporters later, it's easy to change the paths then to make the tree deeper if you need it.)

@@ +314,5 @@
> +MediaMemoryReporter* MediaMemoryReporter::sUniqueInstance;
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(MediaDecodedVideoMemory,
> +                             "explicit/media/decoded/video",
> +                             KIND_HEAP,

Are you certain this memory is always on the heap (ie. allocated with malloc, operator new, or something similar)?  We've had all sorts of problems with correctly identifying where image memory is actually held -- sometimes on the heap, sometimes off the heap, sometimes in another process.  E.g. see bug 664659.  If you get it wrong, it can lead to nonsense (negative) amounts in about:memory.

@@ +321,5 @@
> +                             "Memory used by decoded video frames.")
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(MediaDecodedAudioMemory,
> +                             "explicit/media/decoded/audio",
> +                             KIND_HEAP,

Same question here.
Comment 6 Matthew Gregan [:kinetik] 2011-07-21 04:19:57 PDT
> Maybe just "explicit/media-decoded/video", or
> "explicit/decoded-media/video", or "explicit/media/decoded-video"?  There's

Sounds good.

> Are you certain this memory is always on the heap (ie. allocated with
> malloc, operator new, or something similar)?

Yes, the data in question is queued for later presentation and held in simple heap allocated memory.  Depending on the layers backend, the video images may also use video textures (generally at paint time), but these are not included in this code's accounting.
Comment 7 Nicholas Nethercote [:njn] 2011-07-21 04:50:42 PDT
(In reply to comment #6)
> 
> Yes, the data in question is queued for later presentation and held in
> simple heap allocated memory.  Depending on the layers backend, the video
> images may also use video textures (generally at paint time), but these are
> not included in this code's accounting.

Great!  Is it worth putting that info in a comment above the NS_MEMORY_REPORTER_IMPLEMENT?
Comment 8 Chris Pearce (:cpearce) 2011-07-21 17:29:02 PDT
Comment on attachment 547322 [details] [diff] [review]
patch v1

Can you add comments to nsMediaDecoder::{Audio,Video}QueueSize() so that it's clear they return a memory use counter in bytes, not a count of SoundData/VideoData. Otherwise content/media changes look good to me, modulo Nick's comments of course.
Comment 9 Matthew Gregan [:kinetik] 2011-07-21 20:01:22 PDT
Created attachment 547596 [details] [diff] [review]
patch v2

Addresses comments so far.  In addition to adding comments in nsMediaDecoder, I've renamed {Audio,Video}QueueSize to {Audio,Video}QueueMemoryInUse to clarify the purpose of the functions.  Also, rather than adding a comment as suggested in comment 7, I've added a comment directly to the base GetDataSize() on PlanarYCbCrImage, since that will keep the comment local to where the result is calculated.

Just need Bas to review the gfx/layers changes and this is done.
Comment 10 Nicholas Nethercote [:njn] 2011-07-21 20:07:05 PDT
Comment on attachment 547596 [details] [diff] [review]
patch v2

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

::: content/media/nsMediaDecoder.h
@@ +485,5 @@
> +
> +  DecodersArray mDecoders;
> +
> +  nsIMemoryReporter* mMediaDecodedVideoMemory;
> +  nsIMemoryReporter* mMediaDecodedAudioMemory;

These should be nsCOMPtr<nsIMemoryReporter>, AIUI.
Comment 11 Matthew Gregan [:kinetik] 2011-07-21 20:18:01 PDT
Created attachment 547597 [details] [diff] [review]
patch v3

You're right.  I copied this bug from the WebGL reporters, so someone will need to fix them up as well.
Comment 12 Matthew Gregan [:kinetik] 2011-07-26 16:19:14 PDT
Comment on attachment 547597 [details] [diff] [review]
patch v3

Just found out Bas is on leave, so I'll try Joe.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2011-07-27 12:10:39 PDT
(In reply to comment #11)
> Created attachment 547597 [details] [diff] [review] [review]
> patch v3
> 
> You're right.  I copied this bug from the WebGL reporters, so someone will
> need to fix them up as well.

Thanks for the catch. But nsRefPtr is enough here, no need for nsCOMPtr.
Comment 14 Nicholas Nethercote [:njn] 2011-07-27 17:38:00 PDT
(In reply to comment #13)
> 
> Thanks for the catch. But nsRefPtr is enough here, no need for nsCOMPtr.

Nope, see bug 638549 comment 29.
Comment 15 Joe Drew (not getting mail) 2011-07-28 14:29:31 PDT
Comment on attachment 547597 [details] [diff] [review]
patch v3

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

I only reviewed the layers code. Reflag me if you want other things reviewed.

::: gfx/layers/d3d10/ImageLayerD3D10.h
@@ +119,4 @@
>    virtual already_AddRefed<gfxASurface> GetAsSurface();
>  
>    nsAutoArrayPtr<PRUint8> mBuffer;
> +  PRUint32 mBufferSize;

You should probably initialize this in the constructor, though I won't r- based on it.
Comment 16 Matthew Gregan [:kinetik] 2011-07-28 18:56:41 PDT
Thanks.  mBufferSize is only ever used if it's valid, but I'll add a zero initialization before I check in the patch.

Note You need to log in before you can comment on or make changes to this bug.