Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(DASH): Evict empty indexes in MetaSegmentIndex #7272

Merged
merged 8 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions lib/media/segment_index.js
Original file line number Diff line number Diff line change
Expand Up @@ -698,25 +698,29 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {

/** @private {!Array.<!shaka.media.SegmentIndex>} */
this.indexes_ = [];

/**
* Represents the number of evicted segments from segment indexes that have
* been evicted out of the indexes array.
* @private {number}
*/
this.numEvicted_ = 0
theodab marked this conversation as resolved.
Show resolved Hide resolved
}

/** Evicts all old SegmentIndexes in this MetaSegmentIndex that are empty. */
evictEmpty() {
// If we hypothetically have a segment index that is empty but comes after
// one that has references, assume it's new and leave it in.
const newIndexes = [];
let include = false;
for (const index of this.indexes_) {
if (!index.isEmpty()) {
include = true;
}
if (include) {
newIndexes.push(index);
} else {
index.release();
}
if (this.indexes_.length == 0) {
return;
}
this.indexes_ = newIndexes;
const prevNewestIndex = this.indexes_[this.indexes_.length - 1];
while (this.indexes_.length > 0 && this.indexes_[0].isEmpty()) {
theodab marked this conversation as resolved.
Show resolved Hide resolved
const index = this.indexes_.shift();
this.numEvicted_ += index.getNumEvicted();
index.release();
}
goog.asserts.assert(
this.indexes_[this.indexes_.length - 1] == prevNewestIndex,
'The newest segment index should never be evicted.');
}

/**
Expand All @@ -743,6 +747,7 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
// Be careful to clone the Array. We don't want to share the reference with
// our clone and affect each other accidentally.
clone.indexes_ = this.indexes_.slice();
clone.numEvicted_ = this.numEvicted_;
return clone;
}

Expand Down Expand Up @@ -773,7 +778,7 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
* @export
*/
find(time) {
let numPassedInEarlierIndexes = 0;
let numPassedInEarlierIndexes = this.numEvicted_;

for (const index of this.indexes_) {
const position = index.find(time);
Expand All @@ -794,7 +799,7 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
* @export
*/
get(position) {
let numPassedInEarlierIndexes = 0;
let numPassedInEarlierIndexes = this.numEvicted_;
let sawSegments = false;

for (const index of this.indexes_) {
Expand Down
5 changes: 5 additions & 0 deletions lib/media/streaming_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -2502,6 +2502,11 @@ shaka.media.StreamingEngine = class {
* @private
*/
async evict_(mediaState, presentationTime) {
const segmentIndex = mediaState.stream.segmentIndex;
if (segmentIndex instanceof shaka.media.MetaSegmentIndex) {
segmentIndex.evictEmpty();
}
Comment on lines +2506 to +2509
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you move it here due to performance reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial revision of this PR I put together in a hurry before lunch, and didn't get a good chance to test it for more than a minute or two. It turned out the place I was calling evictEmpty originally was just the wrong function, it wasn't being called often enough.


const logPrefix = shaka.media.StreamingEngine.logPrefix_(mediaState);
shaka.log.v2(logPrefix, 'checking buffer length');

Expand Down
1 change: 0 additions & 1 deletion lib/util/periods.js
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,6 @@ shaka.util.PeriodCombiner = class {
outputStream.segmentIndex.appendSegmentIndex(match.segmentIndex);
}
}
outputStream.segmentIndex.evictEmpty();
}
}

Expand Down
Loading