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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Sep 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1016b14
fix(DASH): Evict empty indexes in MetaSegmentIndex
theodab Sep 6, 2024
161118c
Responded to comments, and fixed performance.
theodab Sep 10, 2024
0405964
Remove unnecessary re-declaration of numEvicted_.
theodab Sep 10, 2024
1cc7c1c
Responded to more comments.
theodab Sep 10, 2024
d658928
Added basic unit test.
theodab Sep 10, 2024
dc8de6b
Also make test see if indexes are released.
theodab Sep 10, 2024
d5face7
Test getNumEvicted
theodab Sep 11, 2024
4584726
Fix error
avelad Sep 11, 2024
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
16 changes: 14 additions & 2 deletions lib/media/segment_index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,17 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
this.indexes_ = [];
}

/** Evicts all old SegmentIndexes in this MetaSegmentIndex that are empty. */
evictEmpty() {
while (this.indexes_.length > 1 && this.indexes_[0].isEmpty()) {
const index = this.indexes_.shift();
// In the case of this class, this.numEvicted_ represents the evicted
// segments that were in indexes that were entirely evicted.
this.numEvicted_ += index.getNumEvicted();
index.release();
}
}

/**
* Append a SegmentIndex to this MetaSegmentIndex. This effectively stitches
* the underlying Stream onto the end of the multi-Period Stream represented
Expand All @@ -724,6 +735,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 @@ -754,7 +766,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 @@ -775,7 +787,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
6 changes: 6 additions & 0 deletions lib/media/streaming_engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ goog.require('shaka.log');
goog.require('shaka.media.InitSegmentReference');
goog.require('shaka.media.ManifestParser');
goog.require('shaka.media.MediaSourceEngine');
goog.require('shaka.media.MetaSegmentIndex');
goog.require('shaka.media.SegmentIterator');
goog.require('shaka.media.SegmentReference');
goog.require('shaka.media.SegmentPrefetch');
Expand Down Expand Up @@ -2502,6 +2503,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
Loading