969117 - Add memory reporter for MediaCacheStream

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I don't think I will have time to work this right now.

Comment 2

[Nicholas Nethercote [inactive]]

cpearce? cdouble? Interested? :)

Comment 3

[Eric Rahm [: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)

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 6

[Eric Rahm [:erahm]]

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

Comment 7

[Eric Rahm [:erahm]]

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 9

[Eric Rahm [:erahm]]

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.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 11

[Eric Rahm [:erahm]]

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

Comment 12

[Nicholas Nethercote [inactive]]

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.

Comment 13

[Eric Rahm [:erahm]]

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.

Comment 14

[Eric Rahm [:erahm]]

Attachment: Report memory usage of MediaResources

Cleaned up style, added sizeof functions to nsDequeue.

Comment 15

[Eric Rahm [:erahm]]

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.

Comment 16

[Nicholas Nethercote [inactive]]

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 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

[Eric Rahm [:erahm]]

Attachment: Report memory usage of MediaResources

Properly measure functor size in deque.

Comment 19

[Nicholas Nethercote [inactive]]

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.

Comment 20

[Eric Rahm [:erahm]]

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

Comment 21

[Nicholas Nethercote [inactive]]

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

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 23

[Matthew Gregan [: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.

Comment 24

[Nicholas Nethercote [inactive]]

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!

Comment 25

[Eric Rahm [:erahm]]

Attachment: Report memory usage of MediaResources

This just removes unnecessary mozilla:: usage, no logic is changed.

Comment 26

[Eric Rahm [:erahm]]

Comment on attachment 8389376 [details] [diff] [review]
Report memory usage of MediaResources

Carrying over njn's r+

Comment 27

[Eric Rahm [:erahm]]

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

Comment 28

[Eric Rahm [:erahm]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3788d8852f62

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3788d8852f62