[MediaEncoder] Add memory reporters for MediaRecorder

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rlin, Assigned: spacetime, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor-lang=zh])

Attachments

(2 attachments, 5 obsolete attachments)

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
Whiteboard: [good first bug][mentor=rlin][mentor-lang=zh]
Assignee

Comment 1

5 years ago
I'm working on this bug, could this be assigned to me?
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/
Assignee: nobody → ra.rishab
Assignee

Comment 3

5 years ago
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.
You can do this bug first and we can start to review your patch without the commit access level.
You can refer the Bug 884368 - Part 1: Add a memory reporter for AudioContexts
Component: Video/Audio → Video/Audio: Recording
Assignee

Comment 6

5 years ago
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 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
Assignee

Comment 8

5 years ago
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.
Attachment #8380848 - Attachment is obsolete: true
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.
Assignee

Comment 10

5 years ago
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?
Attachment #8384771 - Attachment is obsolete: true
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.
Attachment #8387497 - Flags: review?(rlin)
Assignee

Comment 12

5 years ago
(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?
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/
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.
Could you follow of those steps list on this page and I can be the vouch for you.
http://www.mozilla.org/hacking/committer/
Assignee

Comment 16

5 years ago
I'll fix the header and upload it today. In the meantime, could you vouch for me at bug 981973 ?
Yes, already vouch.
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 on attachment 8387497 [details] [diff] [review]
Corrected patch measuring buffer size

still need to fix the crash issues.
Attachment #8387497 - Flags: review?(rlin) → feedback+
Mentor: rlin
Whiteboard: [good first bug][mentor=rlin][mentor-lang=zh] → [good first bug][mentor-lang=zh]
Assignee

Comment 20

5 years ago
Attachment #8387497 - Attachment is obsolete: true
Assignee

Comment 21

5 years ago
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?
Attachment #8443910 - Flags: review?(rlin)
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
Attachment #8443910 - Flags: review?(rlin)
Assignee

Comment 23

5 years ago
(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.
(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 on attachment 8443910 [details] [diff] [review]
Measuring memory for MediaEncoder

Hi roc, could you take a look? thanks.
Attachment #8443910 - Flags: review?(roc)
Assignee

Comment 26

5 years ago
Addressed nits. Measure mAudioEncoder and mVideoEncoder memory only when collecting reports. Fixed interface.
Attachment #8443910 - Attachment is obsolete: true
Attachment #8443910 - Flags: review?(roc)
Attachment #8445959 - Flags: review?(roc)
Attachment #8445959 - Attachment is patch: true
Attachment #8445959 - Attachment mime type: text/x-patch → text/plain
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?
Attachment #8445959 - Flags: review?(roc)
Assignee

Comment 28

5 years ago
(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?
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 ?
Flags: needinfo?(roc)
Summary: [MediaEncoder] Add memory reporters for MediaEncoder framework → [MediaEncoder] Add memory reporters for MediaRecorder
Assignee

Comment 31

5 years ago
I've shifted the reporter to MediaRecorder. Is this what we're looking for?
Attachment #8445959 - Attachment is obsolete: true
Attachment #8448521 - Flags: review?(rlin)
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?
Attachment #8448521 - Flags: review?(roc)
Attachment #8448521 - Flags: review?(rlin)
Attachment #8448521 - Flags: feedback+
Assignee

Comment 33

5 years ago
(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"
(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 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.
Attachment #8448521 - Flags: review?(roc) → review+
Assignee

Comment 36

5 years ago
(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.
Carry original author, reviewer, re-base. 
try result:https://tbpl.mozilla.org/?tree=Try&rev=81aedc0680e3
https://hg.mozilla.org/mozilla-central/rev/0c15fcd00145
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee

Comment 40

5 years ago
Thanks, people!
You need to log in before you can comment on or make changes to this bug.