Closed Bug 1190592 Opened 5 years ago Closed 4 years ago

Add memory reporter to MP4Demuxer

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 1189194 shows a large amount of heap-unclassified related to MP4Demuxer, we should add a reporter for that.
All of the unreported DMD allocation stacks look something like:

>    #05: mp4_demuxer::SampleIterator::GetNext() (nsRefPtr.h:98, in XUL)
>    #06: mozilla::MP4TrackDemuxer::GetSamples(int) (AlreadyAddRefed.h:116, in XUL)
>    #07: mozilla::TrackBuffersManager::DoDemuxAudio() (nsRefPtr.h:231, in XUL)
>    #08: mozilla::TrackBuffersManager::CodedFrameProcessing() (nsRefPtr.h:89, in XUL)
>    #09: mozilla::TrackBuffersManager::SegmentParserLoop() (TrackBuffersManager.cpp:699, in XUL)
>    #10: mozilla::TrackBuffersManager::InitSegmentParserLoop() (TrackBuffersManager.cpp:601, in XUL)

and

>    #05: mozilla::MediaRawData::EnsureCapacity(unsigned long) (nsTArray.h:117, in XUL)
>    #06: mozilla::MediaRawDataWriter::SetSize(unsigned long) (MediaData.cpp:613, in XUL)
>    #07: mp4_demuxer::SampleIterator::GetNext() (Index.cpp:110, in XUL)
>    #08: mozilla::MP4TrackDemuxer::GetSamples(int) (AlreadyAddRefed.h:116, in XUL)
>    #09: mozilla::TrackBuffersManager::DoDemuxVideo() (nsRefPtr.h:231, in XUL)
>    #10: mozilla::TrackBuffersManager::CodedFrameProcessing() (nsRefPtr.h:89, in XUL)
>    #11: mozilla::TrackBuffersManager::SegmentParserLoop() (TrackBuffersManager.cpp:699, in XUL)
>    #12: mozilla::TrackBuffersManager::InitSegmentParserLoop() (TrackBuffersManager.cpp:601, in XUL)

Indicating that TrackBuffersManager is probably the more interesting place to start.
TrackBuffersManager is what holds the data, so yes it will be the biggest.

The core problem however appears to be that the MediaDecoder::Shutdown() is never called
> The core problem however appears to be that the MediaDecoder::Shutdown() is
> never called

This bug is about writing memory reporters for the media data structures. The lack of the Shutdown() call should be fixed in a separate bug.
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> TrackBuffersManager is what holds the data, so yes it will be the biggest.
> 
> The core problem however appears to be that the MediaDecoder::Shutdown() is
> never called

Okay, so the trick here is to determine where to report the TrackBuffersManager memory. I see it's generally owned by a SourceBuffer (and can reach an HTMLElement if we go far enough back).

Jean-Yves is it somehow reachable from MediaDecoder and would it make more sense to report from there? I see MediaShutdownManager has refs to all (?) decoders so if we can connect the two that would be the easiest place to add a reporter.
Flags: needinfo?(jyavenard)
The TrackBuffersManager keeps constant record on the size of the data it holds.
However it provides no thread-safe way to report that data ; it wouldn't be complicated nor long to do that.

We currently have an interface provided by the about:media plugin that let you access that size.

Now, where this would be reported I don't really know.
The thing is that you can't add data to a sourcebuffer until the parent mediasource is attached to a media element.

So my guess would be to report this as part of a HTMLMediaElement report (if you have that)
Flags: needinfo?(jyavenard)
AIUI the expected size of each MediaRawData is up to 100 MiB of sample video data and 15 MiB of sample audio data, per video. With jemalloc rounding up request sizes the actual memory used can be a bit higher (mostly for video, where there's about 21% overhead). And if you have multiple videos playing it'll go higher.

So having a reporter for this is still a good idea.
Currently both audio and video can reach 100MB ; reducing the memory threshold for audio only is tracked in bug 1191712.
Attached patch Add mediasource memory reporter (obsolete) — Splinter Review
A memory reporter is added to HTMLMediaElement primarily to report on the
buffers used by MSE.

Output ends up accounted for under dom/element-nodes:

863.22 MB (100.0%) -- explicit
├──584.07 MB (67.66%) -- window-objects
│  ├──571.70 MB (66.23%) -- top(https://www.youtube.com/watch?v=4Z4j2CrJRn4, id=7)
│  │  ├──567.44 MB (65.74%) -- active
│  │  │  ├──567.24 MB (65.71%) -- window(https://www.youtube.com/watch?v=4Z4j2CrJRn4)
│  │  │  │  ├──537.29 MB (62.24%) -- dom
│  │  │  │  │  ├──536.93 MB (62.20%) ── element-nodes

If we don't want to do that we'll need to break out every element node type
and update all SizeOf functions that derive from nsINode.
Attachment #8645264 - Flags: feedback?(n.nethercote)
Note: I tested this on OS X on youtube. DMD confirms that there is no double reporting.
Comment on attachment 8645264 [details] [diff] [review]
Add mediasource memory reporter

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

My $0.02 :)

::: dom/media/mediasource/MediaSource.cpp
@@ +150,5 @@
> +
> +  size += mSourceBuffers->SizeOfIncludingThis(aMallocSizeOf);
> +
> +  // NB: The elements of mActiveSourceBuffers are all in mSourceBuffers
> +  size += mActiveSourceBuffers->ShallowSizeOfIncludingThis(aMallocSizeOf);

won't this be accounting the size of the sourcebuffer list twice ?

the source buffer list contains refPtr ; hardly worth worrying about.

::: dom/media/mediasource/TrackBuffersManager.h
@@ +97,5 @@
>                                             bool& aError);
>    media::TimeUnit GetNextRandomAccessPoint(TrackInfo::TrackType aTrack);
>  
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

Where will this function be called from ?

If this from the main thread, it won't be thread safe.

Almost every functions called within TrackBuffersManager is designed to run its own task queue ; and all data members are designed to only be accessed from that task queue (it was all designed to be monitor free, except for very selected data member)

We could however keep a thread-safe member that is updated whenever data is added (or removed) and only modified at those time

@@ +277,5 @@
> +
> +      // According to DMD the Most prominent memory usage is in TrackBuffers.
> +      // In the future, if warranted by DMD, we might want to measure the
> +      // following:
> +      //   - mDemuxer (probably not owned)

A 3 hours youtube video, shows that for audio there's a single MP4 demuxer in use, which keeps an internal array of all the samples it has ever parsed. It's highly inefficient usage as it will never need to access those records.
And the size of this samples array climb close to 30MB.

@@ +280,5 @@
> +      // following:
> +      //   - mDemuxer (probably not owned)
> +      //   - mDemuxRequest
> +      //   - mInfo
> +      //   - mLastInfo

those are tiny; < 100 bytes

@@ +282,5 @@
> +      //   - mDemuxRequest
> +      //   - mInfo
> +      //   - mLastInfo
> +
> +      size += mQueuedSamples.ShallowSizeOfExcludingThis(aMallocSizeOf);

mQueuedSamples is extremely shortlived ; and under most circumstances would be empty (unless you access it in a non threadsafe manner)

@@ +288,5 @@
> +        size += data->SizeOfIncludingThis(aMallocSizeOf);
> +      }
> +
> +      size += mBuffers.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +      for (auto& buffer : mBuffers) {

should probably be const auto&
Comment on attachment 8645264 [details] [diff] [review]
Add mediasource memory reporter

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

This basically looks good except the thread-safety needs to be worked out. There's been talk in the past of adding an async reporter class for this kind of case. You'd have a time-out and require that the thread get back to you before the timer runs out. It's not been implemented though there is something very much like that used for getting reports from other processes, e.g. see nsMemoryReporterManager::GetReportsState. (I understand that implementing that would be massive scope creep for this bug...)

::: dom/media/mediasource/SourceBuffer.h
@@ +114,5 @@
>      MonitorAutoLock mon(mMonitor);
>      mAppendMode = aAppendMode;
>    }
>  
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Nit: opening brace on its own line.

@@ +218,5 @@
>    }
>  
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
> +
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Ditto.

::: dom/media/mediasource/SourceBufferList.h
@@ +88,5 @@
>    void Dump(const char* aPath);
>  #endif
>  
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
> +  size_t ShallowSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {

Ditto.

::: dom/media/mediasource/TrackBuffer.h
@@ +119,5 @@
>    void Dump(const char* aPath) override;
>  #endif
>  
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override {
> +    // NB: This is currently just a stub, in the even that DMD indicates

Nit: "even" -> "event"

@@ +124,5 @@
> +    //     significant usage a full implementation can be added.
> +    return 0;
> +  }
> +
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override {

Brace on its own line again. Several more instances below; I won't bother marking them all.

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +130,5 @@
> +TrackBuffersManager::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = 0;
> +
> +  // NB: There are many other members that could be included, currenlty DMD

"currently"
Attachment #8645264 - Flags: feedback?(n.nethercote) → feedback+
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Comment on attachment 8645264 [details] [diff] [review]
> Add mediasource memory reporter
> 
> Review of attachment 8645264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My $0.02 :)

Thank you for the quick response, your feedback is much appreciated. Clearly figuring out the multithreading issue is the most important point before we can move forward.

> ::: dom/media/mediasource/MediaSource.cpp
> @@ +150,5 @@
> > +
> > +  size += mSourceBuffers->SizeOfIncludingThis(aMallocSizeOf);
> > +
> > +  // NB: The elements of mActiveSourceBuffers are all in mSourceBuffers
> > +  size += mActiveSourceBuffers->ShallowSizeOfIncludingThis(aMallocSizeOf);
> 
> won't this be accounting the size of the sourcebuffer list twice ?

They're different objects (AFAICT), so no.

> the source buffer list contains refPtr ; hardly worth worrying about.

That might be true, I don't think it hurts to measure in this case.

> ::: dom/media/mediasource/TrackBuffersManager.h
> @@ +97,5 @@
> >                                             bool& aError);
> >    media::TimeUnit GetNextRandomAccessPoint(TrackInfo::TrackType aTrack);
> >  
> > +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override {
> > +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> 
> Where will this function be called from ?
> 
> If this from the main thread, it won't be thread safe.

Yes, all memory reporting is performed on the main thread. 

> Almost every functions called within TrackBuffersManager is designed to run
> its own task queue ; and all data members are designed to only be accessed
> from that task queue (it was all designed to be monitor free, except for
> very selected data member)

Would it be reasonable to post a "measure your memory" task and block on the response in the main thread? Generally we accept that measuring memory can have a user impact, so blocking isn't the end of the world (we do this in the webaudio reporter).

> We could however keep a thread-safe member that is updated whenever data is
> added (or removed) and only modified at those time

Generally we prefer to avoid counters (it also messes up DMD which tracks MallocSizeOf invocations).

> @@ +277,5 @@
> > +
> > +      // According to DMD the Most prominent memory usage is in TrackBuffers.
> > +      // In the future, if warranted by DMD, we might want to measure the
> > +      // following:
> > +      //   - mDemuxer (probably not owned)
> 
> A 3 hours youtube video, shows that for audio there's a single MP4 demuxer
> in use, which keeps an internal array of all the samples it has ever parsed.
> It's highly inefficient usage as it will never need to access those records.
> And the size of this samples array climb close to 30MB.

Is that demuxer shared or is it owned by the TrackData instance? We'd need to make sure we measure it only one time, so choosing a defacto owner is important.

> @@ +280,5 @@
> > +      // following:
> > +      //   - mDemuxer (probably not owned)
> > +      //   - mDemuxRequest
> > +      //   - mInfo
> > +      //   - mLastInfo
> 
> those are tiny; < 100 bytes

Good to know, I'll remove them from the list.

> @@ +282,5 @@
> > +      //   - mDemuxRequest
> > +      //   - mInfo
> > +      //   - mLastInfo
> > +
> > +      size += mQueuedSamples.ShallowSizeOfExcludingThis(aMallocSizeOf);
> 
> mQueuedSamples is extremely shortlived ; and under most circumstances would
> be empty (unless you access it in a non threadsafe manner)

Hopefully that is the case, but it probably doesn't hurt to measure it just in case.

> @@ +288,5 @@
> > +        size += data->SizeOfIncludingThis(aMallocSizeOf);
> > +      }
> > +
> > +      size += mBuffers.ShallowSizeOfExcludingThis(aMallocSizeOf);
> > +      for (auto& buffer : mBuffers) {
> 
> should probably be const auto&

The function itself is const, but it certainly wouldn't hurt to be more explicit. I'll change that.
(In reply to Nicholas Nethercote [:njn] from comment #11)
> Comment on attachment 8645264 [details] [diff] [review]
> Add mediasource memory reporter
> 
> Review of attachment 8645264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This basically looks good except the thread-safety needs to be worked out.
> There's been talk in the past of adding an async reporter class for this
> kind of case. You'd have a time-out and require that the thread get back to
> you before the timer runs out. It's not been implemented though there is
> something very much like that used for getting reports from other processes,
> e.g. see nsMemoryReporterManager::GetReportsState. (I understand that
> implementing that would be massive scope creep for this bug...)

That would definitely be a good follow up. I think it would be useful on the webaudio side as well, so it might make sense to hold off and think about it in a more general sense. On the other hand if we can't implement this reporter without it we may need to file a blocking bug and do that first.

I'll cleanup the style nits / typos you noted as well.
Posting to the task queue and waiting for a response doesn't seem like it's going to work. The best I can tell, I try to dispatch, wait on a condition on the main thread, and then the task queue ends up blocking on the main thread before it runs the task I dispatched.
Depends on: 1194555
I don't see the problem in having the TrackBuffersManager performs this calculation when it updates the data and keep the results in a thread-safe member.

If you're accessing mQueuedSamples outside the trackbuffer's task queue, its size will always be empty.

It's extremely short lived and only used to keep the frame coming out of the demuxer ; and immediately after it's emptied to fill the trackbuffer's vector
(In reply to Jean-Yves Avenard [:jya] from comment #15)
> I don't see the problem in having the TrackBuffersManager performs this
> calculation when it updates the data and keep the results in a thread-safe
> member.
> 
> If you're accessing mQueuedSamples outside the trackbuffer's task queue, its
> size will always be empty.
> 
> It's extremely short lived and only used to keep the frame coming out of the
> demuxer ; and immediately after it's emptied to fill the trackbuffer's vector

DMD [1] expects the MallocSizeOf calls to happen during memory reporting. Accumulating in a value and tallying that after the fact confuses DMD.

On the other hand if I can access mBuffers on the main thread and ignore mQueuedSamples then we can avoid the complexity and call it good. Let me know if mBuffers is main thread safe.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD
Flags: needinfo?(jyavenard)
I'm only suggesting to perform the cache of parsing the arrays in TrackBuffersManagers.cpp (everything else run on the mainthread already).

Potentially iterating through a nsTArray on two different threads at the same time is going to really mess things up. Especially when one thread is going to modify that array.

And I would much better not introduce a big monitor in a design specifically crafted so monitors aren't necessary.

access to mBuffers isn't thread-safe... every access to it (either read or write) is done via queuing a task on a dedicated taskqueue.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #17)
> I'm only suggesting to perform the cache of parsing the arrays in
> TrackBuffersManagers.cpp (everything else run on the mainthread already).
> 
> Potentially iterating through a nsTArray on two different threads at the
> same time is going to really mess things up. Especially when one thread is
> going to modify that array.
> 
> And I would much better not introduce a big monitor in a design specifically
> crafted so monitors aren't necessary.
> 
> access to mBuffers isn't thread-safe... every access to it (either read or
> write) is done via queuing a task on a dedicated taskqueue.

Unfortunately |mBuffers| is where all the memory is. Bug 1194555 will allow use to do asynchronous memory reporting; once that is complete we can continue here.
Duplicate of this bug: 1196965
Component: Audio/Video → Audio/Video: Playback
Attached patch Add mediasource memory reporter (obsolete) — Splinter Review
This adds asyncrounous reporting of MSE resources. It's partially based on Matt Woodrow's work in bug 1196965.

Reporting format is as follows:
216.28 MB (100.0%) -- explicit
├───70.17 MB (32.45%) ++ window-objects
├───58.78 MB (27.18%) -- media
│   ├──58.78 MB (27.18%) ── resources
│   └───0.00 MB (00.00%) -- (4 tiny)
│       ├──0.00 MB (00.00%) ++ decoded
│       ├──0.00 MB (00.00%) ── libnestegg
│       ├──0.00 MB (00.00%) ── libogg
│       └──0.00 MB (00.00%) ── libvpx

This patch is also dependent on the patches from bug 1194555.
Attachment #8667595 - Flags: review?(jyavenard)
Attachment #8645264 - Attachment is obsolete: true
Comment on attachment 8667595 [details] [diff] [review]
Add mediasource memory reporter

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

r=me with changing ResourceSizes to be ref counted and performing the report on the last release instead (there's a macro for that)

::: dom/media/MediaDecoder.cpp
@@ +1548,5 @@
>    int64_t video = 0, audio = 0;
> +
> +  nsCOMPtr<nsIHandleReportCallback> handleReport = aHandleReport;
> +  nsCOMPtr<nsISupports> data = aData;
> +  auto resourceSizes = new MediaDecoder::ResourceSizes(MediaMemoryTracker::MallocSizeOf);

that looks dangerous to me ; and how that itself could easily leak if we missed one.
Can't ResourceSizes be made a ref counted object?

@@ +1555,5 @@
> +      handleReport->Callback(EmptyCString(), NS_LITERAL_CSTRING("explicit/media/resources"),
> +                             KIND_HEAP, UNITS_BYTES, resourceSizes->mByteSize,
> +                             NS_LITERAL_CSTRING("Memory used by media resources including streaming buffers, caches, etc."),
> +                             data);
> +      delete resourceSizes;

if refcounted this could then be more elegantly handled by using the various mechanism on the last Release()

::: dom/media/MediaDecoder.h
@@ +548,5 @@
> +      : mMallocSizeOf(aMallocSizeOf)
> +      , mByteSize(0)
> +      , mPendingResources(0)
> +      , mCallback()
> +    {

Can you make this a refcounted object.

Especially as it's shared by many and deleted on its last use.

::: dom/media/mediasource/MediaSourceDecoder.cpp
@@ +178,5 @@
>  }
>  
>  void
> +MediaSourceDecoder::AddSizeOfResources(ResourceSizes* aSizes)
> +{

Please have an assertion to guarantee on which thread this is supposed to run.
Attachment #8667595 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #21)
> Comment on attachment 8667595 [details] [diff] [review]
> Add mediasource memory reporter
> 
> Review of attachment 8667595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with changing ResourceSizes to be ref counted and performing the report
> on the last release instead (there's a macro for that)

This seems like a good idea, but see my note below. Any suggestion on how to work around that would be much appreciated.

> ::: dom/media/MediaDecoder.cpp
> @@ +1548,5 @@
> >    int64_t video = 0, audio = 0;
> > +
> > +  nsCOMPtr<nsIHandleReportCallback> handleReport = aHandleReport;
> > +  nsCOMPtr<nsISupports> data = aData;
> > +  auto resourceSizes = new MediaDecoder::ResourceSizes(MediaMemoryTracker::MallocSizeOf);
> 
> that looks dangerous to me ; and how that itself could easily leak if we
> missed one.
> Can't ResourceSizes be made a ref counted object?
> 
> @@ +1555,5 @@
> > +      handleReport->Callback(EmptyCString(), NS_LITERAL_CSTRING("explicit/media/resources"),
> > +                             KIND_HEAP, UNITS_BYTES, resourceSizes->mByteSize,
> > +                             NS_LITERAL_CSTRING("Memory used by media resources including streaming buffers, caches, etc."),
> > +                             data);
> > +      delete resourceSizes;
> 
> if refcounted this could then be more elegantly handled by using the various
> mechanism on the last Release()

We would need a ref to |resourceSizes| in the lambda, which would mean resourceSizes would never be released (b/c the lambda would only be called on release). I'm not sure how to work around that.

> ::: dom/media/mediasource/MediaSourceDecoder.cpp
> @@ +178,5 @@
> >  }
> >  
> >  void
> > +MediaSourceDecoder::AddSizeOfResources(ResourceSizes* aSizes)
> > +{
> 
> Please have an assertion to guarantee on which thread this is supposed to
> run.

I'll add the assertion.
Attached patch Add mediasource memory reporter (obsolete) — Splinter Review
Updates per review comments. Main change is ResourceSizes is ref counted and
uses a completion promise rather than a runnable to signal completion.
Attachment #8671054 - Flags: review?(jyavenard)
Attachment #8667595 - Attachment is obsolete: true
Attachment #8671095 - Flags: review?(jyavenard)
Comment on attachment 8671095 [details] [diff] [review]
Add test for mediasource memory reporter

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

::: dom/media/mediasource/test/test_MediaSource_memory_reporting.html
@@ +19,5 @@
> +    fetchWithXHR("seek.webm", (arrayBuffer) => {
> +      loadSegment(sb, arrayBuffer).then(() => ms.endOfStream());
> +    });
> +
> +    once(sb, "updateend", () => v.play());

you can replace it all with something like:
    fetchAndLoad(sb, 'seek', [''], '.webm').then(() => v.play());
    once(sb, "updateend", () => ms.endOfStream());

and continue on with... (it will have be moved up)

    // Test that memory reporting works now that we've played a video.
    once(v, "ended", () => {
      ...

you don't have to wait for updateend to start playback (two will be fired: one at the end of appendBuffer, and one after endOfStream())

And you could even get rid of the "once(sb, "updateend", () => ms.endOfStream());" by waiting on the "stalled" event instead of "ended"
Attachment #8671095 - Flags: review?(jyavenard) → review+
Comment on attachment 8671054 [details] [diff] [review]
Add mediasource memory reporter

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

r=me with comment addressed.

Bobby, what do you think of using MozPromise::Private directly ?

::: dom/media/MediaDecoder.h
@@ +560,5 @@
> +
> +public:
> +    mozilla::MallocSizeOf mMallocSizeOf;
> +    mozilla::Atomic<size_t> mByteSize;
> +    nsRefPtr<CompletionPromise::Private> mCallback;

You should use a MozPromiseRequestHolder / MozPromiseHolder<CompletionPromise> pair instead

So you can more strongly assert that everything has run its course properly.

So instead of the client accessing the promise::private directly, you access the MozPromiseRequestHolder instead, and ~ResourceSizes access the MozPromiseHolder.

I don't think bholley ever intended for MozPromise::Private to be used publicly, hence the name :)

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +2126,5 @@
>  }
>  
> +void
> +TrackBuffersManager::TrackData::AddSizeOfResources(MediaSourceDecoder::ResourceSizes* aSizes)
> +{

MOZ_ASSERT(OnTaskQueue()); I know it's redundant, but we have this in all functions regarless
Attachment #8671054 - Flags: review?(jyavenard)
Attachment #8671054 - Flags: review+
Attachment #8671054 - Flags: feedback?(bobbyholley)
Comment on attachment 8671054 [details] [diff] [review]
Add mediasource memory reporter

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

::: dom/media/MediaDecoder.h
@@ +560,5 @@
> +
> +public:
> +    mozilla::MallocSizeOf mMallocSizeOf;
> +    mozilla::Atomic<size_t> mByteSize;
> +    nsRefPtr<CompletionPromise::Private> mCallback;

The point of MozPromise::Private is that the consumer that ->Then()s a Promise shouldn't be allowed to resolve/reject it (JS promises accomplish this with a closure). In this case, you should probably make mCallback Private (ideally with one of the *Holder classes) and add an accessor that returns an already_AddRefed<CompletionPromise>.

On that note, please pick a different name than 'CompletionPromise', which has a special meaning in MozPromise.
Attachment #8671054 - Flags: feedback?(bobbyholley)
(In reply to Jean-Yves Avenard [:jya] from comment #27)
> Comment on attachment 8671054 [details] [diff] [review]
> Add mediasource memory reporter
> 
> Review of attachment 8671054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed.
> 
> Bobby, what do you think of using MozPromise::Private directly ?
> 
> ::: dom/media/MediaDecoder.h
> @@ +560,5 @@
> > +
> > +public:
> > +    mozilla::MallocSizeOf mMallocSizeOf;
> > +    mozilla::Atomic<size_t> mByteSize;
> > +    nsRefPtr<CompletionPromise::Private> mCallback;
> 
> You should use a MozPromiseRequestHolder /
> MozPromiseHolder<CompletionPromise> pair instead
> 
> So you can more strongly assert that everything has run its course properly.
> 
> So instead of the client accessing the promise::private directly, you access
> the MozPromiseRequestHolder instead, and ~ResourceSizes access the
> MozPromiseHolder.
> 
> I don't think bholley ever intended for MozPromise::Private to be used
> publicly, hence the name :)

Discussed w/ bholley, switched to MozPromiseHolder.

> ::: dom/media/mediasource/TrackBuffersManager.cpp
> @@ +2126,5 @@
> >  }
> >  
> > +void
> > +TrackBuffersManager::TrackData::AddSizeOfResources(MediaSourceDecoder::ResourceSizes* aSizes)
> > +{
> 
> MOZ_ASSERT(OnTaskQueue()); I know it's redundant, but we have this in all
> functions regarless

I originally attempted this, but struct TrackBuffersManager::TrackData does not have access to the non-static method |TrackBuffersManager::OnTaskQueue|.
(In reply to Jean-Yves Avenard [:jya] from comment #26)
> Comment on attachment 8671095 [details] [diff] [review]
> Add test for mediasource memory reporter
> 
> Review of attachment 8671095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/mediasource/test/test_MediaSource_memory_reporting.html
> @@ +19,5 @@
> > +    fetchWithXHR("seek.webm", (arrayBuffer) => {
> > +      loadSegment(sb, arrayBuffer).then(() => ms.endOfStream());
> > +    });
> > +
> > +    once(sb, "updateend", () => v.play());
> 
> you can replace it all with something like:
>     fetchAndLoad(sb, 'seek', [''], '.webm').then(() => v.play());
>     once(sb, "updateend", () => ms.endOfStream());
> 
> and continue on with... (it will have be moved up)
> 
>     // Test that memory reporting works now that we've played a video.
>     once(v, "ended", () => {
>       ...
> 
> you don't have to wait for updateend to start playback (two will be fired:
> one at the end of appendBuffer, and one after endOfStream())

That's definitely much simpler, I'll update to fetchAndLoad.

> And you could even get rid of the "once(sb, "updateend", () =>
> ms.endOfStream());" by waiting on the "stalled" event instead of "ended"

Perfect, I'll just used 'stalled' instead and remove the endOfStream call.
Final version w/ all review comments addressed.
Attachment #8671054 - Attachment is obsolete: true
Final version with all review comments addressed.
Attachment #8671095 - Attachment is obsolete: true
Comment on attachment 8671683 [details] [diff] [review]
Part 1: Add mediasource memory reporter

Carrying forward r+
Attachment #8671683 - Flags: review+
Comment on attachment 8671684 [details] [diff] [review]
Part 2: Add test for mediasource memory reporter

Carrying forward r+
Attachment #8671684 - Flags: review+
We're going to have to wait for bug 1194555 to land before landing this.
https://hg.mozilla.org/mozilla-central/rev/96810d75408d
https://hg.mozilla.org/mozilla-central/rev/65e8c6761f7a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.