Closed Bug 1193142 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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).
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.
Attachment #8646755 - Flags: review?(jyavenard)
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
Attachment #8646755 - Flags: review?(jyavenard) → review+
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()));
Attachment #8646755 - Flags: feedback?(jyavenard)
ah yes, good point.
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.
Attachment #8646755 - Flags: feedback?(jyavenard) → feedback+
Updated patch as per comment 2 and comment 3. Carrying r+.
Attachment #8646755 - Attachment is obsolete: true
Attachment #8646865 - Flags: review+
: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+.
Attachment #8646865 - Attachment is obsolete: true
Attachment #8646887 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/87347323281a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1197083
Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.