1055904 - Improve MSE eviction calculation

Status: RESOLVED FIXED

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

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Make eviction use a byte-accurate offset for frame eviction. In the MP4 case this is the first byte of the moof.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Improve eviction calculations

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8475643 [details] [diff] [review]
Improve eviction calculations

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

I moved ResourceQueue into ResourceQueue.h, so this will need to be rebased.

::: content/media/mediasource/SourceBuffer.cpp
@@ +486,5 @@
> +    if (ranges->Length()) {
> +      MSE_DEBUG(
> +        "SourceBuffer(%p)::AppendBuffer Evict; current buffered start=%f", this,
> +        GetBufferedStart());
> +      mMediaSource->NotifyEvicted(0.0, ranges->GetStartTime());

This chunk seems unnecessary, the GetBufferedStart() helper that was being used takes care of this for you.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> ::: content/media/mediasource/SourceBuffer.cpp
> @@ +486,5 @@
> > +    if (ranges->Length()) {
> > +      MSE_DEBUG(
> > +        "SourceBuffer(%p)::AppendBuffer Evict; current buffered start=%f", this,
> > +        GetBufferedStart());
> > +      mMediaSource->NotifyEvicted(0.0, ranges->GetStartTime());
> 
> This chunk seems unnecessary, the GetBufferedStart() helper that was being
> used takes care of this for you.

GetBufferedStart() doesn't appear to be returning the correct value. I need to investigate further.

Comment 4

[Matthew Gregan [:kinetik]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> GetBufferedStart() doesn't appear to be returning the correct value. I need
> to investigate further.

Ah, perhaps because SB::GetBuffered() changed behaviour and you're using MD::GetBuffered() directly.  The issue may be that we need to do the eviction on all of the decoders in mDecoders, not just the current mDecoder.  Then GetBufferedStart() should report the expected time.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> GetBufferedStart() doesn't appear to be returning the correct value. I need
> to investigate further.

Have you had a chance to investigate this?

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I've appeneded it to jya's queue.

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Bug 1065235 may make this irrelevant.

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #7)
> Bug 1065235 may make this irrelevant.

It's not intended to; that bug is "just" applying the current eviction code to all of the TrackBuffer's decoders rather than just the current one.

Comment 9

[Chris Peterson [:cpeterson]]

This bug is necessary for basic playback of EME video on Windows.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Chris - close this bug if you're using another bug.

Comment 11

[cajbir (:cajbir)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=44cd358c7d67

Comment 12

[cajbir (:cajbir)]

Attachment: Improve MSE eviction calculation

Fixes a bug in the SourceBufferResource eviction code where it was
using the mOffset of the resource as the min bound for what to evict.
This offset is almost always zero though due to ReadFromCache being
used which never updates the offset. This prevented eviction from
happening in most cases.

Adds optimisations to remove decoders that have an end time that
is less that the start time. This fixes the case where a user
seeks forward often. The old decoders will now be cleaned up.

Keeps track of the time that was evicted from the current decoder
and uses that as the time to EvictBefore for all decoders in the
track buffer when doing MediaSource::NotifyEvict.

I tested manually to ensure eviction occurs with the about:media page patch in bug 1112424 applied. I went to an MP4 video and started playing then viewed the decoder data in about:media. It shows the byte size of each decoder. You should see the 'start' time of the active decoder increasing as eviction occurs. Older decoders should start to disappear too. Some tests are:

1) Play video then seek forward past the bufferdd range by 10 second or so. View about:media and notice there is a new decoder. On the next append data the old one should be reclaimed.

2) Play video then seek backwards past the currentTime and past the start time of the existing decoder. Do this a few times. You should see in about:media a few decoders appear then go away on eviction.

3) Play video then switch playback resolution at some point. A new decoder will be created and eventually the old one should be reclaimed.

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8547891 [details] [diff] [review]
Improve MSE eviction calculation

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

Good spotting.

::: dom/media/mediasource/SourceBuffer.cpp
@@ +395,5 @@
>                this, GetBufferedStart());
>  
>      // We notify that we've evicted from the time range 0 through to
>      // the current start point.
> +    mMediaSource->NotifyEvicted(0.0, evictTime);

shouldn't the comment be updated ? or rename evictTime? it's hard to tell that it is the new starting point mTrackBuffer->EvictData will return

::: dom/media/mediasource/TrackBuffer.cpp
@@ +275,5 @@
>        RemoveDecoder(decoders[i]);
>      }
> +
> +    if (decoders[i] == mCurrentDecoder) {
> +      currentDecoderIndex = i;

So we continue to evict even if we have evicted more than we had to?
why continue the loop?

We would potentially be calling decoders[i]->GetResource()->EvictData on the next iteration with a negative value, as a int32 -> uint32 conversion occurs, we would now evict everything.
SourceBufferResource::EvictData attempts to evict "at least aSizeToEvict"

Also, ConvertToByteOffset currently returns a very approximated value, I fear that we could be evicting data located after our current play position. Maybe should have some leeways to make sure this doesn't happen.

@@ +290,5 @@
>        // TODO: We could implement forward-eviction within a decoder and
>        // be able to evict within the current decoder.
> +      if (decoders[i] == mCurrentDecoder) {
> +        break;
> +      }

Why break rather than just skip mCurrentDecoder ?
Any particular reason we wouldn't be evicting on earlier decoders?

@@ +319,5 @@
> +          break;
> +        }
> +      }
> +    }
> +  }

I think we should be doing this as first step, as it's the one likely going to free the highest amount of unused data.

@@ +327,5 @@
> +  if (evicted) {
> +    nsRefPtr<TimeRanges> ranges = new TimeRanges();
> +    mCurrentDecoder->GetBuffered(ranges);
> +    *aEvictTime = std::max(0.0, ranges->GetStartTime());
> +  }

What we have done is evict data before aPlaybackTime, why not return this value instead?

::: dom/media/mediasource/TrackBuffer.h
@@ +39,5 @@
>    // Append data to the current decoder.  Also responsible for calling
>    // NotifyDataArrived on the decoder to keep buffered range computation up
>    // to date.  Returns false if the append failed.
>    bool AppendData(const uint8_t* aData, uint32_t aLength, int64_t aTimestampOffset /* microseconds */);
> +  bool EvictData(double aPlaybackTime, uint32_t aThreshold, double* aEvictTime);

I think this argument should be renamed to be more explicit. It is going to be the buffered start of the current decoder.

And this function is in need of a comment

Comment 14

[cajbir (:cajbir)]

I'm addressing the review comments. Also fixing a bug where data can be removed from a decoder that has an active reader causing stalls.

Comment 15

[cajbir (:cajbir)]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
> shouldn't the comment be updated ? or rename evictTime? it's hard to tell
> that it is the new starting point mTrackBuffer->EvictData will return

I've changed the name in the patch to be uploaded.

> So we continue to evict even if we have evicted more than we had to?
> why continue the loop?

I've changed to break out of the loop if we have evicted enough.

> Also, ConvertToByteOffset currently returns a very approximated value, I
> fear that we could be evicting data located after our current play position.
> Maybe should have some leeways to make sure this doesn't happen.

I added a leeway.

> Any particular reason we wouldn't be evicting on earlier decoders?

We are assuming data added after the currently active decoder is new data that might want to be used. We can revisit this is another bug if needed I think.

> I think we should be doing this as first step, as it's the one likely going
> to free the highest amount of unused data.

I've modified it to do this in the same iteration loop so that it will remove all data from a decoded if it's considered not needed or evict a portion if neeed.

> @@ +327,5 @@
> > +  if (evicted) {
> > +    nsRefPtr<TimeRanges> ranges = new TimeRanges();
> > +    mCurrentDecoder->GetBuffered(ranges);
> > +    *aEvictTime = std::max(0.0, ranges->GetStartTime());
> > +  }
> 
> What we have done is evict data before aPlaybackTime, why not return this
> value instead?

Because we have an eviction threshold less data may have been evicted and the start time of the buffer will not always be aPlaybackTime.

> I think this argument should be renamed to be more explicit. It is going to
> be the buffered start of the current decoder.
> 
> And this function is in need of a comment

Done.

Comment 16

[cajbir (:cajbir)]

Attachment: Fix with review comments addressed

Addresses review comments and fixed a couple of other issues as noted in the commit comment below:

    Fixes a bug in the SourceBufferResource eviction code where it was
    using the mOffset of the resource as the min bound for what to evict.
    This offset is almost always zero though due to ReadFromCache being
    used which never updates the offset. This prevented eviction from
    happening in most cases.
    
    Moves the code to remove old decoders so that it does this during
    the same loop as that which remove data from existing decoders.
    This more aggressively prunes old decoders and is more likely to
    keep data in the current playing decoder around for seeking, etc.
    
    Prevent removing any decoder that the MediaSourceReader is
    currently using for playback to prevent RemoveDecoder crashes.
    
    Add a threshold to subtract from the current time when working out
    the time bound to evict before to make it less likely to evict
    current data that is needed for current playback.
    
    Remove all data from evicted decoders in the initial iteration then
    iterate after to remove empty decoders to put the RemoveDecoder
    logic in one place.
    
    Iterate decoders in order that they were added rather than sorted
    by time so the logic that removes entire decoders can do it only
    to those old decoders that existed before the existing one was
    created.

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8549325 [details] [diff] [review]
Fix with review comments addressed

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

I think you should go back to your previous approach, the removal of all decoders, simply because they have an earlier start time, regardless of their end buffered is I believe, too aggressive...

But I don't want to delay this landing.

So r+ with the tiny nit addressed. And whatever you deemed necessary.

::: dom/media/mediasource/SourceBufferResource.h
@@ +120,5 @@
>    // Remove data from resource before the given offset.
>    void EvictBefore(uint64_t aOffset);
>  
> +  // REmove all data from the resource
> +  uint32_t EvictAll();

nit: s/REmove/Remove

::: dom/media/mediasource/TrackBuffer.cpp
@@ +288,5 @@
> +      // Remove data from older decoders than the current one. Don't remove data if
> +      // it is currently active.
> +      MSE_DEBUG("TrackBuffer(%p)::EvictData evicting all before start bufferedStart=%f bufferedEnd=%f aPlaybackTime=%f size=%lld",
> +                this, buffered->GetStartTime(), buffered->GetEndTime(), aPlaybackTime, decoders[i]->GetResource()->GetSize());
> +      toEvict -= decoders[i]->GetResource()->EvictAll();

this is super aggressive... And you can evict data in the future we could later play.
you also have evicted data past the [0, currentTime] eviction window notification.

What about the earlier approach of only removing data located before the current play time ; and removing all decoders whose end times were before our current end time.

What you had before seems much better to me.

@@ +293,2 @@
>      }
> +    else {

nit: } else {

@@ +296,5 @@
> +      // of a few seconds back and evict data up to that point.
> +      if (aPlaybackTime > MSE_EVICT_THRESHOLD_TIME) {
> +        double time = aPlaybackTime - MSE_EVICT_THRESHOLD_TIME;
> +        int64_t playbackOffset = decoders[i]->ConvertToByteOffset(time);
> +        MSE_DEBUG("TrackBuffer(%p)::EvictData evicting some bufferedEnd=%f aPlaybackTime=%f time=%f, playbackOffset=%lld size=%lld",

80 columns formatting (using my rillian's hat here) and in all code above and below

@@ +309,5 @@
> +  // Remove decoders that have no data in them
> +  for (i = 0; i < decoders.Length(); ++i) {
> +    nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> +    decoders[i]->GetBuffered(buffered);
> +     MSE_DEBUG("TrackBuffer(%p):EvictData maybe remove empty decoders=%d size=%lld start=%f end=%f",

nit: extra space at the beggining

@@ +334,1 @@
>    return toEvict < (totalSize - aThreshold);

return evicted;

@@ +342,5 @@
>    for (uint32_t i = 0; i < mInitializedDecoders.Length(); ++i) {
>      int64_t endOffset = mInitializedDecoders[i]->ConvertToByteOffset(aTime);
>      if (endOffset > 0) {
>        MSE_DEBUG("TrackBuffer(%p)::EvictBefore decoder=%u offset=%lld", this, i, endOffset);
>        mInitializedDecoders[i]->GetResource()->EvictBefore(endOffset);

They are empty, why not remove them while at it ?
Any particular reason to stop doing that?

Comment 18

[cajbir (:cajbir)]

(In reply to Jean-Yves Avenard [:jya] from comment #17)
> 
> I think you should go back to your previous approach, the removal of all
> decoders, simply because they have an earlier start time, regardless of
> their end buffered is I believe, too aggressive...

Yeah, it's aggressive - especially if the eviction threshold is set small. At 75MB it works out ok though. And the site should be checking if data was evicted and downloading it if needed. 

I think we should come up with better heuristics for eviction for sure and we'll do this in a different bug.

Nits have been corrected and I'll update the patch shortly.

> this is super aggressive... And you can evict data in the future we could
> later play.
> you also have evicted data past the [0, currentTime] eviction window
> notification.

This approach was added in a previous patch to cater for this case where someone seeks backwards in small increments. It would end up growing the number of decoders a lot and they weren't evicted due the start/end times not being in the right ranges. We should revisit after doing some simulations/analysis I think.

> >    for (uint32_t i = 0; i < mInitializedDecoders.Length(); ++i) {
> >      int64_t endOffset = mInitializedDecoders[i]->ConvertToByteOffset(aTime);
> >      if (endOffset > 0) {
> >        MSE_DEBUG("TrackBuffer(%p)::EvictBefore decoder=%u offset=%lld", this, i, endOffset);
> >        mInitializedDecoders[i]->GetResource()->EvictBefore(endOffset);
> 
> They are empty, why not remove them while at it ?
> Any particular reason to stop doing that?

I stopped doing it because RemoveDecoder removes it from mInitializedDecoders which we are also iterating over at the time. I think mInitializedDecoders has to be stable while iterating. They'll be removed if they are empty in a later eviction. I could also copy mInitializedDecoders, iterate over the copy and remove afterwards as we did before but since we remove on later eviction I left it for that.

Thanks for the review!

Comment 19

[cajbir (:cajbir)]

Attachment: Fix with review comments addressed

Address review comments, carry forward r+.

Comment 21

[cajbir (:cajbir)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2604b0655e0d

Comment 22

[cajbir (:cajbir)]

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

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8550024 [details] [diff] [review]
Fix with review comments addressed

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Youtube playback uses more resources than necessary.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This is moderately invasive, but MSE-specific and has been in testing for some time.
[String/UUID change made/needed]: None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/31aac3cd543e
https://hg.mozilla.org/releases/mozilla-beta/rev/595835cd60a0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

And a non-unified bustage fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/def96daddf8c
https://hg.mozilla.org/releases/mozilla-beta/rev/c703f90c5b80

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6ac12d931a

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c6ac12d931a