I'll attach a patch here shortly.

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 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).

This version extends PlanarYCbCrImage's interface so that it's possible to query the buffer size it actually used.

Comment on attachment 547322 [details] [diff] [review] patch v1 Chris for the content/media bit and Bas for the gfx/layers bit.

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.

> 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.

(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 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.

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 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.

You're right. I copied this bug from the WebGL reporters, so someone will need to fix them up as well.

Comment on attachment 547597 [details] [diff] [review] patch v3 Just found out Bas is on leave, so I'll try Joe.

(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.

(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 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.

Thanks. mBufferSize is only ever used if it's valid, but I'll add a zero initialization before I check in the patch.