1176178 - Eviction can cause Youtube playback stall

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

We evict data in two steps.

The first one is to remove data before our playback position, and if we still haven't freed enough memory we will evict at the end.

When we evict data at the end, Youtube doesn't appear to check that data was removed and continue to append data without consideration of the data removed. This creates a gap in the samples and later a stall.

While this appears to be more an issue with how YouTube doesn't fully follow the spec, we need to find an eviction strategy that doesn't break their player.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Be slightly less aggressive when evicting data.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Bump eviction threshold to 100MiB.

A 75MB eviction threshold often causes stalls when playing YouTube's 4K content.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P3. Returns error as per spec if eviction failed.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8625354 [details] [diff] [review]
P1. Be slightly less aggressive when evicting data.

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

r+ with comments addressed (all minor except the last one).

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +344,5 @@
>                                   uint32_t aSizeToEvict)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>  
> +  int64_t finalSize = mSizeSourceBuffer - aSizeToEvict;

finalSize is first used 30+ lines below, you should move this statement closer to where it's needed. Unless you think it's important to know about this early.

@@ +350,5 @@
>    // Video is what takes the most space, only evict there if we have video.
>    const auto& track = HasVideo() ? mVideoTracks : mAudioTracks;
>    const auto& buffer = track.mBuffers.LastElement();
> +  // Remove any data we've already played, or next sample to be demuxed
> +  // whichever is lowest.

I think you mean "or before the next sample to be demuxed".

@@ +372,5 @@
>      partialEvict += sizeof(*frame) + frame->mSize;
>    }
>    if (lastKeyFrameIndex > 0) {
> +    MSE_DEBUG("Step1. Evicting %u bytes prior currentTime",
> +              aSizeToEvict - toEvict);

Wouldn't "prior currentTime" be misleading if lowerLimit is based on track.mNextSampleTime?

@@ +384,5 @@
>  
> +  toEvict = mSizeSourceBuffer - finalSize;
> +
> +  // Still some to remove. Remove data starting from the end, up to 30s ahead
> +  // of our playtime. 30s is a value chosen as it appears to work with YouTube.

"of our playtime" -> "of the later of the playback time or the next sample to be demuxed"

@@ +402,5 @@
>        break;
>      }
>      partialEvict += sizeof(*frame) + frame->mSize;
>    }
> +  if (lastKeyFrameIndex < buffer.Length()) {

Unless you initialize lastKeyFrame to buffer.Length() or more before the loop, this test will always be true. And you could end up removing more frames than needed, based on the value in the previous loop!

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8625356 [details] [diff] [review]
P3. Returns error as per spec if eviction failed.

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

r+ with comments addressed, especially this:
In TrackBuffersManager there are separate Atomics for mBufferFull and mEvictionOccurred; Are you confident that there cannot be some race conditions?
E.g.:
1. CodedFrameRemoval runs until |if (mSizeSourceBuffer < mEvictionThreshold) { mBufferFull = false; }|
2. CompleteCodedFrameProcessing takes over, adds some data, and runs at least until |if (mSizeSourceBuffer >= mEvictionThreshold) { mBufferFull = true; mEvictionOccurred = false; }|
3. CodedFrameRemoval resumes and runs |mEvictionOccurred = true;|
So now with have mBufferFull==true and mEvictionOccurred==true, which means the next EvictData will just return without trying to evict anything.
Probably unlikely, but just in case, you may want to combine them into one atomic value (enum/field set/number with 4 values).

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +596,5 @@
>    mSizeSourceBuffer = mVideoTracks.mSizeBuffer + mAudioTracks.mSizeBuffer;
>  
> +  // 4. If buffer full flag equals true and this object is ready to accept more bytes, then set the buffer full flag to false.
> +  if (mSizeSourceBuffer < mEvictionThreshold) {
> +    mBufferFull = false;

Comment says "if buffer full flag equals true", but code actually doesn't test for it.

@@ +1201,5 @@
>  
>    // Return to step 6.4 of Segment Parser Loop algorithm
>    // 4. If this SourceBuffer is full and cannot accept more media data, then set the buffer full flag to true.
> +  if (mSizeSourceBuffer >= mEvictionThreshold) {
> +    mBufferFull = false;

Don't you mean mBufferFull = true, as per the above comment?

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #5)
> Comment on attachment 8625356 [details] [diff] [review]
> P3. Returns error as per spec if eviction failed.
> 
> Review of attachment 8625356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed, especially this:
> In TrackBuffersManager there are separate Atomics for mBufferFull and
> mEvictionOccurred; Are you confident that there cannot be some race
> conditions?
> E.g.:
> 1. CodedFrameRemoval runs until |if (mSizeSourceBuffer < mEvictionThreshold)
> { mBufferFull = false; }|
> 2. CompleteCodedFrameProcessing takes over, adds some data, and runs at
> least until |if (mSizeSourceBuffer >= mEvictionThreshold) { mBufferFull =
> true; mEvictionOccurred = false; }|
> 3. CodedFrameRemoval resumes and runs |mEvictionOccurred = true;|
> So now with have mBufferFull==true and mEvictionOccurred==true, which means
> the next EvictData will just return without trying to evict anything.
> Probably unlikely, but just in case, you may want to combine them into one
> atomic value (enum/field set/number with 4 values).
> 

this can't ever happen.

Only one task can run at a time, no other task can be started until the previous task has completed its run. This is controlled by the SourceBuffer which only start a new task if mUpdating is false. mUpdating is set to true when a task is started, and to false once the trackbuffer promise (either rangeremoval or buffer append).


> ::: dom/media/mediasource/TrackBuffersManager.cpp
> @@ +596,5 @@
> >    mSizeSourceBuffer = mVideoTracks.mSizeBuffer + mAudioTracks.mSizeBuffer;
> >  
> > +  // 4. If buffer full flag equals true and this object is ready to accept more bytes, then set the buffer full flag to false.
> > +  if (mSizeSourceBuffer < mEvictionThreshold) {
> > +    mBufferFull = false;
> 
> Comment says "if buffer full flag equals true", but code actually doesn't
> test for it.

I don't see much advantage in doing:
if (bufferfull == true) {
  bufferfull = false;
}


> 
> @@ +1201,5 @@
> >  
> >    // Return to step 6.4 of Segment Parser Loop algorithm
> >    // 4. If this SourceBuffer is full and cannot accept more media data, then set the buffer full flag to true.
> > +  if (mSizeSourceBuffer >= mEvictionThreshold) {
> > +    mBufferFull = false;
> 
> Don't you mean mBufferFull = true, as per the above comment?

good catch !

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9fe30a0b56
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db419aa68c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/57483a7b271a

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e9fe30a0b56
https://hg.mozilla.org/mozilla-central/rev/9db419aa68c2
https://hg.mozilla.org/mozilla-central/rev/57483a7b271a

Comment 9

[Jean-Yves Avenard [:jya]]

Youtube is pushing a fix to what caused the original stall.