Skip to content

Commit

Permalink
fix: Fix failed assertion for eviction logic
Browse files Browse the repository at this point in the history
In shaka-project#3169, we discovered failing assertions for some multi-Period DASH
live streams.  The failing assertion was meant to ensure that our
eviction logic was correct, and that our assumptions are maintained
properly.

Though the assertion failed, nothing was actually wrong with playback.
But the assertion itself should only fail if there is actually
something wrong.

With the choice between removing the assertion (since playback is
fine) or "fixing" the assertion (to avoid this false negative), I
chose to fix the assertion to retain the value of it to catch actual
bugs in our logic in the future.

The assertion specifies that you should not see evicted segments after
non-evicted segments.  But eviction was only done when merging a
segment index with an old version of the same index (same
Representation and Period).  When an update drops a Period from the
manifest, this means everything in that Period should be evicted.  But
the eviction call wouldn't happen, because we had no new version of
the segment index to update the old one.

An extra call to evict() on each manifest update can fix the state
of the system so that our assertions pass.

The assertion message appeared identically in two different
assertions, so this change also rewrites the assertion messages to
differentiate them and to clarify their meanings.

Issue shaka-project#3169

Change-Id: Ifeb7a1a8f48af704b028a22d987349209d7ee485
  • Loading branch information
joeyparrish committed Apr 21, 2021
1 parent 1dbd75a commit 29e7ab1
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 2 deletions.
13 changes: 13 additions & 0 deletions lib/dash/dash_parser.js
Expand Up @@ -345,6 +345,19 @@ shaka.dash.DashParser = class {
let presentationTimeline;
if (this.manifest_) {
presentationTimeline = this.manifest_.presentationTimeline;

// Before processing an update, evict from all segment indexes. Some of
// them may not get updated otherwise if their corresponding Period
// element has been dropped from the manifest since the last update.
// Without this, playback will still work, but this is necessary to
// maintain conditions that we assert on for multi-Period content.
// This gives us confidence that our state is maintained correctly, and
// that the complex logic of multi-Period eviction and period-flattening
// is correct. See also:
// https://github.com/google/shaka-player/issues/3169#issuecomment-823580634
for (const segmentIndex of Object.values(this.segmentIndexMap_)) {
segmentIndex.evict(presentationTimeline.getSegmentAvailabilityStart());
}
} else {
// DASH IOP v3.0 suggests using a default delay between minBufferTime
// and timeShiftBufferDepth. This is literally the range of all
Expand Down
4 changes: 2 additions & 2 deletions lib/media/segment_index.js
Expand Up @@ -595,7 +595,7 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
appendSegmentIndex(segmentIndex) {
goog.asserts.assert(
this.indexes_.length == 0 || segmentIndex.numEvicted == 0,
'Cannot have evicted segments in non-first Periods');
'Should not append a new segment index with already-evicted segments');
this.indexes_.push(segmentIndex);
}

Expand Down Expand Up @@ -655,7 +655,7 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
for (const index of this.indexes_) {
goog.asserts.assert(
!sawSegments || index.numEvicted == 0,
'Cannot have evicted segments in non-first Periods');
'Should not see evicted segments after available segments');
const reference = index.get(position - numPassedInEarlierIndexes);

if (reference) {
Expand Down

0 comments on commit 29e7ab1

Please sign in to comment.