Add memory reporter for MediaCacheStream

RESOLVED FIXED in mozilla30

Status

()

Core
Audio/Video
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: erahm)

Tracking

({perf})

unspecified
mozilla30
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=memory p= s=2014.03.14 u=] [MemShrink:P2])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
DMD run for the poppit app on b2g v1.3 showed that almost 4MB of heap-uncategorized was coming from MediaCacheStream.
(Reporter)

Comment 1

4 years ago
I don't think I will have time to work this right now.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
cpearce? cdouble? Interested? :)
Whiteboard: [c=memory p= s= u=][MemShrink] → [c=memory p= s= u=][MemShrink:P2]
Assignee: nobody → erahm
Ben, I'm not seeing unreported MediaCacheStream on my buri. Do you have the DMD logs anywhere? 

I _am_ seeing plenty of unreported memory though:
  - 3.5MB from ChannelMediaResource (libxul)
  - 3.5MB from MediaBuffer (libstagefright)
  - 3.0MB from SoftMP3 (libstagefright)
  - ~5MB  from mozilla::gfx::DrawTargetSkia::CreateSourceSurfaceFromData (libxul)
Flags: needinfo?(bkelly)
(Reporter)

Comment 4

4 years ago
I think the MediaCacheStream is indirectly allocated from ChannelMediaResource.  So thats probably where we want the reporter.  I can verify when I get back to my computer.
(Reporter)

Comment 5

4 years ago
Yea, MediaCacheStream is a memory of ChannelMediaResource:

  http://mxr.mozilla.org/mozilla-central/source/content/media/MediaResource.h#634

And the 32kB buffer assigned to MediaCacheStream::mPartialBlockBuffer accounts for most of the 3.5MB space:

  http://mxr.mozilla.org/mozilla-central/source/content/media/MediaCache.h#504

The mPartialBlockBuffer used to be an inline array memory of MediaCacheStream, but I moved it to be heap allocated to avoid slop in bug 969114:

  https://hg.mozilla.org/integration/b2g-inbound/rev/b986786cf7dd

So I guess depending on if your gecko has that commit or not, you might see the space show up under MediaCacheStream or ChannelMediaResource.

Does that help?
Flags: needinfo?(bkelly)
Thanks Ben, I eventually tracked it down. It just looks like DMD is getting confused / there's a bunch of inlining going on. 

Discussed with njn, it looks like the most correct path will be to track HTMLMediaElement's (which owns the decoder, which owns the resource, which owns the cache, which owns the 32k array).
Created attachment 8380056 [details]
DMD report for poppit

My best guess is that poppit has created 108 media elements to play MP3s, which seems like a bug on their end but adding memory reporting would certainly make it easier to track down. This expands a bit to an additional 6.5MB of memory related to audio decoding.
(Reporter)

Comment 8

4 years ago
Yes, it creates 4 or 5 HTML5 <audio> elements for each sound effect in the game.  This is a platform compatible way to get multi-channel audio (same sound effect playing more than once at a time).  I'm trying to get them to switch to use web audio in another bug.
It looks like we can use an existing reporter in MediaDecoder, my proposal is to report a cumulative value for "resources" (as in MediaResource) under explicit/media which would now look like this:

62.91 MB (100.0%) -- explicit
├──19.25 MB (30.59%) -- media
│  ├──15.71 MB (24.97%) -- decoded
│  │  ├──15.71 MB (24.97%) ── audio
│  │  └───0.00 MB (00.00%) ── video
│  └───3.54 MB (05.62%) ── resources

Interestingly Poppit also has 15.71 MB of decoded audio, which overall seems like a much larger issue.
(Reporter)

Comment 10

4 years ago
(In reply to Eric Rahm [:erahm] from comment #9)
> Interestingly Poppit also has 15.71 MB of decoded audio, which overall seems
> like a much larger issue.

Yes, that's a known issue.  See bug 959603.
Created attachment 8387207 [details] [diff] [review]
Report memory usage of MediaResources

Added sizeof functions to MediaResources, which in turn contain the MediaCacheStream. Memory is reported from the existing memory reporter in MediaDecoder.
Attachment #8387207 - Flags: review?(n.nethercote)
Attachment #8387207 - Flags: review?(cpearce)
Comment on attachment 8387207 [details] [diff] [review]
Report memory usage of MediaResources

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

Lots of nits, but it'll be good once they're addressed. Thanks.

::: content/media/BufferMediaResource.h
@@ +148,5 @@
> +    // Not owned:
> +    // - mBuffer
> +    // - mPrincipal
> +    return mContentType.SizeOfExcludingThisIfUnshared(aMallocSizeOf) +
> +           MediaResource::SizeOfExcludingThis(aMallocSizeOf);

Do the MediaResource:: one first.

::: content/media/MediaCache.cpp
@@ +388,5 @@
>  
> +size_t MediaCacheStream::SizeOfExcludingThis(
> +                                mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  // Looks these are not owned:

"Looks like"?

@@ +390,5 @@
> +                                mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  // Looks these are not owned:
> +  // - ChannelMediaResource*  mClient
> +  // - nsCOMPtr<nsIPrincipal> mPrincipal

Just give the names, not the types.

@@ +395,5 @@
> +  return mPartialBlockBuffer.SizeOfExcludingThis(aMallocSizeOf) +
> +         mReadaheadBlocks.SizeOfExcludingThis(aMallocSizeOf) +
> +         mMetadataBlocks.SizeOfExcludingThis(aMallocSizeOf) +
> +         mPlayedBlocks.SizeOfExcludingThis(aMallocSizeOf) +
> +         mBlocks.SizeOfExcludingThis(aMallocSizeOf);

Make the order of the sub-expressions match the order of the member declarations in the class. So mBlocks first, etc.

Also, usual style is to have a variable n and then do += to it repeatedly, and then |return n| (like you did with RtspMediaResource::SizeOfExcludingThis()). It's more flexible, e.g. it works when you have a loop in the middle of the function. There are several other functions in the patch that could be converted similarly. (But don't do it for the standard-form SizeOfIncludingThis() functions.)

@@ +399,5 @@
> +         mBlocks.SizeOfExcludingThis(aMallocSizeOf);
> +}
> +
> +size_t MediaCacheStream::SizeOfIncludingThis(
> +                                mozilla::MallocSizeOf aMallocSizeOf) const

For the simple ones like this that don't involve inheritance, you can squash SizeOfIncludingThis() and SizeOfExcludingThis() together if only the former is directly called. (I'm not sure if that's the case here, though.)

::: content/media/MediaResource.h
@@ +393,5 @@
>      return false;
>    }
>  
> +  virtual size_t SizeOfExcludingThis(
> +                    mozilla::MallocSizeOf /*aMallocSizeOf*/) const {

No need to comment out aMallocSizeOf, we don't have -Wunused-params on (because it would give a gazillion warnings).

Also: MOZ_OVERRIDE?

@@ +398,5 @@
> +    return 0;
> +  }
> +
> +  virtual size_t SizeOfIncludingThis(
> +                    mozilla::MallocSizeOf aMallocSizeOf) const {

MOZ_OVERRIDE?

::: content/media/RtspMediaResource.cpp
@@ +70,5 @@
>      mRingBuffer = nullptr;
>    };
> +
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +    return aMallocSizeOf(mRingBuffer.get());

nsAutoArrayPtr has a SizeOfExcludingThis() function, so use that instead.

@@ +385,5 @@
> +  size_t size = BaseMediaResource::SizeOfExcludingThis(aMallocSizeOf);
> +  size += mTrackBuffer.SizeOfExcludingThis(aMallocSizeOf);
> +
> +  // Include the size of each track buffer.
> +  for (size_t idx = 0; idx < mTrackBuffer.Length(); idx++)

Use |i| for the loop variable.

@@ +386,5 @@
> +  size += mTrackBuffer.SizeOfExcludingThis(aMallocSizeOf);
> +
> +  // Include the size of each track buffer.
> +  for (size_t idx = 0; idx < mTrackBuffer.Length(); idx++)
> +  {

Opening brace at end of the previous line.

::: content/media/mediasource/MediaSourceDecoder.h
@@ +136,5 @@
> +  virtual size_t SizeOfExcludingThis(
> +                      mozilla::MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE
> +  {
> +    return mType.SizeOfExcludingThisIfUnshared(aMallocSizeOf) +
> +           MediaResource::SizeOfExcludingThis(aMallocSizeOf);

Do MediaResource:: first again.

::: content/media/mediasource/SourceBufferResource.h
@@ +52,5 @@
>        mData.AppendElements(aData, aSize);
>      }
>      nsTArray<uint8_t> mData;
> +
> +    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Opening brace on the next line.

@@ +56,5 @@
> +    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +      return mData.SizeOfExcludingThis(aMallocSizeOf);
> +    }
> +
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Ditto.

@@ +57,5 @@
> +      return mData.SizeOfExcludingThis(aMallocSizeOf);
> +    }
> +
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +      return aMallocSizeOf(this) + mData.SizeOfExcludingThis(aMallocSizeOf);

Typo! Remove the |mData.|.

@@ +183,5 @@
>        }
>        return evicted;
>      }
> +
> +    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Opening brace on next line.

@@ +189,5 @@
> +
> +      // Calculate the size of the internal deque.
> +      if (mData != mBuffer) {
> +        size += aMallocSizeOf(mData);
> +      }

It would be better to implement nsDeque::SizeOfExcludingThis() and then calling that, rather than measuring mData and mDeallocator directly.
Attachment #8387207 - Flags: review?(n.nethercote) → review+
Comment on attachment 8387207 [details] [diff] [review]
Report memory usage of MediaResources

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

::: content/media/MediaCache.cpp
@@ +399,5 @@
> +         mBlocks.SizeOfExcludingThis(aMallocSizeOf);
> +}
> +
> +size_t MediaCacheStream::SizeOfIncludingThis(
> +                                mozilla::MallocSizeOf aMallocSizeOf) const

IncludingThis is never called, will remove it. Others have been merged where it makes sense.

::: content/media/MediaResource.h
@@ +393,5 @@
>      return false;
>    }
>  
> +  virtual size_t SizeOfExcludingThis(
> +                    mozilla::MallocSizeOf /*aMallocSizeOf*/) const {

This is the base class, so it hasn't been overridden yet. (I wish we had expandable context in bz reviews, it would be easier to tell).

@@ +398,5 @@
> +    return 0;
> +  }
> +
> +  virtual size_t SizeOfIncludingThis(
> +                    mozilla::MallocSizeOf aMallocSizeOf) const {

This is the base class, so it hasn't been overridden yet.

::: content/media/mediasource/SourceBufferResource.h
@@ +52,5 @@
>        mData.AppendElements(aData, aSize);
>      }
>      nsTArray<uint8_t> mData;
> +
> +    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

This is following the class style (note the above constructor), which my from my understanding gets priority. Of course this file actually uses multiple styles so I don't care too much.

@@ +56,5 @@
> +    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +      return mData.SizeOfExcludingThis(aMallocSizeOf);
> +    }
> +
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Same deal.

@@ +57,5 @@
> +      return mData.SizeOfExcludingThis(aMallocSizeOf);
> +    }
> +
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +      return aMallocSizeOf(this) + mData.SizeOfExcludingThis(aMallocSizeOf);

Nice catch, I'll just merge the two since ExcludingThis is never called directly.

@@ +189,5 @@
> +
> +      // Calculate the size of the internal deque.
> +      if (mData != mBuffer) {
> +        size += aMallocSizeOf(mData);
> +      }

Good point, I was avoiding a larger impact but that will probably be useful for others.
Created attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

Cleaned up style, added sizeof functions to nsDequeue.
Attachment #8387207 - Attachment is obsolete: true
Attachment #8387207 - Flags: review?(cpearce)
Comment on attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

Carrying over r+ from njn. cpearce I'm tagging you for feedback on the MediaDecoder changes in case you have any opinion.
Attachment #8387832 - Flags: review+
Attachment #8387832 - Flags: feedback?(cpearce)
Comment on attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

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

::: xpcom/glue/nsDeque.cpp
@@ +97,5 @@
> +    size += aMallocSizeOf(mData);
> +  }
> +
> +  if (mDeallocator) {
> +    size += aMallocSizeOf(mData);

s/mData/mDeallocator/, presumably?

DMD would detect this double-counting of mData.
Comment on attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

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

::: xpcom/glue/nsDeque.cpp
@@ +97,5 @@
> +    size += aMallocSizeOf(mData);
> +  }
> +
> +  if (mDeallocator) {
> +    size += aMallocSizeOf(mData);

Also, you could use ?: for both of the additions in this function.
Created attachment 8388676 [details] [diff] [review]
Report memory usage of MediaResources

Properly measure functor size in deque.
Attachment #8388676 - Flags: review?(n.nethercote)
Comment on attachment 8388676 [details] [diff] [review]
Report memory usage of MediaResources

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

::: xpcom/glue/nsDeque.cpp
@@ +97,5 @@
> +    size += aMallocSizeOf(mData);
> +  }
> +
> +  if (mDeallocator) {
> +    size += aMallocSizeOf(mDeallocator);

Should nsDequeFunctor have a SizeOfIncludingThis() function? I'm not sure; it's a odd little class.
Attachment #8388676 - Flags: review+
Comment on attachment 8388676 [details] [diff] [review]
Report memory usage of MediaResources

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

::: xpcom/glue/nsDeque.cpp
@@ +97,5 @@
> +    size += aMallocSizeOf(mData);
> +  }
> +
> +  if (mDeallocator) {
> +    size += aMallocSizeOf(mDeallocator);

I don't think it's worth it, the only use is in this class and standard usage would imply no heap allocated memory within.

Otherwise we'd need to track down all implementations and update them: http://dxr.mozilla.org/mozilla-central/search?q=nsDequeFunctor&case=true
> I don't think it's worth it, the only use is in this class and standard
> usage would imply no heap allocated memory within.

Fair enough.
Comment on attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

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

Looks good to me, but I think Matthew Gregan should f+ this, to ensure that the MediaSourceBuffer stuff is correct.

::: content/media/MediaDecoder.cpp
@@ +1827,5 @@
>    DecodersArray& decoders = Decoders();
>    for (size_t i = 0; i < decoders.Length(); ++i) {
> +    MediaDecoder* decoder = decoders[i];
> +    video += decoder->VideoQueueMemoryInUse();
> +    audio += decoder->SizeOfAudioQueue();

MediaDecoder::VideoQueueMemoryInUse() doesn't need to be virtual, right?
Attachment #8387832 - Flags: feedback?(cpearce) → feedback?(kinetik)
Comment on attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

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

Looks good.  The mozilla:: prefixes shouldn't be needed as this (and most) code should be inside the mozilla namespace already.
Attachment #8387832 - Flags: feedback?(kinetik) → feedback+
Comment on attachment 8388676 [details] [diff] [review]
Report memory usage of MediaResources

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

Due to some BMO database weirdness (bug 981986) I get to r+ this patch again!
Attachment #8388676 - Flags: review?(n.nethercote) → review+
Created attachment 8389376 [details] [diff] [review]
Report memory usage of MediaResources

This just removes unnecessary mozilla:: usage, no logic is changed.
Attachment #8387832 - Attachment is obsolete: true
Attachment #8388676 - Attachment is obsolete: true
Comment on attachment 8389376 [details] [diff] [review]
Report memory usage of MediaResources

Carrying over njn's r+
Attachment #8389376 - Flags: review+
Comment on attachment 8387832 [details] [diff] [review]
Report memory usage of MediaResources

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

::: content/media/MediaDecoder.cpp
@@ +1827,5 @@
>    DecodersArray& decoders = Decoders();
>    for (size_t i = 0; i < decoders.Length(); ++i) {
> +    MediaDecoder* decoder = decoders[i];
> +    video += decoder->VideoQueueMemoryInUse();
> +    audio += decoder->SizeOfAudioQueue();

It looks like it does not, I think we can address that in Bug 962154
(In reply to Matthew Gregan [:kinetik] from comment #23)
> Comment on attachment 8387832 [details] [diff] [review]
> Report memory usage of MediaResources
> 
> Review of attachment 8387832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  The mozilla:: prefixes shouldn't be needed as this (and most)
> code should be inside the mozilla namespace already.

Thanks for taking a look, I removed the mozilla namespace usage (except in nsDeque where it's needed).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3788d8852f62
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Updated

4 years ago
Priority: -- → P2
Whiteboard: [c=memory p= s= u=][MemShrink:P2] → [c=memory p= s=2014.03.14 u=] [MemShrink:P2]
You need to log in before you can comment on or make changes to this bug.