1193142 - TrackBuffersManager::DoEvictData does not need to keep a keyframe at the end

Status: RESOLVED FIXED

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

Description

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

In TrackBuffersManager::DoEvictData, the 2nd step (evicting trailing data) currently evicts from the first keyframe *after* the frame at which enough data would have been evicted.
Because of this:
- Less data than requested could be evicted,
- Some sequences of events could end up leaving lonely keyframes (with gaps on both sides) in the buffer, which is effectively useless. (It did help highlight some edge case issues, e.g. bug 1192791)

Instead the algorithm should just try and remove as many frames as needed to reach the requested eviction request (plus other dependent frames as required).

Comment 1

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

Attachment: 1193142-evict-more-trailing-frames.patch

Evict as many trailing frames as needed (if possible) to reach the eviction request, instead of evicting only everything after the first keyframe in the eviction blast radius.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8646755 [details] [diff] [review]
1193142-evict-more-trailing-frames.patch

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

r=me with comment addressed

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +472,4 @@
>      MSE_DEBUG("Step2. Evicting %u bytes from trailing data",
>                mSizeSourceBuffer - finalSize);
>      CodedFrameRemoval(
> +      TimeInterval(TimeUnit::FromMicroseconds(buffer[lastFrameIndex]->GetEndTime() + 1),

the + 1 is unnecessary.
as the end time of an interval is always exclusive

Comment 3

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

Comment on attachment 8646755 [details] [diff] [review]
1193142-evict-more-trailing-frames.patch

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8646755 [details] [diff] [review]
> 1193142-evict-more-trailing-frames.patch
> 
> Review of attachment 8646755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed
> 
> ::: dom/media/mediasource/TrackBuffersManager.cpp
> @@ +474,2 @@
> >      CodedFrameRemoval(
> > +      TimeInterval(TimeUnit::FromMicroseconds(buffer[lastFrameIndex]->GetEndTime() + 1),
> 
> the + 1 is unnecessary.
> as the end time of an interval is always exclusive

Note that this one is the start time of the interval to evict, so I think it's inclusive.

Anyway this made me realize:
The current code removes everything after 'lastFrameIndex', not including it (which was what we wanted before, to keep the keyframe).
Shouldn't we remove that frame as well, otherwise we wouldn't evict enough data?

Also I'm tempted to rename the slightly-misleading 'lastFrameIndex' to 'evictedFramesStartIndex'. Do you agree?

So would end up with:
> CodedFrameRemoval(
>   TimeInterval(
>     TimeUnit::FromMicroseconds(buffer[evictedFramesStartIndex]->mTime),
>     TimeUnit::FromInfinity()));

Comment 4

[Jean-Yves Avenard [:jya]]

ah yes, good point.

Comment 5

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

Comment on attachment 8646755 [details] [diff] [review]
1193142-evict-more-trailing-frames.patch

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> ah yes, good point.

Taking that is "feedback+" :-)
Will post updated patch soon.

Comment 6

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

Attachment: 1193142-evict-more-trailing-frames.patch v2

Updated patch as per comment 2 and comment 3. Carrying r+.

Comment 7

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

Attachment: 1193142-evict-more-trailing-frames.patch v3

:jya noticed that at the time we want to break from the loop, the current frame should *not* be evicted (because it's within a range we want to keep, or the previously considered frame reached the requested eviction amount). Carrying r+.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b53fdfade971

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87347323281a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87347323281a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87347323281a

Comment 12

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdc74c64c9c9

Comment 13

[Ralph Giles (:rillian)]

Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c

Comment 14

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd742b6c5596