Closed Bug 962154 Opened 6 years ago Closed 6 years ago

decoded-video data does not use MallocSizeOf

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bkelly, Assigned: erahm)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

Similar to bug 961441, the decoded-video entry in about:memory is not generated using MallocSizeOf().  The VideoQueueMemoryFunctor in MediaDecoderReader.cpp is pulling the size using PlanarYCbCrImage::GetDataSize().  This in turn simply returns the size_t used to new buffer.

I think GetDataSize() is used for other image processing, so perhaps we should add a new malloc-specific method here.

Its not clear to me at the moment, though, how the PlanarYCbCrImage size is handled with the other image memory reporting.
Whiteboard: [MemShrink]
Blocks: 682195
No longer blocks: 682195
Whiteboard: [MemShrink] → [MemShrink:P2]
Note, when this is fixed these other minor issues should be addressed as well:

1) Rename VideoQueueMemoryInUse() to SizeOfVideoQueue() to match memory reporter conventions.
2) Use size_t instead int64_t in SizeOfVideoQueue() to match memory reporter conventions.
3) SizeOfVideoQueue probably does not need to be virtual any more.

These were all things fixed on the audio side in bug 961441.
Assignee: nobody → erahm
tl;dr - We've been double reporting this memory.

It looks like if I fix this we'll end up double-reporting the memory used (at least on desktop OSX). In my testing I found SharedPlanarYCbCrImage[1] is actually used rather than PlanarYCbCrImage[2]. PlanarYCbCrImage's |mBuffer| is empty, SharedPlanarYCbCrImage uses a buffer contained in a BufferedTextureClient, which in turn can be either a MemoryTextureClient or a ShmemTextureClient. In my case it's a MemoryTextureClient and it's |mBuffer| is already reported[3].

So we could either not report the memory under explicit/gfx/heap-textures or not report the memory under explicit/media/decoded/video, it's not totally clear to me which one is right at this point.

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/SharedPlanarYCbCrImage.h?from=SharedPlanarYCbCrImage#92
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.h?from=PlanarYCbCrImage&case=true#810
[3] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#604
kinetik, do you have any preference w.r.t. comment 2?

Also, this bug is very much on point: if the reporter was using MallocSizeOf, then DMD would have complained about the double-reporting.
Flags: needinfo?(kinetik)
I'm pretty sure this reporter was written before we had MallocSizeOf and hasn't been touched since.

No real preference re comment 2, I can think of good reasons to use either of them.
Flags: needinfo?(kinetik)
At a high level it would be nice to report the memory under decoded video (what it's being used for), but that would mean we would lose reporting of heap-textures used for other things (or we could track down where those are used and add reporters for all of those cases, that sounds like a large effort). 

I think there's a third (slightly hacky) option where we could add a TextureClient flag that says "don't report this memory via GfxMemoryImageReporter" which we could set on TextureClients used for video. That way we can fix up GfxMemoryImageReporter to do the right thing for non-video memory and video memory could still be properly classified.
This patch properly uses MallocSizeOf to measure the memory used in the decoded video queue. Unfortunately it became a little wider reaching as there are a few classes derived from PlanarYCbCrImage.

One major side-effect is that we will no longer double-report video memory usage if it is in the form of a SharedPlanarYCbCrImage -- that memory will continue to be reported via gfx/textures and the decoded video entry will most likely be empty.
Attachment #8393832 - Flags: review?(n.nethercote)
Attachment #8393832 - Flags: review?(mwu)
Attachment #8393832 - Flags: review?(mwu) → review?(cpearce)
Comment on attachment 8393832 [details] [diff] [review]
Use MallocSizeOf to reporte decoded-video memory.

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

This all looks ok from my memory-reporting POV. cpearce will provide the expertise on the video side. Thanks.

::: content/media/MediaData.cpp
@@ +93,5 @@
> +VideoData::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = aMallocSizeOf(this);
> +
> +  if (mImage && mImage->GetFormat() == ImageFormat::PLANAR_YCBCR) {

It's not obvious to me why you only consider the PLANAR_YCBCR ones. Add a comment?

(Oh, I see you just moved this from elsewhere, so this comment and the one below relate to code that pre-dates your patch. Still, feel free to address if you want.)

@@ +96,5 @@
> +
> +  if (mImage && mImage->GetFormat() == ImageFormat::PLANAR_YCBCR) {
> +    const mozilla::layers::PlanarYCbCrImage* vi =
> +        static_cast<const mozilla::layers::PlanarYCbCrImage*>(mImage.get());
> +    size += vi->SizeOfIncludingThis(aMallocSizeOf);

Nit: |vi| seems like a strange name for this.

::: content/media/MediaDecoderReader.h
@@ +140,2 @@
>  
> +    virtual void* operator()(void* anObject) {

Again, this pre-dates your patch... I see why somebody used "anObject", but really it should be "aObject". "a" is short for "argument"; it's not the indefinite article.
Attachment #8393832 - Flags: review?(n.nethercote) → review+
Comment on attachment 8393832 [details] [diff] [review]
Use MallocSizeOf to reporte decoded-video memory.

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

Thanks for the review! I'll take care of the cleanup and see if I can track down a reason for only using PLANAR_YCBCR. I believe the answer is "Because all video data is PLANAR_YCBR except sometimes in B2G where it's PLANAR_YCBR_GRALLOC."

::: content/media/MediaData.cpp
@@ +93,5 @@
> +VideoData::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = aMallocSizeOf(this);
> +
> +  if (mImage && mImage->GetFormat() == ImageFormat::PLANAR_YCBCR) {

The best I can say is "this fixed a crash on B2G in bug 887968" which I guess is more useful than not. Although maybe that's misleading, b/c we were blindly casting prior to that.

@@ +96,5 @@
> +
> +  if (mImage && mImage->GetFormat() == ImageFormat::PLANAR_YCBCR) {
> +    const mozilla::layers::PlanarYCbCrImage* vi =
> +        static_cast<const mozilla::layers::PlanarYCbCrImage*>(mImage.get());
> +    size += vi->SizeOfIncludingThis(aMallocSizeOf);

I'll update it, it kind of made more sense when there was a |v| variable.

::: content/media/MediaDecoderReader.h
@@ +140,2 @@
>  
> +    virtual void* operator()(void* anObject) {

I'll clean it up.
Comment on attachment 8393832 [details] [diff] [review]
Use MallocSizeOf to reporte decoded-video memory.

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

::: content/media/MediaData.cpp
@@ +93,5 @@
> +VideoData::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = aMallocSizeOf(this);
> +
> +  if (mImage && mImage->GetFormat() == ImageFormat::PLANAR_YCBCR) {

Why not add a virtual SizeOfIncludingThis on mozilla::layers::Image? Then you can at least account for sizeof(Image), right?

Some of the other Image types store their data in GPU surfaces, I assume that's tracked elsewhere?

::: content/media/MediaDecoderReader.h
@@ +131,5 @@
>    // called.
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime);
>  
>    class VideoQueueMemoryFunctor : public nsDequeFunctor {

Move VideoQueueMemoryFunctor's definition into MediaDecoderReader.cpp, and move the body of SizeOfVideoQueue() into MediaDecoderReader.cpp too. This will reduce the amount of stuff in the header file, making it easier to read.

Please do the same for AudioQueueMemoryFunctor while you're here.

@@ +148,3 @@
>    };
>  
> +  size_t SizeOfVideoQueue() {

Please rename the method and add a comment to make it clear that this returns the number of bytes of memory allocated by structures/frames in the video queue, and that this does *not* return a count of the number of video frames we have in our queue.

Please do the same for SizeOfAudioQueue().
Attachment #8393832 - Flags: review?(cpearce) → review+
Comment on attachment 8393832 [details] [diff] [review]
Use MallocSizeOf to reporte decoded-video memory.

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

Thanks for the review Chris, I'll update per your recommendations and have added a follow-up to implement SizeOf* in the Image base class (as well as it's descendants).

::: content/media/MediaData.cpp
@@ +93,5 @@
> +VideoData::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = aMallocSizeOf(this);
> +
> +  if (mImage && mImage->GetFormat() == ImageFormat::PLANAR_YCBCR) {

I agree this is a better solution, for now I'll keep this patch set limited to the goal of reporting decoded video data (which AFAICT is always PLANAR_YCBCR). I've filed a follow-up to implement Image::SizeOf* in Bug 986611.

::: content/media/MediaDecoderReader.h
@@ +131,5 @@
>    // called.
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime);
>  
>    class VideoQueueMemoryFunctor : public nsDequeFunctor {

Will move both of them.

@@ +148,3 @@
>    };
>  
> +  size_t SizeOfVideoQueue() {

I'll rename them to SizeOf{Audio,Video}QueueInBytes to be explicit and add documentation.
Minor naming cleanup, added clarifying comments, moved helper functors into cpp file.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a256c4188213
Attachment #8393832 - Attachment is obsolete: true
Comment on attachment 8395036 [details] [diff] [review]
Use MallocSizeOf to report decoded-video memory.

Try looks good, carrying over r+ from cpearce, njn.
Attachment #8395036 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5942a52e928
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.