958378 - [MediaEncoder] Add memory reporters for MediaRecorder

Status: RESOLVED FIXED

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

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

We can refer to this bug and dump memory usage.
Add memory reporters for Web Audio
https://bugzilla.mozilla.org/show_bug.cgi?id=884368

Comment 1

[Rishab Arora [:spacetime]]

I'm working on this bug, could this be assigned to me?

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi  Rishab,
Thanks, you can start working on it.
BTW, do you have Level 1 access? You may need to find some guy who can be your voucher.
The detail information is listed below.
https://www.mozilla.org/hacking/commit-access-policy/

Comment 3

[Rishab Arora [:spacetime]]

Hi! Thanks for assigning the bug.
I don't have Level 1 access. Unfortunately, I don't know anyone with Level 2+ access who can vouch for me.

Comment 4

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

You can do this bug first and we can start to review your patch without the commit access level.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

You can refer the Bug 884368 - Part 1: Add a memory reporter for AudioContexts

Comment 6

[Rishab Arora [:spacetime]]

Attachment: First attempt at memory reporter for MediaEncoder

Hi rlin,

Sorry for the delay, I was traveling for a couple of weeks. I've started with the patch and here's the progress so far. Could you have a look?

Thanks,
Rishab

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8380848 [details] [diff] [review]
First attempt at memory reporter for MediaEncoder

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

Thanks for your working,
The major problem may related to the sUniqueInstance object.
I think you can study to separate or sum of all the memory usage if possible.

::: content/media/encoder/MediaEncoder.cpp
@@ +45,5 @@
>  namespace mozilla {
>  
> +
> +/* Memory Reporter for MediaEncoder */
> +

nit:Remove this line.

@@ +48,5 @@
> +/* Memory Reporter for MediaEncoder */
> +
> +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> +{
> +    MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)

^^^^ nit:We usually use two spaces as indent.

@@ +51,5 @@
> +{
> +    MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +	MediaEncoder *mEncoder;
> +	virtual ~MediaEncoderReporter();
> +	static StaticRefPtr<MediaEncoderReporter> sUniqueInstance;

This would cause problem because it's possible to create several MediaEncoder instance in the same process..

@@ +76,5 @@
> +    }
> +
> +    NS_METHOD
> +    CollectReports(nsIHandleReportCallback* aHandleReport,
> +                                 nsISupports* aData)

"nsISupports" align to left parenthesis.

@@ +78,5 @@
> +    NS_METHOD
> +    CollectReports(nsIHandleReportCallback* aHandleReport,
> +                                 nsISupports* aData)
> +    {
> +

nits:remove this line

@@ +83,5 @@
> +        int64_t amount = Encoder()->SizeOfIncludingThis(MallocSizeOf);
> +        return MOZ_COLLECT_REPORT("explicit/mediaencoder", KIND_HEAP, UNITS_BYTES,
> +                                  amount, "Memory used by Media Encoder.");
> +    }
> +

nits: remove two lines

@@ +89,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS1(MediaEncoderReporter, nsIMemoryReporter)
> +
> +

remove this line.

@@ +129,5 @@
>  
> +nsRefPtr <MediaEncoderReporter> mediaEncoderReporter;
> +
> +MediaEncoder::MediaEncoder(ContainerWriter* aWriter,
> +						   AudioTrackEncoder* aAudioEncoder,

"AudioTrackEncoder" align to align to left parenthesis.

@@ +391,5 @@
>  
> +size_t
> +MediaEncoder::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +    size_t amount = aMallocSizeOf(this);
> +    return amount; //TODO (spacetime) measure writer, videoEncoder, audioEncoder

We usually don't write our name/todo task on it, if there are any following tasks, you can create new one and just take it.

::: content/media/encoder/MediaEncoder.h
@@ +128,5 @@
>  
>  #ifdef MOZ_OMX_ENCODER
>    static bool IsOMXEncoderEnabled();
>  #endif
> +    

^^^ remove this tab.

@@ +152,5 @@
>      return decodeTime.ToMilliseconds();
>    }
>  };
>  
> +    

^^^ remove this tab

Comment 8

[Rishab Arora [:spacetime]]

Attachment: Patch supporting multiple encoder instances

Thanks for the review! I've hopefully addressed most of the nits. This patch is based on the reporter implementation of MediaDecoder. I'm not sure if this is the best way to measure memory usage, though.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8384771 [details] [diff] [review]
Patch supporting multiple encoder instances

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

Looks good. Thanks for your contribute, I have some comment about this patch, 
If you have time, you can make it more perfect.

::: content/media/encoder/MediaEncoder.cpp
@@ +42,5 @@
>  #define LOG(type, msg)
>  #endif
>  
>  namespace mozilla {
>  

Add some comment on this, including the class/object life cycle, purpose.

@@ +44,5 @@
>  
>  namespace mozilla {
>  
> +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> +{

nit:Usually tag it with :private and move after :public

@@ +46,5 @@
>  
> +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> +{
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +  MediaEncoder *mEncoder;

This class has EncodersArray, why need this?

@@ +50,5 @@
> +  MediaEncoder *mEncoder;
> +  virtual ~MediaEncoderReporter();
> +  static StaticRefPtr<MediaEncoderReporter> sUniqueInstance;
> +  typedef nsTArray<MediaEncoder*> EncodersArray;
> +  static EncodersArray& Encoders() {

nits: Suggest use GetEncoders()

@@ +61,5 @@
> +  MediaEncoderReporter();
> +  static MediaEncoderReporter* UniqueInstance();
> +  void InitMemoryReporter();
> +
> +  static MediaEncoder *Encoder() {

I think you can remove this. :) No one use that.

@@ +91,5 @@
> +    }
> +
> +  #define REPORT(_path, _amount, _desc)                                         \
> +    do {                                                                        \
> +        nsresult rv;                                                            \

nits: use two space only:)

@@ +233,5 @@
>                        audioEncoder != nullptr, videoEncoder != nullptr,
>                        writer != nullptr, mimeType.get()));
>    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
>                               videoEncoder.forget(), mimeType);
> +  MediaEncoderReporter::AddMediaEncoder(encoder);

AddMediaEncoder twice?

@@ +320,5 @@
>        bool isVideoCompleted = (mVideoEncoder && mVideoEncoder->IsEncodingComplete()) || !mVideoEncoder;
>        rv = mWriter->GetContainerData(aOutputBufs,
>                                       isAudioCompleted && isVideoCompleted ?
>                                       ContainerWriter::FLUSH_NEEDED : 0);
> +      mSizeOfBuffer += aOutputBufs->SizeOfIncludingThis(MallocSizeOf);

This buffer wound be consume when exit this switch case, so just use mSizeOfBuffer = to indicate how much buffers used in this class. In the future we would sum of mAudioEncoder nad mVideoEncoder buffer usage on this point.

::: content/media/encoder/MediaEncoder.h
@@ +145,5 @@
>    nsAutoPtr<VideoTrackEncoder> mVideoEncoder;
>    TimeStamp mStartTime;
>    nsString mMIMEType;
>    int mState;
> +  int64_t mSizeOfBuffer;

nits: put int64_t before int, for aligns with class member.

Comment 10

[Rishab Arora [:spacetime]]

Attachment: Corrected patch measuring buffer size

Oops! I accidentally uploaded the patch with the += in the buffer. When testing, the buffer was getting consumed so often I was getting zero usage all the time. 

Is this better?

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8387497 [details] [diff] [review]
Corrected patch measuring buffer size

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

Hi spacetime, 
If you have new patch, you can set review to me and I can see the hint on my bugzilla.
I will review it again later.

Comment 12

[Rishab Arora [:spacetime]]

(In reply to Randy Lin [:rlin] from comment #11)
> Comment on attachment 8387497 [details] [diff] [review]
> Corrected patch measuring buffer size
> 
> Review of attachment 8387497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi spacetime, 
> If you have new patch, you can set review to me and I can see the hint on my
> bugzilla.

Sure! I'll do that from now on.

> I will review it again later.

Are you referring to the last patch (attachment 8387497 [details] [diff] [review])? 


Also, can this patch be checked in? Or does it need more work?

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Usually, We use hg to generate the patch, So it would contain the header with proper format.
like this: https://bug952052.bugzilla.mozilla.org/attachment.cgi?id=8355528
So please update the correct one.

Then, we would use try server to check if any regression happen on this patch. (this require level 1
I push your patch to try server and wait the result on this link. 
https://tbpl.mozilla.o>review  
patch ->review->patch->... got review plus ->test on try server ->check-in in inbound chunk->auto run test on inbound trunk->check-in central.


ps: This document may useful for you. 
https://www.mozilla.org/hacking/commit-access-policy/

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

oh, 
html tag affect my comment. 

I push your patch to try server and wait the result on this link. 
https://tbpl.mozilla.org/?tree=Try&rev=619c33026c2f

The development process would be 
patch ->review->patch->... got review plus ->test on try server ->check-in in inbound chunk->auto run test on inbound trunk->check-in central.

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Could you follow of those steps list on this page and I can be the vouch for you.
http://www.mozilla.org/hacking/committer/

Comment 16

[Rishab Arora [:spacetime]]

I'll fix the header and upload it today. In the meantime, could you vouch for me at bug 981973 ?

Comment 17

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Yes, already vouch.

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

oh, Found some crashes on the try server
https://tbpl.mozilla.org/php/getParsedLog.php?id=35922985&tree=Try
You can try to run locally 

  ./mach mochitest-plain content/media/test/test_mediarecorder_record_4ch_audiocontext.html to reproduce this issue.

Comment 19

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8387497 [details] [diff] [review]
Corrected patch measuring buffer size

still need to fix the crash issues.

Comment 20

[Rishab Arora [:spacetime]]

Attachment: Measuring memory for MediaEncoder

Comment 21

[Rishab Arora [:spacetime]]

Comment on attachment 8443910 [details] [diff] [review]
Measuring memory for MediaEncoder

I've made some more changes. I've begun to measure memory from mAudioEncoder and mVideoEncoder too. Could you have a look?
I had some questions. Can I email them?

Comment 22

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8443910 [details] [diff] [review]
Measuring memory for MediaEncoder

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

Thanks for your contribution. 

Using static to hold this instance would cause sharing problem.
  static StaticRefPtr<MediaEncoderReporter> sUniqueInstance;

We would have the use case that the same web page can have several media recorder objects.
And it would also create the related encoder objects in the same process.


For example, 
If application(js) creates a media recorder (MR1) and start recording, 
application(js) also new another one (MR2) and start recording. If MR1 stop recording, what's in the sUniqueInstance in MR2?

I think you could sum all of the encoder memory usage in the same process.
If you have questions, don't hesitate to post here or mail to me directly.
Thanks.

::: content/media/encoder/MediaEncoder.cpp
@@ +54,5 @@
> + */
> +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

miss NS_DECL_NSIMEMORYREPORTER?

@@ +55,5 @@
> +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  MediaEncoderReporter();

MediaEncoderReporter() {}

@@ +299,5 @@
>        }
>  
>        rv = mWriter->GetContainerData(aOutputBufs,
>                                       ContainerWriter::GET_HEADER);
> +      if (aOutputBufs != nullptr) mSizeOfBuffer = aOutputBufs->SizeOfExcludingThis(MallocSizeOf);

if (aOutputBufs != nullptr) {
  mSizeOfBuffer = aOutputBufs->SizeOfExcludingThis(MallocSizeOf);
}

@@ +334,5 @@
>                                       isAudioCompleted && isVideoCompleted ?
>                                       ContainerWriter::FLUSH_NEEDED : 0);
> +      if (aOutputBufs != nullptr) mSizeOfBuffer = aOutputBufs->SizeOfExcludingThis(MallocSizeOf)
> +          + (mAudioEncoder != nullptr ? mAudioEncoder->SizeOfExcludingThis(MallocSizeOf) : 0)
> +          + (mVideoEncoder != nullptr ? mVideoEncoder->SizeOfExcludingThis(MallocSizeOf) : 0);

if (aOutputBufs != nullptr) {
   mSizeOfBuffer = aOutputBufs->SizeOfExcludingThis(MallocSizeOf) +
                    (mAudioEncoder != nullptr ? mAudioEncoder->SizeOfExcludingThis(MallocSizeOf) : 0) +
                    (mVideoEncoder != nullptr ? mVideoEncoder->SizeOfExcludingThis(MallocSizeOf) : 0);
 }

@@ +443,5 @@
> +  }
> +  return sUniqueInstance;
> +}
> +
> +MediaEncoderReporter::MediaEncoderReporter()

remove this one.

::: content/media/encoder/TrackEncoder.cpp
@@ +7,5 @@
>  #include "MediaStreamGraph.h"
>  #include "VideoUtils.h"
>  
>  #undef LOG
> +#ifdef MOZ_WIDGET_GONK  

space

@@ +140,5 @@
>        aOutput[i * aDuration + j] = aInput[i + j * aChannels];
>      }
>    }
>  }
> +    

tab

@@ +144,5 @@
> +    
> +size_t AudioTrackEncoder::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const{
> +  return mRawSegment.SizeOfExcludingThis(aMallocSizeOf);
> +}
> +    

tab

@@ +253,5 @@
>    memset(aOutputBuffer->Elements(), 0x10, yPlaneLen);
>    // Fill Cb/Cr planes.
>    memset(aOutputBuffer->Elements() + yPlaneLen, 0x80, cbcrPlaneLen);
>  }
> +    

tab

Comment 23

[Rishab Arora [:spacetime]]

(In reply to Randy Lin [:rlin] from comment #22)
> Comment on attachment 8443910 [details] [diff] [review]
> Measuring memory for MediaEncoder
> 
> Using static to hold this instance would cause sharing problem.
>   static StaticRefPtr<MediaEncoderReporter> sUniqueInstance;
> 
> We would have the use case that the same web page can have several media
> recorder objects.
> And it would also create the related encoder objects in the same process.
> 

The way I wished to implement it was that the encoder reporter exists as a
Singleton object. 

I got the idea from MediaDecoder.
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp (line 73)

> 
> For example, 
> If application(js) creates a media recorder (MR1) and start recording, 
> application(js) also new another one (MR2) and start recording. If MR1 stop
> recording, what's in the sUniqueInstance in MR2?

The unique instance is for the reporter only. So there would at most one instance of MediaEncoderReporter. It would register 2 encoders, MR1 and MR2, and then de-register the first one when MR1 stops. 

Will this implementation not work in the way I intended?

> 
> I think you could sum all of the encoder memory usage in the same process.

I think I'm confused about which process you're talking about. I thought every encoder creates a new thread. Could you elaborate?

> If you have questions, don't hesitate to post here or mail to me directly.
Thank you! You've been a great help! :)
> Thanks.
> 
> ::: content/media/encoder/MediaEncoder.cpp
> @@ +54,5 @@
> > + */
> > +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> > +{
> > +public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> miss NS_DECL_NSIMEMORYREPORTER?

I've inherited the declaration for CollectReports() from the class nsIMemoryReporter, adding this creates a conflict.

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Rishab Arora [:spacetime] from comment #23)
> (In reply to Randy Lin [:rlin] from comment #22)
> > Comment on attachment 8443910 [details] [diff] [review]
> > Measuring memory for MediaEncoder
> > 
> > Using static to hold this instance would cause sharing problem.
> >   static StaticRefPtr<MediaEncoderReporter> sUniqueInstance;
> > 
> > We would have the use case that the same web page can have several media
> > recorder objects.
> > And it would also create the related encoder objects in the same process.
> > 
> 
> The way I wished to implement it was that the encoder reporter exists as a
> Singleton object. 
> 
> I got the idea from MediaDecoder.
> http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp
> (line 73)
> 
> > 
> > For example, 
> > If application(js) creates a media recorder (MR1) and start recording, 
> > application(js) also new another one (MR2) and start recording. If MR1 stop
> > recording, what's in the sUniqueInstance in MR2?
> 
> The unique instance is for the reporter only. So there would at most one
> instance of MediaEncoderReporter. It would register 2 encoders, MR1 and MR2,
> and then de-register the first one when MR1 stops. 
OK. I got it.
> Will this implementation not work in the way I intended?
> 
> > 
> > I think you could sum all of the encoder memory usage in the same process.
> 
> I think I'm confused about which process you're talking about. I thought
> every encoder creates a new thread. Could you elaborate?
> 
> > If you have questions, don't hesitate to post here or mail to me directly.
> Thank you! You've been a great help! :)
> > Thanks.
> > 
> > ::: content/media/encoder/MediaEncoder.cpp
> > @@ +54,5 @@
> > > + */
> > > +class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
> > > +{
> > > +public:
> > > +  NS_DECL_THREADSAFE_ISUPPORTS
> > 
> > miss NS_DECL_NSIMEMORYREPORTER?
> 
> I've inherited the declaration for CollectReports() from the class
> nsIMemoryReporter, adding this creates a conflict.
Thanks for clarify that. 

You did a good job about this, thanks.
BTW, I update to today's reversion and you need a small interface change to avoid build break.
========
 0:25.13 /home/randy-lin/gecko_c1/content/media/encoder/MediaEncoder.cpp:55:7: note:   because the following virtual functions are pure within ‘mozilla::MediaEncoderReporter’:
 0:25.13  class MediaEncoderReporter MOZ_FINAL : public nsIMemoryReporter
Need change this interface to:
NS_IMETHOD CollectReports(nsIMemoryReporterCallback *callback, nsISupports *data, bool anonymize) = 0;
========

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8443910 [details] [diff] [review]
Measuring memory for MediaEncoder

Hi roc, could you take a look? thanks.

Comment 26

[Rishab Arora [:spacetime]]

Attachment: Measuring memory for MediaEncoder

Addressed nits. Measure mAudioEncoder and mVideoEncoder memory only when collecting reports. Fixed interface.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8445959 [details] [diff] [review]
Measuring memory for MediaEncoder

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

Why are we tracking MediaEncoders as a separate memory class? Can't we just include MediaEncoder memory usage as part of the memory used by MediaRecorder?

Comment 28

[Rishab Arora [:spacetime]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 8445959 [details] [diff] [review]
> Measuring memory for MediaEncoder
> 
> Review of attachment 8445959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are we tracking MediaEncoders as a separate memory class? Can't we just
> include MediaEncoder memory usage as part of the memory used by
> MediaRecorder?

From what I understand I guess we could; since we're only using MediaEncoders in MediaRecorder. Unless there are plans to use them for something other than MediaRecorder. 
Maybe Randy could shed some light on this?

Comment 29

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi roc, 
Do you want us to collect the memory start from the MediaRecorder? 
i.e.
1.00 MB (100.0%) -- MediaRecorder

= sum of MediaRecorder + MediaEncoder + AudioTrackEncoder + VideoTrackEncoder + EncodedBufferCache ?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes

Comment 31

[Rishab Arora [:spacetime]]

Attachment: Measuring memory for MediaRecorder

I've shifted the reporter to MediaRecorder. Is this what we're looking for?

Comment 32

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8448521 [details] [diff] [review]
Measuring memory for MediaRecorder

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

Thanks for your patch and it can be reviewed by roc.
Hi Roc,
  could you review this again? 
Thanks.

::: content/media/MediaRecorder.cpp
@@ +899,5 @@
>      Stop(result);
>    }
>  }
>  
> +

extra line

@@ +904,5 @@
> +
> +size_t
> +MediaRecorder::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t amount = 42;

What is 42?

::: content/media/encoder/MediaEncoder.cpp
@@ +204,5 @@
>        }
>  
>        rv = mWriter->GetContainerData(aOutputBufs,
>                                       ContainerWriter::GET_HEADER);
> +      if (aOutputBufs != nullptr) {

I think aOutputBufs shouldn't be nullptr.
You would assert this when aOutputBufs == nullptr.

@@ +239,5 @@
>        bool isVideoCompleted = (mVideoEncoder && mVideoEncoder->IsEncodingComplete()) || !mVideoEncoder;
>        rv = mWriter->GetContainerData(aOutputBufs,
>                                       isAudioCompleted && isVideoCompleted ?
>                                       ContainerWriter::FLUSH_NEEDED : 0);
> +      if (aOutputBufs != nullptr) {

The same above.

::: content/media/encoder/MediaEncoder.h
@@ +143,5 @@
>  #endif
>  
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +  /*
> +   * Measure the size of the buffer, and memory occupied by mAudioEncoder

Measure the size of the mRawSegment

@@ +157,5 @@
>    nsAutoPtr<ContainerWriter> mWriter;
>    nsAutoPtr<AudioTrackEncoder> mAudioEncoder;
>    nsAutoPtr<VideoTrackEncoder> mVideoEncoder;
>    TimeStamp mStartTime;
>    nsString mMIMEType;

Could you add a comment to describe which part of memory in trackEncoder has been calculated?

Comment 33

[Rishab Arora [:spacetime]]

(In reply to Randy Lin [:rlin] from comment #32)
> Comment on attachment 8448521 [details] [diff] [review]
> Measuring memory for MediaRecorder
> 
> Review of attachment 8448521 [details] [diff] [review]:
> -----------------------------------------------------------------

> What is 42?

I accidentally left that in. It's supposed to be 0. I'll fix this.

> 
> ::: content/media/encoder/MediaEncoder.cpp
> @@ +204,5 @@
> >        }
> >  
> >        rv = mWriter->GetContainerData(aOutputBufs,
> >                                       ContainerWriter::GET_HEADER);
> > +      if (aOutputBufs != nullptr) {
> 
> I think aOutputBufs shouldn't be nullptr.
> You would assert this when aOutputBufs == nullptr.

I didn't quite understand this one. Are you saying I should add an assertion verifying it's not nullptr. .i.e. MOZ_ASSERT(aOutputBufs != nullptr);

> 
> @@ +239,5 @@
> >        bool isVideoCompleted = (mVideoEncoder && mVideoEncoder->IsEncodingComplete()) || !mVideoEncoder;
> >        rv = mWriter->GetContainerData(aOutputBufs,
> >                                       isAudioCompleted && isVideoCompleted ?
> >                                       ContainerWriter::FLUSH_NEEDED : 0);
> > +      if (aOutputBufs != nullptr) {
> 
> The same above.
> 
> ::: content/media/encoder/MediaEncoder.h
> @@ +143,5 @@
> >  #endif
> >  
> > +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> > +  /*
> > +   * Measure the size of the buffer, and memory occupied by mAudioEncoder
> 
> Measure the size of the mRawSegment

Should I include this information here? It's the implementation of SizeOfExcludingThis in TrackEncoder that measures mRawSegment; MediaEncoder doesn't have mRawSegment.

> 
> @@ +157,5 @@
> >    nsAutoPtr<ContainerWriter> mWriter;
> >    nsAutoPtr<AudioTrackEncoder> mAudioEncoder;
> >    nsAutoPtr<VideoTrackEncoder> mVideoEncoder;
> >    TimeStamp mStartTime;
> >    nsString mMIMEType;
> 
> Could you add a comment to describe which part of memory in trackEncoder has
> been calculated?

I've added that in TrackEncoder.h " * Measure size of mRawSegment"

Comment 34

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Rishab Arora [:spacetime] from comment #33)
> (In reply to Randy Lin [:rlin] from comment #32)
> > Comment on attachment 8448521 [details] [diff] [review]
> > Measuring memory for MediaRecorder
> > 
> > Review of attachment 8448521 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> > What is 42?
> 
> I accidentally left that in. It's supposed to be 0. I'll fix this.
> 
> > 
> > ::: content/media/encoder/MediaEncoder.cpp
> > @@ +204,5 @@
> > >        }
> > >  
> > >        rv = mWriter->GetContainerData(aOutputBufs,
> > >                                       ContainerWriter::GET_HEADER);
> > > +      if (aOutputBufs != nullptr) {
> > 
> > I think aOutputBufs shouldn't be nullptr.
> > You would assert this when aOutputBufs == nullptr.
> 
> I didn't quite understand this one. Are you saying I should add an assertion
> verifying it's not nullptr. .i.e. MOZ_ASSERT(aOutputBufs != nullptr);
The aOutputBufs is created by this line
  https://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#310
It shouldn't be nullptr...
and you can add MOZ_ASSERT at 
  https://mxr.mozilla.org/mozilla-central/source/content/media/encoder/MediaEncoder.cpp#186

> > 
> > @@ +239,5 @@
> > >        bool isVideoCompleted = (mVideoEncoder && mVideoEncoder->IsEncodingComplete()) || !mVideoEncoder;
> > >        rv = mWriter->GetContainerData(aOutputBufs,
> > >                                       isAudioCompleted && isVideoCompleted ?
> > >                                       ContainerWriter::FLUSH_NEEDED : 0);
> > > +      if (aOutputBufs != nullptr) {
> > 
> > The same above.
> > 
> > ::: content/media/encoder/MediaEncoder.h
> > @@ +143,5 @@
> > >  #endif
> > >  
> > > +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> > > +  /*
> > > +   * Measure the size of the buffer, and memory occupied by mAudioEncoder
> > 
> > Measure the size of the mRawSegment
> 
> Should I include this information here? It's the implementation of
> SizeOfExcludingThis in TrackEncoder that measures mRawSegment; MediaEncoder
> doesn't have mRawSegment.
oh, I check the wrong file, you are right.
> > 
> > @@ +157,5 @@
> > >    nsAutoPtr<ContainerWriter> mWriter;
> > >    nsAutoPtr<AudioTrackEncoder> mAudioEncoder;
> > >    nsAutoPtr<VideoTrackEncoder> mVideoEncoder;
> > >    TimeStamp mStartTime;
> > >    nsString mMIMEType;
> > 
> > Could you add a comment to describe which part of memory in trackEncoder has
> > been calculated?
> 
> I've added that in TrackEncoder.h " * Measure size of mRawSegment"
Thanks.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8448521 [details] [diff] [review]
Measuring memory for MediaRecorder

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

::: content/media/MediaRecorder.cpp
@@ +37,5 @@
> + * MediaRecorderReporter measures memory being used by the Media Recorder.
> + *
> + * It is a singleton reporter and the single class object lives as long as at
> + * least one Recorder is registered. In MediaRecorder, the reporter is unregistered
> + * when it is destroyed.

Why don't we just make MediaRecorder implement nsIMemoryReporter?

::: content/media/encoder/MediaEncoder.h
@@ +158,5 @@
>    nsAutoPtr<AudioTrackEncoder> mAudioEncoder;
>    nsAutoPtr<VideoTrackEncoder> mVideoEncoder;
>    TimeStamp mStartTime;
>    nsString mMIMEType;
> +  int64_t mSizeOfBuffer;

Please explain why you need this. Generally whoever's holding a pointer to the buffer (which buffer?) should be able to measure its size and report it without storing the size explicitly.

Comment 36

[Rishab Arora [:spacetime]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> Comment on attachment 8448521 [details] [diff] [review]
> Measuring memory for MediaRecorder
> 
> Review of attachment 8448521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaRecorder.cpp
> @@ +37,5 @@
> > + * MediaRecorderReporter measures memory being used by the Media Recorder.
> > + *
> > + * It is a singleton reporter and the single class object lives as long as at
> > + * least one Recorder is registered. In MediaRecorder, the reporter is unregistered
> > + * when it is destroyed.
> 
> Why don't we just make MediaRecorder implement nsIMemoryReporter?

I could give it a try. I had trouble making MediaEncoder implement nsIMemoryReporter (The exact problem escapes me, I tried that in February). Why would we want it that way, though? I don't think I understand when a separate reporter class makes sense either (Though I feel it's cleaner and the MediaRecorder class shouldn't be responsible for reporting memory). 

> 
> ::: content/media/encoder/MediaEncoder.h
> @@ +158,5 @@
> >    nsAutoPtr<AudioTrackEncoder> mAudioEncoder;
> >    nsAutoPtr<VideoTrackEncoder> mVideoEncoder;
> >    TimeStamp mStartTime;
> >    nsString mMIMEType;
> > +  int64_t mSizeOfBuffer;
> 
> Please explain why you need this. Generally whoever's holding a pointer to
> the buffer (which buffer?) should be able to measure its size and report it
> without storing the size explicitly.

The intent was to measure what the contents of the 'aOutputBufs' buffer. It should probably be renamed to mSizeOfOutputBufs. Since 'aOutputBuf's is a local variable inside GetEncodedData()'s state machine and the memory is measured on a separate thread(?), I chose to update a member variable.

Comment 37

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

Carry original author, reviewer, re-base. 
try result:https://tbpl.mozilla.org/?tree=Try&rev=81aedc0680e3

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c15fcd00145

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c15fcd00145

Comment 40

[Rishab Arora [:spacetime]]

Thanks, people!