Eviction can cause Youtube playback stall

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments)

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.
A 75MB eviction threshold often causes stalls when playing YouTube's 4K content.
Attachment #8625355 - Flags: review?(ajones)
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!
Attachment #8625354 - Flags: review?(gsquelart) → review+
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?
Attachment #8625356 - Flags: review?(gsquelart) → review+
(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 !
Assignee: nobody → jyavenard
Attachment #8625355 - Flags: review?(ajones) → review+
Youtube is pushing a fix to what caused the original stall.
You need to log in before you can comment on or make changes to this bug.