1158658 - Flush MSE SourceBufferResource data to disk when Dormant

Status: RESOLVED FIXED

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

Description

[cajbir (:cajbir)]

When a MediaSource becomes dormant we should flush data held by SourceBufferResource to disk. When it comes out of dormant we can restore it from disk. This is to help reduce memory footprint of inactive MediaSource videos.

Comment 1

[cajbir (:cajbir)]

Attachment: Dump data to disk when dormant

When mediasource goes dormant the data in the SourceBufferResource is stored in temporary files on disk and the data removed from memory. When coming out of dormant the data is restored and the temporary files removed.

Comment 2

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

Comment on attachment 8604405 [details] [diff] [review]
Dump data to disk when dormant

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

The guts of it look fine, but you're doing the I/O on the main thread, which we're not supposed to do.

::: dom/media/mediasource/MediaSource.cpp
@@ +572,5 @@
> +  if (mSourceBuffers) {
> +    mSourceBuffers->ReturnFromDormant();
> +  }
> +}
> +

Nit: two blank lines here.

::: dom/media/mediasource/MediaSourceDecoder.cpp
@@ +370,5 @@
> +  if (mMediaSource) {
> +    if (isDormant && !IsDormant()) {
> +      mMediaSource->ReturnFromDormant();
> +    }
> +    else if(!isDormant && IsDormant()) {

Style guide [1] says this should be

} else if (!IsDormant && IsDormant()) {


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +371,5 @@
> +    if (isDormant && !IsDormant()) {
> +      mMediaSource->ReturnFromDormant();
> +    }
> +    else if(!isDormant && IsDormant()) {
> +      mMediaSource->BecomeDormant();

There's a bigger problem here; if I understand the object hierarchy here, you'll be doing I/O on the main thread, which is forbidden (and even fatal assertion inducing last time I tried) in B2G.

I think you should be initiating the dump-to-disk from MediaSourceReader::ReleaseMediaResources(). You'll need that to set a flag, and when we come out of dormant we call MediaSourceReader::ReadMetadata() again and you can check the flag and re-load data from disk.

::: dom/media/mediasource/ResourceQueue.cpp
@@ +153,5 @@
>  
>  size_t
>  ResourceQueue::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
>  {
> +  EnsureNotDormant();

Why do you need to EnsureNotDormant() here? That would mean you'll actually load everything in from disk unnecessarily?

If you can safely remove this call, I think you should.

@@ +188,5 @@
>  
> +void
> +ResourceQueue::BecomeDormant()
> +{
> +  if (!mIsDormant) {

I think this should be:

if (mIsDormant) {
  return;
}

So that you can reduce the indent in the rest of the function. Ditto for ReturnFromDormant() below.

@@ +264,5 @@
> +void
> +ResourceQueue::EnsureNotDormant() const
> +{
> +  if (mIsDormant) {
> +    const_cast<ResourceQueue*>(this)->ReturnFromDormant();

My preference would be to unconstify the callers rather than make const annotations untruthful.

Comment 3

[cajbir (:cajbir)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> The guts of it look fine, but you're doing the I/O on the main thread, which
> we're not supposed to do.

Yeah, I discussed this with Anthony that it was main thread and we thought it would be fine for this case - I'll change it to be non-main thread as we weren't aware of the restriction.

> I think you should be initiating the dump-to-disk from
> MediaSourceReader::ReleaseMediaResources(). You'll need that to set a flag,
> and when we come out of dormant we call MediaSourceReader::ReadMetadata()
> again and you can check the flag and re-load data from disk.

Note that the data is restored not just when we come out of dormant. It's also restored whenever any method on ResourceQueue that touches the data is used. I'll assert to see if any is called on the main thread.

> ::: dom/media/mediasource/ResourceQueue.cpp
> @@ +153,5 @@
> >  
> >  size_t
> >  ResourceQueue::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> >  {
> > +  EnsureNotDormant();
> 
> Why do you need to EnsureNotDormant() here? That would mean you'll actually
> load everything in from disk unnecessarily?

Because it iterates over the the ResourceItem's which don't exist when dormant. I'll see if I can refactor things so that we keep the size information around.

> 
> @@ +264,5 @@
> > +void
> > +ResourceQueue::EnsureNotDormant() const
> > +{
> > +  if (mIsDormant) {
> > +    const_cast<ResourceQueue*>(this)->ReturnFromDormant();
> 
> My preference would be to unconstify the callers rather than make const
> annotations untruthful.

I was keeping logical constness here vs physical constness. To the user of the API it should appear const.

Comment 4

[Jean-Yves Avenard [:jya]]

this bug is no longer relevant ; we do not use the SourceBufferResource to hold data any longer.