Add memory reporters for media code

RESOLVED FIXED in mozilla8

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
I'll attach a patch here shortly.
(Assignee)

Comment 1

6 years ago
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 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).
(Assignee)

Comment 3

6 years ago
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.
Attachment #547313 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Comment on attachment 547322 [details] [diff] [review]
patch v1

Chris for the content/media bit and Bas for the gfx/layers bit.
Attachment #547322 - Flags: review?(chris)
Attachment #547322 - Flags: review?(bas.schouten)
Whiteboard: [MemShrink]
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.
(Assignee)

Comment 6

6 years ago
> 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.
Attachment #547322 - Flags: review?(chris) → review+
(Assignee)

Comment 9

6 years ago
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.
Attachment #547322 - Attachment is obsolete: true
Attachment #547596 - Flags: review?
Attachment #547322 - Flags: review?(bas.schouten)
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.
(Assignee)

Comment 11

6 years ago
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.
Attachment #547596 - Attachment is obsolete: true
Attachment #547597 - Flags: review?
Attachment #547596 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #547597 - Flags: review? → review?(bas.schouten)
Blocks: 657115
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 12

6 years ago
Comment on attachment 547597 [details] [diff] [review]
patch v3

Just found out Bas is on leave, so I'll try Joe.
Attachment #547597 - Flags: review?(bas.schouten) → review?(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.
Attachment #547597 - Flags: review?(joe) → review+
(Assignee)

Comment 16

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

Updated

6 years ago
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
http://hg.mozilla.org/mozilla-central/rev/b5705468f96f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][inbound] → [MemShrink:P2]
Target Milestone: --- → mozilla8

Updated

6 years ago
Depends on: 675630
You need to log in before you can comment on or make changes to this bug.